Optional Dependencies decorator - #476

Well since you already did the work I don't see why not. Though I'd like to focus on getting done what we have left for 0.2 rather than adding new work :)

(BTW I haven't actually reviewed this yet but I plan to)

Well, if you look closely at the commits, I did much of the work months ago and just never issued a PR because I was planning on expanding it more (in ways I now think might have been unwise, anyway). So I'm not prioritizing this over other things, I promise :)

Hey I'm not your boss, I'm not telling you how to spend your time :) I'm just anxious to get this release out. And I have tons to do myself >_>

@mdboom @astrofrog, what do you think about this decorator? Do you want to use this approach for tagging optional dependencies?

Lets move this discussion to the general comments so it doesn't get wiped out by commits, and may involve major changes... @iguananaut is concerned about using the wrapper, but I'm still not completely clear what the concern is: is it that you want all optional dependencies to be imported at the top of the module in which they are used? I think this is unwise because it will be very expensive to do this on import of major subpackages. Also, in that way of doing things, if you call a function that uses say, scipy, instead of getting an ImportError, you get an ambiguous AttributeError that doesn't really mean anything to a user.

That said, I can see the argument for just completely eliminating the wrapper and instead just attaching the name of the optional dependency and just using find_all_optional_dependencies when we want to know the optional dependencies. Then the cost is really only borne in that function, which was the most important aspect intent of this scheme anyway (to mark functions for utilities to figure out what all the opdeps are and which are and aren't installed). And then when someone calls the function they get an ImportError that is more clearly a sign you need to install something.

@astrofrog - do you have thoughts on either of these items?

The problem I have mainly comes down to the fact that the import is not attempted until the function that uses it is called. This in turn delays the user from finding out whether or not a required dependency is available, and could occur deep within some running code, rather than at startup time.

I see the argument about importing optional dependencies slowing down initial imports. But so long as we just do top-level package imports such as import scipy or import matplotlib it's not too bad. Those libraries are fast to import since they don't import many submodules. Other libraries I can't directly speak to, but those are the two I think we're most concerned with right now.

There are other advantages to attempting to import dependencies immediately. For example the decorator can actually be used to update a function's docstring, such that if that function is missing one of its dependencies a big fat warning can be added to the top: "WARNING: This function depends on a missing package 'scipy' and is disabled." Or something of the like. Rather than raising an ImportError the function could raise a NotImplementedError or something else more specific (MissingDependencyError?)

Alternatively, if you don't want to do the imports immediately, we could instead use pkg_resources. For example:

try:
    dist = pkg_resources.get_distribution('matplotlib')
except pkg_resources.DistributionNotFound:
    # Do something else

Then you can still delay the actual import until the function is run. This will work on most modern Python installations. Though it's not guaranteed either, so if import pkg_resources doesn't work we may still want to fall back on test-importing the packages. I still also prefer the imports at the top for aesthetic reasons--it makes it easier to see what packages are used by the code in a module, even if it's optional.

Hmm, I guess we just see things differently here. I think it's much better if when the users starts up astropy, it works as long as they have numpy, and they only start seeing errors about missing optional dependencies when they try to use them. That way if I just want astropy for, say, units on a system where scipy is difficult to install, I don't end up with an error message about scipy. From the user perspective its irrelevant that you need scipy to do some things if you never actually want to do those things.

Of course, the point here is that we do want to keep track of optional dependencies for developer needs (e.g. listing them in the docs, making scripts that will install them if needed, etc).

I wonder if maybe I should post this on astropy-dev given the lack of responses here? This seems a rather far-reaching question, potentially.

Just to be sure, I think you might be misunderstanding me?
I'm not suggesting there be an unadorned import scipy at the top of a module that might use it. If it's an optional import it should still be something like:

try:
    import scipy
except ImportError:
    # Do something else...

And in that case I suppose that if the dependency isn't checked for until the function is run it could still come as a surprise. But this at least gives us the opportunity to warn the user that the function won't be available, either in the docstring (my preference) or maybe even with an (optional) warning message at import time.

Right, that was what I thought you meant - I'm saying that doesn't solve the problem that this PR needs to solve - an automated way to identify all of astropy's opdeps, and causes the problem that the user then gets a vague AttributeError about scipy not being in the global namespace rather than an ImportError that makes it clear they're missing a needed package. So I was saying one solution is for # Do something else to give a warning and/or somehow record the relevant opdep, but then I was saying the user will often get warnings when they don't actually apply (e.g. they import cosmology but don't use the part of it that needs scipy)

So @iguananaut - I see a few options that we could do:

1. Something like this PR, where we have a decorator to decorate functionsthat is always run as a wrapper whenever those functions are called, and issues a custom error if the optional dependency is missing.
2. A "lite" version of this PR, where we still decorate at the function or class level, but it just adds some metadata to the function that find_all_optional_dependencies uses to identify the opdeps, and the function/class will just end up throwing an ordinary ImportError if the opdep is missing (or really is free to do whatever it wants).
3. We tell everyone to put their optional dependencies in a try ... except ImportError block with specific semantics that something like find_all_optional_dependencies can still use to identify the opdeps at a module-level.

As that all the cases we've discussed here, or do you want to suggest another? Once we've got a clear set of options, I'll post to astropy-dev and hopefully we can pick one.

@iguananaut - pinging you on the above discussion. If I don't hear from you soonish I'll just go-ahead and ask on astropy-dev

Sorry, I've been traveling the last several days. I prefer a combination of option 3, and a class/function decorator that adds information to the docstring when the dependencies for that object are not met, plus any other instrumentation we may need/want, and to raise an exception that's more helpful than just an ImportError.

Either way, I think this is worth bringing to the attention of astropy-dev if you haven't already.

I totally thought I'd commented on this before, but apparently not... I think I would favor an approach where one would use a decorator that is just used to collect information about dependencies, but doesn't do anything magical. If developers have to follow a specific try...except syntax, they could just as well just add a decorator which advertises which optional dependencies are needed, and it avoids any kind of code inspection (which I would prefer). I don't feel too strongly about this though.

Thanks @astrofrog and @iguananaut - my preference is 2 because it seems simplest (and hence most likely that people will actually use it), and has the least performance impact, and fine-grained control of where the opdeps apply, but I also like @iguananaut's idea of also intelligently updating the docstring when it makes sense to do so.

And I posted to astropy-dev, so we'll see what people say.

@iguananaut @eteq - I feel like we should re-schedule this to 0.2.1 - it's not critical, as it's only going to be informational at this stage.

@astrofrog - perhaps... the reason to include it now is that if we get a sudden increase in contributors after 0.2, we want them to start using this rather than having to go back by hand and fix it. But that's probably not a big deal because they would all be working from the dev version if they need this, anyway. And I may be overly-optimistic with the "sudden increase in contributors" scenario :)

I don't think we need to worry about a sudden increase in contributions, and we'll want to limit contributions relying on optional dependencies anyway. I'm re-tagging this as 0.2.1 as we have enough major issues to deal with for 0.2.0.

vo package will use it, but it is still under development.

I don't really think this needs to be done by 0.3.0 so re-tagging as 0.4.0. Feel free to change back if you think it's ready and everyone agrees it should be merged.

It may be that this should just be dropped completely, as the need for it hasn't really clearly materialized... perhaps if it doesn't seem necessary still by 0.4 we can just close it without merging.

Sounds good!

@eteq - just sorting through some old issues, since the need for this hasn't really come up since we last discussed it, shall we close?

@astrofrog - Yeah, if it becomes a problem down the road if we accumulate non-scipy dependencies, we might resurrect it, but you're right at this point it seems unlikely. So closing!