Skip to content

Shouldn't the try: __import__ in __init__ re-raise original exception? #23868

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

Closed
haphaeu opened this issue Nov 23, 2018 · 7 comments · Fixed by #26665 or #26685
Closed

Shouldn't the try: __import__ in __init__ re-raise original exception? #23868

haphaeu opened this issue Nov 23, 2018 · 7 comments · Fixed by #26665 or #26685
Labels
Error Reporting Incorrect or improved errors from pandas good first issue Needs Discussion Requires discussion from core team before further action
Milestone

Comments

@haphaeu
Copy link

haphaeu commented Nov 23, 2018

Problem description

It is confusing when import pandas raises a message like:

>>> import pandas
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/raf/anaconda3/lib/python3.7/site-packages/pandas/__init__.py", line 19, in <module>
    "Missing required dependencies {0}".format(missing_dependencies))
ImportError: Missing required dependencies ['pytz']

Where actually the true error is not in pytz, but in a missing library required by pytz:

>>> import pytz
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/raf/anaconda3/lib/python3.7/site-packages/pytz/__init__.py", line 20, in <module>
    from pytz.tzinfo import unpickler, BaseTzInfo
  File "/home/raf/anaconda3/lib/python3.7/site-packages/pytz/tzinfo.py", line 4, in <module>
    from bisect import bisect_right
ImportError: cannot import name 'bisect_right' from 'bisect' (/home/raf/tmp/bisect.py)

As you can see, I happen to have a file called bisect.py in the same folder where I'm trying to import pandas, but the error message raised by pandas doesn't reflect that. It was a silly error after all, but It gave me a headache to sort that out.

Tracking back the source of it, in pandas/__init__.py, note that the original error e is not used after except ImportError as e. I think it should.

for dependency in hard_dependencies:
    try:
        __import__(dependency)
    except ImportError as e:
        missing_dependencies.append(dependency)

if missing_dependencies:
    raise ImportError(
        "Missing required dependencies {0}".format(missing_dependencies))

Expected Output

raise ImportError(...) should mention the error e raised by __import__(dependency)


Question also asked in SO:
https://stackoverflow.com/questions/53445555/shouldnt-the-try-import-in-init-re-raise-original-exception

@WillAyd
Copy link
Member

WillAyd commented Nov 23, 2018

I'm -1 here as I think this is somewhat nuanced. At the end of the day pytz wasn't fully installed, hence the original error so it's not as if pandas is misleading in that regards.

@WillAyd WillAyd added Error Reporting Incorrect or improved errors from pandas Needs Discussion Requires discussion from core team before further action labels Nov 23, 2018
@gfyoung
Copy link
Member

gfyoung commented Nov 24, 2018

I actually wouldn't be too opposed to clarifying this. Would it be so bad to print out all relevant error messages related to each dependency that failed?

@DanielFEvans
Copy link
Contributor

DanielFEvans commented Jun 5, 2019

A +1 for this. I do think Pandas is being misleading in this case - I've just spend a good ten minutes trying to figure out why Pandas thinks Numpy isn't installed when pip freeze says it is, before finding out it's masking an underlying ImportError in Numpy (which I think is caused by Conda shenanigans).

I don't see any particular advantage to what Pandas is doing - why not just allow the underlying ImportError to be raised, whether it's the requirement itself missing or some dependency of that, like just about every other package? Turning an ImportError into a potentially less informative ImportError doesn't seem to be very useful.

If you really, really want to have the little message that can give a single message saying that all three of numpy, pytz, and dateutil missing, I'd be tempted to go with:

for dependency in hard_dependencies:
    try:
        __import__(dependency)
    except ImportError as e:
        expected_msg = 'cannot import name {}'.format(dependency)
        if expected_msg not in str(e):
            # Dependency installed but raised a separate ImportError
            raise
        missing_dependencies.append(dependency)

@TomAugspurger
Copy link
Contributor

We're python 3 only now. All we need to do is


except ImportError as e:
    raise ImportError(...) from e

and the full traceback is printed. Are you interested in working on it @DanielFEvans?

@TomAugspurger
Copy link
Contributor

I'm not sure of an easy way to test this. I suspect the current behavior is untested.

@DanielFEvans
Copy link
Contributor

Is there any need to raise a separate import error - i.e. what would Pandas add that isn't already there? If there's a default ImportError that already says that it couldn't find numpy, I'm not sure there's much more to say.

(This is different from some other cases further down in __init__.py, where the try-catch gives some hints suggesting that compiled code is missing).

I'd be happy to give this one a go - seems like a fairly simple one to start with!

@TomAugspurger
Copy link
Contributor

Ah, sorry, I misunderstood the issue. The idea is to raise a single error message with all of the import errors, so #23868 (comment) won't quite work.

I think the simplest thing is to include the original exception in the missing_dependencies.append(dependency). So something like missing_dependencies.append((dependency, e))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas good first issue Needs Discussion Requires discussion from core team before further action
Projects
None yet
6 participants