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

Matplotlib dependency not listed in setup.py #14

Open
rmjarvis opened this issue Oct 4, 2016 · 5 comments
Open

Matplotlib dependency not listed in setup.py #14

rmjarvis opened this issue Oct 4, 2016 · 5 comments

Comments

@rmjarvis
Copy link

rmjarvis commented Oct 4, 2016

When setting up Travis CI for GalSim, I discovered that starlink-pyast has a hidden dependency of matplotlib at the top of Grf.py. Since none of the other packages we use in GalSim require matplotlib, it didn't get installed on the Travis sandbox, and import starlink.Atl led to an ImportError from the matplotlib dependency.

Even with matplotlib installed, it's still an annoying import, since it has to set up the font cache the first time it is imported. This takes time and adds noise to the output:

/home/travis/virtualenv/python2.7.12/lib/python2.7/site-packages/matplotlib/font_manager.py:273:
UserWarning: Matplotlib is building the font cache using fc-list. This may take a moment.
  warnings.warn('Matplotlib is building the font cache using fc-list. This may take a moment.')

Possible solutions in order of (my) preference:

  1. Make matplotlib an optional dependency and only import Grf (and thus matplotlib) if the user calls Atl.plotframeset. i.e. Move the line import starlink.Grf as Grf into this function.
  2. Make matplotlib an optional dependency and only import matplotlib in the Grf functions that use it. Then import starlink.Grf as Grf is quick, and matplotlib would only have to be imported if the user calls the functions in Grf that need it.
  3. Make matplotlib an official dependency by adding it to setup.py, but also do (1) or (2) to avoid the above warning.
  4. Make matplotlib an official dependency, and continue to needlessly import matplotlib whenever Atl is imported.

Thanks for your consideration. :)

@timj
Copy link
Member

timj commented Oct 4, 2016

Sorry about that. I'll take a look.

@timj
Copy link
Member

timj commented Oct 12, 2016

I've had a quick look and I think I would be inclined to fix the Atl interface so that it does something like:

try:
    import starlink.Grf as Grf
except ImportError:
    Grf = None

and then raise an exception in the plotframeset routine if Grf is None. Ideally we would have put the ATL plot routines in a different support package but it's a bit late to do that now.

@rmjarvis
Copy link
Author

I suppose that would solve the problem, but it seems less than ideal from an end user's perspective, since it swallows the traceback for the ImportError. If the user thought they had matplotlib installed, this wouldn't give them any feedback as to why import starlink.Grf is failing.

Why not just put import starlink.Grf in the plotframeset function itself? Is this just to conform to PEP8, or is there a more concrete reason to prefer putting this at the top?

Remember, "A foolish consistency is..." not always the best choice. :)

@timj
Copy link
Member

timj commented Oct 12, 2016

Hmmm. Yes, I tend to prefer imports at the top of code files rather than buried in the middle, but in this case, given that the failure mode is identical (you get an unexpected error some time after your program starts rather than at import time), I guess having the real error trigger inside plotframeset is better than an error we make up ourselves when plotframeset is called.

@timj
Copy link
Member

timj commented Oct 16, 2016

Ok, I'm sleep-addled but I've put in an attempted fix in #15.

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

No branches or pull requests

2 participants