Skip to content
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

Re-raise exception when NumPy/SciPy imports fail #181

Merged
merged 3 commits into from
Jun 9, 2020

Conversation

mattwthompson
Copy link
Contributor

The check for NumPy/SciPy installs in setup.py prints out a helpful message for the user and exits (so that the rest of the install attempt does not proceed) but raises exit code 0 when it should probably raise 1. This may cause some issues downstream when these packages are not installed properly but this error is only printed to stdout or a logfile but doesn't stop a conda build from happening.

Just to demonstrate this, since it can't easily be added to CI (echo $? prints the exit status of the last command, which I just learned by working on this)

bash-3.2$ python -c "exit()"
bash-3.2$ echo $?
0
bash-3.2$ python -c "raise Exception"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
Exception
bash-3.2$ echo $?
1

And here's what it looks like to the user before and after this patch

(omnia) [forcebalance] python setup.py install                                                                                                                                                                                        master
Error importing numpy and scipy but these are required to install ForceBalance
Please make sure the numpy and scipy modules are installed and try again
(omnia) [forcebalance] echo $?                                                                                                                                                                                                        master
0
(omnia) [forcebalance] git checkout loud-importerror                                                                                                                                                                                  master
Switched to branch 'loud-importerror'
(omnia) [forcebalance] python setup.py install                                                                                                                                                                              loud-importerror
Traceback (most recent call last):
  File "setup.py", line 16, in <module>
    import numpy
ModuleNotFoundError: No module named 'numpy'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "setup.py", line 23, in <module>
    raise ImportError(msg) from e
ImportError:
Error importing numpy and scipy but these are required to install ForceBalance.
Please make sure the numpy and scipy modules are installed and try again.

(omnia) [forcebalance] echo $?                                                                                                                                                                                              loud-importerror
1

@leeping
Copy link
Owner

leeping commented Jun 9, 2020

Thank you, Matt! Is there somewhere I can learn more about the as e/from e syntax? One of the tests has a failure that I think is stochastic so I'm rerunning it.

@mattwthompson
Copy link
Contributor Author

I mostly grabbed the solution from here and the official docs are here. The intention with this approach was to re-raise the same exception. as e catches the exception, and from e makes clear where the raised exception come from. This is my first time chaining exceptions, but I think it's working as intended; the ModuleNotFoundError is printed as the cause of the ImportError (which itself could be a ModuleNotFoundError, I suppose).

This is all a little fluffy, to be honest; simply changing the exit() to an exception would function just about as well.

@leeping
Copy link
Owner

leeping commented Jun 9, 2020

I can appreciate why one would want the original exception, it could help with debugging. :) Thanks for the explanation!

@mattwthompson
Copy link
Contributor Author

Ah, but it looks like that's not Python 2.7 compatible. This should work now (works locally, at least)

@leeping
Copy link
Owner

leeping commented Jun 9, 2020

Thanks. I plan on keeping Python 2.7 support for a while because you never know who is still using it. My guess is there are many research groups that still use it.

@leeping
Copy link
Owner

leeping commented Jun 9, 2020

LGTM. Thank you!

@leeping leeping merged commit 743a7d6 into leeping:master Jun 9, 2020
jstoppelman pushed a commit to jstoppelman/forcebalance that referenced this pull request Jul 31, 2020
Re-raise exception when NumPy/SciPy imports fail
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants