Skip to content

Check if Dask is installed #62

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Sep 9, 2024
Merged

Check if Dask is installed #62

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fc27229
Check if Dask is installed
jacobtomlinson Aug 6, 2024
b445c38
Explicity check for None
jacobtomlinson Aug 6, 2024
62f672b
Turn dask_array_type into a tuple to simplify isinstance check
weiji14 Sep 3, 2024
133f194
Merge branch 'main' into no-dask
weiji14 Sep 3, 2024
f82e829
Add DuckArrayTypes type hint
weiji14 Sep 9, 2024
ec25b8f
Import TYPE_CHECKING and Any from typing
weiji14 Sep 9, 2024
bdd3be1
Put dask_array_type type hint under if TYPE_CHECKING block
weiji14 Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cupy_xarray/accessors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Member

@keewis keewis Aug 6, 2024 •
edited
Loading

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
Member

@keewis keewis Aug 6, 2024 •
edited
Loading

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 •
edited
Loading

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
Member

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
Member

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 •
edited
Loading

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?

return isinstance(self.da.data._meta, cp.ndarray)
return isinstance(self.da.data, cp.ndarray)

Expand All @@ -61,7 +61,7 @@ def as_cupy(self):
<class 'cupy.ndarray'>

"""
if isinstance(self.da.data, dask_array_type):
if dask_array_type is not None and isinstance(self.da.data, dask_array_type):
return DataArray(
data=self.da.data.map_blocks(cp.asarray),
coords=self.da.coords,
Expand All @@ -87,7 +87,7 @@ def as_numpy(self):
DataArray with underlying data cast to numpy.
"""
if self.is_cupy:
if isinstance(self.da.data, dask_array_type):
if dask_array_type is not None and isinstance(self.da.data, dask_array_type):
return DataArray(
data=self.da.data.map_blocks(
lambda block: block.get(), dtype=self.da.data._meta.dtype
Expand Down