ENH: avoid importing optional dependencies just to know if they are installed#15771
ENH: avoid importing optional dependencies just to know if they are installed#15771mhvk merged 3 commits intoastropy:mainfrom
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
That seems like a clear improvement! (but note small mistake) From a quick test, it looks like find_spec is indeed substantially faster.
p.s. I also wondered about doing
present = globals()[name] = findspec(_deps[name[4:]]) is not None
return present
so that the result is remembered for the next call. But am not sure that saves much time after this!
| except (ImportError, ModuleNotFoundError): | ||
| return False | ||
| return True | ||
| return find_spec(name.removeprefix("HAS_").lower()) is not None |
There was a problem hiding this comment.
This should be find_spec(_deps[name[4:]])
There was a problem hiding this comment.
at the risk of bike shedding, isn't .removeprefix("HAS_") clearer than [4:] (albeit ever so slightly slower) ?
There was a problem hiding this comment.
(though getting the name for _deps is clearly required, so I applied that fix)
There was a problem hiding this comment.
Yes, I agree, more obvious what it does.
pllim
added
units
Refactoring
no-changelog-entry-needed
and removed
visualization
labels
neutrinoceros
force-pushed
the
rfc/lazier_optional_deps
branch
from
f426128 to
37b74e8
Compare
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Nice improvement, now all good!
|
Well, I approved too quickly: quite a few failures! |
|
Indeed ! I just learned that diff --git a/astropy/utils/compat/optional_deps.py b/astropy/utils/compat/optional_deps.py
index b079a82e99..08140b4439 100644
--- a/astropy/utils/compat/optional_deps.py
+++ b/astropy/utils/compat/optional_deps.py
@@ -37,7 +37,7 @@ _optional_deps = [
_deps = {k.upper(): k for k in _optional_deps}
# Any subpackages that have different import behavior:
-_deps["PLT"] = "matplotlib.pyplot"
+_deps["PLT"] = "matplotlib"
__all__ = [f"HAS_{pkg}" for pkg in _deps]which seems reasonable to me, since I highly doubt that |
|
pushing the fix now to prove my point. I may be away until Wednesday, so feel free to push/merge/close as needed ! |
|
pre-commit.ci autofix |
|
OK, now all tests pass and this looks like a very obvious improvement, so merging! |
3 participants
Description
Inspired by #13821 (comment)
I noticed that we could define
HAS_<optional_dep>constant more lazily without breaking tests, though maybe this isn't desired in the overall context of #13821 ?I bet @mhvk has opinions here.