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

optional dependencies (matplotlib and scipy) are not truly optional #1361

Closed
rathann opened this issue May 22, 2017 · 8 comments
Closed

optional dependencies (matplotlib and scipy) are not truly optional #1361

rathann opened this issue May 22, 2017 · 8 comments

Comments

@rathann
Copy link
Contributor

rathann commented May 22, 2017

Expected behaviour

The lack of any of the optional dependencies listed in setup.py (netcdf4, matplotlib, scipy, seaborn and sklearn) is handled gracefully, with no exceptions thrown.

Actual behaviour

ImportError exception will be thrown when matplotlib or scipy are missing.

Code to reproduce the behaviour

The following imports don't catch ImportErrors when importing scipy:

MDAnalysis/analysis/psa.py:1936
MDAnalysis/analysis/psa.py:2053
MDAnalysis/analysis/polymer.py:169
MDAnalysis/analysis/pca.py:355
MDAnalysis/analysis/hbonds/hbond_autocorrel.py:443
MDAnalysis/visualization/streamlines_3D.py:42
MDAnalysis/visualization/streamlines_3D.py:43
MDAnalysis/visualization/streamlines.py:48

and matplotlib:

MDAnalysis/analysis/psa.py:1646
MDAnalysis/analysis/psa.py:1747
MDAnalysis/analysis/psa.py:1847
MDAnalysis/analysis/psa.py:1935
MDAnalysis/analysis/polymer.py:138
MDAnalysis/analysis/hole.py:372
MDAnalysis/analysis/hole.py:451
MDAnalysis/analysis/hole.py:520
MDAnalysis/analysis/legacy/x3dna.py:416

Currently version of MDAnalysis:

0.16.0

@kain88-de
Copy link
Member

There isn't really a way for us to handle this more graceful then throwing an exception in those cases. For example in the pca case. If we can't import scipy that function can not return a suitable result because missing the integrator is a grave error justifying an exception.

How would you imagine here a graceful error handling?

@richardjgowers
Copy link
Member

So something we could do here would be to do all the imports at the top of the file, then raise warnings about things that won't work. Eg in polymer, lots of it will work, but certain methods will need things, so a warning like ImportWarning: The following methods require scipy: PersistenceLength.curve_fit.

@rathann
Copy link
Contributor Author

rathann commented May 23, 2017

I do note that some of the missing optional dependencies are already handled nicely in the code. Thank you for that.

@kain88-de I'm just trying to express package dependencies correctly and let users uninstall dependencies listed in setup.py under extras_require. Having the software throw exceptions in case of missing optional dependencies is bad user experience and I'd like to avoid that. Perhaps some of these dependencies shouldn't be listed as optional, then.

@richardjgowers Yes, that'd be awesome.

@mnmelo
Copy link
Member

mnmelo commented May 23, 2017

So, this idea was already floated in #577, but we dropped when we realized it'd probably involve registering import hooks.

In the meantime I found the importing package, which has a lazyModule. Their implementation is hook-independent, and delays errors until the module is actually used.

I figure we can subclass lazyModule to customize error messages, to print something like "A functionality of MDAnalysis requiring module so-and-so was requested, but said module is not installed. Please install and retry.".

@mnmelo
Copy link
Member

mnmelo commented May 23, 2017

Using importing.lazyModule has the further benefit of not immediately loading potentially heavy optional dependencies (like scipy) until they're actually needed.

@orbeckst
Copy link
Member

There's some discussion in #1159 , which we haven't really concluded. I think we are moving towards making more "core scientific python packages" (see #1159 (comment) for qualifiers of this term...) standard requirements of MDAnalysis.

@orbeckst
Copy link
Member

Under this scheme, scipy and matplotlib would become required dependencies. I think, given Linux whl and conda I could live with this, especially, as these packages are only imported when a specific analysis module is imported; I therefore don't think that #1361 (comment)

not immediately loading potentially heavy optional dependencies (like scipy) until they're actually needed.

is a big issue. But if lazyModule works well, then we could try it – anything to make a user's life easier.

@orbeckst
Copy link
Member

If we decide that scipy and matplotlib are required dependencies #1159 (comment) then we can close this issue. (We can then still look into other approaches like lazyModule but at least we would have solved this immediate problem.)

orbeckst added a commit that referenced this issue Jun 16, 2017
- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes #1159
- fixes #1361
orbeckst added a commit that referenced this issue Jun 16, 2017
- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes #1159
- fixes #1361
kain88-de pushed a commit that referenced this issue Jun 17, 2017
- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes #1159
- fixes #1361
orbeckst added a commit that referenced this issue Jun 18, 2017
- require scipy and matplotlib in setup.py (fixes #1361)
- scipy and matplotlib are imported at top in analysis
  - updated all modules
  - removed any code that guards against scipy or matplotlib import
  - conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
  - fixes #1159
- removed conditional skipping of tests when scipy or matplotlib are missing
kain88-de pushed a commit that referenced this issue Jun 21, 2017
- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes #1159
- fixes #1361
kain88-de pushed a commit that referenced this issue Jun 21, 2017
- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes #1159
- fixes #1361
mnmelo added a commit that referenced this issue Jun 21, 2017
This now addresses issues #577 (facultative imports), #1361 (cleaner
optional dependencies) and #1159 (optional dependencies in the analysis
module)
utkbansal pushed a commit to utkbansal/mdanalysis that referenced this issue Jun 24, 2017
- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes MDAnalysis#1159
- fixes MDAnalysis#1361
utkbansal pushed a commit to utkbansal/mdanalysis that referenced this issue Jun 24, 2017
- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes MDAnalysis#1159
- fixes MDAnalysis#1361
utkbansal pushed a commit to utkbansal/mdanalysis that referenced this issue Jun 24, 2017
Converted test_units to use parametrize

Some hacking of test_distances to use pytest features

Completed moving test_distances to pytest style

rewrote test_transformations into pytest style

make scipy and matplotlib full dependencies (MDAnalysis#1159)

scipy and matplotlib are imported at top in analysis

- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes MDAnalysis#1159
- fixes MDAnalysis#1361

removed conditional skipping of tests when scipy or matplotlib are missing

minor clean ups
utkbansal pushed a commit to utkbansal/mdanalysis that referenced this issue Jun 24, 2017
Converted test_units to use parametrize

Some hacking of test_distances to use pytest features

Completed moving test_distances to pytest style

rewrote test_transformations into pytest style

make scipy and matplotlib full dependencies (MDAnalysis#1159)

scipy and matplotlib are imported at top in analysis

- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes MDAnalysis#1159
- fixes MDAnalysis#1361

removed conditional skipping of tests when scipy or matplotlib are missing

minor clean ups
utkbansal pushed a commit to utkbansal/mdanalysis that referenced this issue Jun 24, 2017
Converted test_units to use parametrize

Some hacking of test_distances to use pytest features

Completed moving test_distances to pytest style

rewrote test_transformations into pytest style

make scipy and matplotlib full dependencies (MDAnalysis#1159)

scipy and matplotlib are imported at top in analysis

- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes MDAnalysis#1159
- fixes MDAnalysis#1361

removed conditional skipping of tests when scipy or matplotlib are missing

minor clean ups
orbeckst added a commit that referenced this issue Jul 3, 2017
We do not need the copy of of scipy.io.netcdf that was added in
68ff0d5 because scipy is now
a full dependency (see #1361)
PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this issue Jan 22, 2019
- updated all modules
- removed any code that guards against scipy or matplotlib import
- conforms to style guide https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#module-imports-in-mdanalysisanalysis
- fixes MDAnalysis#1159
- fixes MDAnalysis#1361
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants