-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Soft import #9561
base: main
Are you sure you want to change the base?
Soft import #9561
Conversation
Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems excellent!
(though others know this code better, so will wait for them to comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is a great contribution!
Can we resolve merge conflicts and then merge? |
resolved conflict stemming from pydata#9505 by accepting this branches changes (pydata#9505 was just a slight edit on the old try import .. except pattern)
@max-sixty sorry about that! I just pushed a commit ( 3c5d35d ) to resolve the merge conflict, which stemmed from #9505. That PR had touched one of the |
Marking as draft until I can look into the failures. I think the proposed pinging @hoechenberger as the person driving the static type checking adoption in MNE-Python.. Do you recall whether MNE's |
@scott-huberty I don't remember details, but I do recall those soft imports to be an absolute nightmare to work with when using static type checkers, and I gave up trying to add those type hints at one point. |
I think it is basically impossible to have such imports work properly with static type checking. The imported modules will all be Any (ModuleType is effectively Any). |
Yes. It also means that you won't receive useful help from your IDE during development. I'd advise against this change, but that's just my very personal opinion without knowing any of the considerations that went into this work. |
Ah.. OK, thanks @hoechenberger ! Yes agreed if this proposal breaks Xarray functionality then maybe it's not a good idea after all.. I'll let the devs make the call on that. |
Agree it doesn't make sense if we really lose all type checking. but is it really not possible? Couldn't we even have a |
In the Something like
But this ofc doesn't protect against import errors. |
I tried that and never got it to work. I think the language just doesn't allow for this. |
@headtr1ck I think I see what you are suggesting. So for example if we just start out with the optional dependencies that get imported a lot across the codebase (and thus could benefit from a dedicated function; e.g. def check_installed(name, purpose, strict=True):
is_installed = module_available(name)
if not is_installed and strict:
raise ImportError(
f"For {purpose}, {name} is required. Please install it via pip or conda."
)
return is_installed
def check_cftime_installed(strict=True):
"""Check if cftime is installed, otherwise raise an ImportError."""
purpose = "working with dates with non-standard calendars"
return check_installed("cftime", purpose, strict=strict)
def some_function_that_needs_cftime():
check_cftime_intalled()
import cftime
... It's basically just a wrapper around the |
You are right. Maybe it makes more sense to add the new functionality to Maybe the TYPE_CHECKING idea is indeed better?
But somehow all of this is adding a lot of boilerplate for just an improved error message, but I guess that's what the issue is for... |
One proposal, small change to the current version, retaining type checking and integration with
I agree it's a bit of boilerplate, but I do think the experience is much better for newer users. And if we do this across the library it'll be an easily recognizable pattern. |
Thanks both! I think the If folks don't want the boilerplate, one last idea is something like:
Which would work fine with |
Unfortunately that doesn't work, because the return type is simply |
whats-new.rst
api.rst
I haven't replaced all individual instances of
try: import optional_dep ... except: raise
, but wanted to get this on the board earlier rather than later so folks have time to give feedback!soft_import
toxarray.core.utils
. It makes use of a function you already have namedmodule_available
.soft_import
more than once or twice for the same optional dependency, I created a helper function for it that is just a thin wrapper aroundsoft_import