-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: Improve ImportError message to suggest importing dependencies directly for full error details #61084
base: main
Are you sure you want to change the base?
BUG: Improve ImportError message to suggest importing dependencies directly for full error details #61084
Conversation
Hi @rhshadrach , I have implemented your suggestion from the #61030. Could you please verify if this implementation meets your expectations? Thank you! Below are example outputs before and after this change:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mroeschke - what do you think about adding tzdata
to the _hard_dependencies
on L6 here.
pandas/__init__.py
Outdated
"Unable to import required dependencies:\n" + "\n".join(_missing_dependencies) | ||
"Unable to import required dependencies:\n" | ||
+ "\n".join(_missing_dependencies) | ||
+ "\n\nTo see the full error message, " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You can remove the +
on each line; Python will perform string concatenation automatically.
Also, I think we should remove one of the \n
here:
ImportError: Unable to import required dependencies:
dateutil: No module named 'dateutil'
To see the full error message, try importing the missing dependencies directly.
vs
ImportError: Unable to import required dependencies:
dateutil: No module named 'dateutil'
To see the full error message, try importing the missing dependencies directly.
By removing the blank line, it is more apparent to me that it's all part of the same error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm what if we just change this to
for _dependency in _hard_dependencies:
try:
__import__(_dependency)
except ImportError as _e: # pragma: no cover
raise ImportError(f"Unable to import required dependency {_dependency} ...") from _e
I know we won't be able to collect all the missing dependencies all at once but at least we'll maintain the traceback of the failed import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mroeschke - I'm good with your suggested approach, especially given how few hard dependencies pandas has.
Yeah we should probably add that too. |
@mroeschke For |
Please create a separate issue for this |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.