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

Check if Dask is installed #62

Merged
merged 7 commits into from
Sep 9, 2024
Merged

Conversation

jacobtomlinson
Copy link
Collaborator

Closes #61

If Dask is not installed then dask_array_type will be set to None. This PR checks this variable before using it.

@@ -34,7 +34,7 @@ def is_cupy(self):
is_cupy: bool
Whether the underlying data is a cupy array.
"""
if isinstance(self.da.data, dask_array_type):
if dask_array_type is not None and isinstance(self.da.data, dask_array_type):
Copy link
Contributor

@keewis keewis Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could also set dask_array_type to (), isinstance(variable, ()) always evaluates to False (though to be consistent, dask_array_type could also be set to a 1-tuple if dask.array is available)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we could do that, but it feels less readable. Particularly for more junior developers.

My preference would be the more clear is not None, but happy to defer to folks who are more active here.

Copy link
Contributor

@keewis keewis Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is in a try/except it should be clear that this is a fallback, and I do feel that the check for None somewhat diverts the attention from the purpose of the condition, which is to compare types.

As for the trick, a comment just before the assignment should help.

(in any case, I'm even less active here)

Copy link
Member

@weiji14 weiji14 Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't want to go here, but we could also refactor the try/except import to have a _HAS_DASK boolean so that mypy doesn't complain. Xref https://adamj.eu/tech/2021/12/29/python-type-hints-optional-imports/

try:
    import dask.array
    
    _HAS_DASK = True
except ImportError:
    _HAS_DASK = False

Then the check would become:

    if _HAS_DASK:
        if isinstance(self.da.data, dask.array.Array):

but I'm happy enough with the is not None check for now, and we could refactor to the style above later when there's a mypy CI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that link appears to be broken, I get a 404

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, was missing an 's' at the end, fixed now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is fine with mypy (we're using it in xarray.namedarray, which is the most thoroughly typed part of xarray – at least as far as I can tell):

if TYPE_CHECKING:
    DuckArrayTypes = tuple[type[Any], ...]

dask_array_type: DuckArrayTypes
try:
    import dask.array

    dask_array_type = (dask.array.Array,)
except ImportError:
    dask_array_type = ()

Copy link
Member

@weiji14 weiji14 Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If xarray.namedarray uses tuples, I think cupy-xarray should use it too! The developers in this repo should be fairly competent, so I think we can figure out the syntax. @jacobtomlinson, do you have time to apply this change? Or I can push to this branch also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might not have time to get back here quickly (getting ready to go on vacation). So if you don't mind pushing here that would probably get things resolved more quickly. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, was travelling for a bit last month too, but finally got around to this again. @keewis, can you check to see if the changes look ok?

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Edit: See suggested change above at #62 (comment) first.

Copy link
Contributor

@keewis keewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got one (optional) comment about typing, otherwise this looks good to me

cupy_xarray/accessors.py Show resolved Hide resolved
@weiji14 weiji14 added the bug Something isn't working label Sep 9, 2024
@weiji14 weiji14 merged commit c34feeb into xarray-contrib:main Sep 9, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

is_cupy raises TypeError if dask is not installed
3 participants