Change isinstance checks to duck Dask Array checks #4208 #4221

rpmanser · 2020-07-13T21:39:46Z

Closes Support for duck Dask Arrays #4208
Tests added
Passes isort -rc . && black . && mypy . && flake8

I added the discussed is_duck_dask_array(x) function and replaced isinstance(x, dask_array_type) checks with is_duck_dask_array(x). Existing tests are passing. I did not change isinstance(x, dask_array_type) checks in the testing modules since I am unsure if that is appropriate.

As for additional tests, I am leaning towards testing Xarray ( Pint ( Dask ) ) ) objects as implemented in upcoming Pint v0.15 rather than a mock class, but I am open to discussion. I am unfamiliar with the release schedules of Pint and Xarray but I imagine that would influence which direction the testing should go.

jthielen

Thank you for going through and finding/adjusting all of these Dask checks!

I've left some comments below, which, other than the one about is_duck_dask_array still using isinstance instead of is_dask_collection, are mostly there to help get discussion going on a few points.

xarray/core/dask_array_compat.py

xarray/core/duck_array_ops.py

xarray/core/formatting.py

xarray/core/indexing.py

xarray/core/nanops.py

xarray/core/rolling_exp.py

xarray/core/variable.py

jthielen · 2020-07-14T00:22:09Z

Also, a broader discussion that's I've seen hinted to in the past, but is brought to the forefront by this PR: how should xarray be checking for general duck array types it can wrap?

Right now, it looks like there is a mix of

is_array_like(data)

and

hasattr(data, "__array_function__") or isinstance(data, dask_array_type)

and it would be nice to bring consistency. However, both these checks seem like they'd let too many types through. For example, is_array_like as currently implemented would still let through too much, such as MemoryCachedArray and xarray Variables/DataArrays, and __array_function__ doesn't truly indicate a NumPy duck array on its own (and it really only works to only capture downcast types right now since xarray does not yet implement __array_function__ itself, #3917).

keewis · 2020-07-14T11:37:23Z

Revisiting it I now realize that is_array_like is actually not a good name for that function: officially, "array_like" means that it can be casted using np.asarray. Maybe we should rename that to is_duck_array? But then we'd also need to check for __array_ufunc__ and __array_function__ because we also need those to properly wrap duck arrays.

jthielen · 2020-07-14T12:03:05Z

Revisiting it I now realize that is_array_like is actually not a good name for that function: officially, "array_like" means that it can be casted using np.asarray. Maybe we should rename that to is_duck_array? But then we'd also need to check for __array_ufunc__ and __array_function__ because we also need those to properly wrap duck arrays.

As long as that doesn't break any of the current uses, I think that would be the best way forwards. This would require xarray to be on NumPy 1.16+ (in order to ensure hasattr(np.ndarray, "__array_function__"), but that's only 9 days away by NEP 29, so hopefully that isn't too much of an issue. Then, most of the checks for "can this be wrapped by xarray" should be able to become something like

is_duck_array(x) and not isinstance(x, (DataArray, Variable))

with is_duck_dask_array only being needed for checking Dask Array-like support.

Although, while we're at it, do we also need more careful handling of upcast types? I'm not sure if there even are any out there right now (not sure if a HoloViews Dataset counts here), but that doesn't necessarily mean there never will be any.

pep8speaks · 2020-07-14T15:25:43Z

Hello @rpmanser! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-31 21:28:01 UTC

rpmanser · 2020-07-15T19:01:17Z

I'm getting a bit lost within the testing suite while trying to figure out the implications of this change and what kinds of tests are necessary for it. Would a test module for this

Individually test all functions that include the newly implemented is_duck_dask_array()?
Test that a duck dask Array has attributes of a dask Array and redirects the appropriate methods to those defined by Variable, DataArray, and Dataset?
Be similar to test_dask.py?
All, some, or none of the above?

Any suggestions would be greatly appreciated.

jthielen · 2020-07-17T15:04:50Z

@rpmanser For what it's worth, I'd think doing tests like test_dask.py but with a post-hgrecco/pint#1129 Pint Quantity or suitably mocked wrapper class between xarray and Dask would be best. But, I think this is more of a maintainer-level question.

dcherian · 2020-07-17T16:07:20Z

Thanks for working on this @rpmanser.

Since you've changed every isinstance check, if test_dask passes, you're doing fine in general.

Test that a duck dask Array has attributes of a dask Array and redirects the appropriate methods to those defined by Variable, DataArray, and Dataset?

Seems like this is covered by test_dask already but I could be wrong.

We could add specific pint-as-dask tests to test_units with appropriate xfails (they should run on the upstream-dev test config).

This could be a discussion topic for our next dev meeting on wednesday.

rpmanser · 2020-07-20T17:35:31Z

I have erred on the side of leaving the isinstance checks in place where @jthielen reviewed them. If it is decided that no further tests are needed, then this should be ready for review.

On another note, if it is appropriate to apply these changes:

Revisiting it I now realize that is_array_like is actually not a good name for that function: officially, "array_like" means that it can be casted using np.asarray. Maybe we should rename that to is_duck_array? But then we'd also need to check for __array_ufunc__ and __array_function__ because we also need those to properly wrap duck arrays.

As long as that doesn't break any of the current uses, I think that would be the best way forwards. This would require xarray to be on NumPy 1.16+ (in order to ensure hasattr(np.ndarray, "__array_function__"), but that's only 9 days away by NEP 29, so hopefully that isn't too much of an issue. Then, most of the checks for "can this be wrapped by xarray" should be able to become something like
is_duck_array(x) and not isinstance(x, (DataArray, Variable))
with is_duck_dask_array only being needed for checking Dask Array-like support.

I can do that as well. If not, should a new issue be opened for this?

xarray/backends/common.py

xarray/core/duck_array_ops.py

xarray/core/nanops.py

xarray/core/variable.py

xarray/core/duck_array_ops.py

xarray/tests/test_units.py

dcherian

Thanks @rpmanser, this is quite close! Thanks for your efforts here.

* Rename `is_array_like` to `is_duck_array` * `is_duck_array` checks for `__array_function__` and `__array_ufunc__` in addition to previous checks * Replace checks for `is_duck_dask_array` and `__array_function__` with `is_duck_array`

xarray/testing.py

xarray/core/duck_array_ops.py

keewis · 2020-08-27T14:24:14Z

we went with

xarray/xarray/core/variable.py

Line 938 in edd5c1e

or (not IS_NEP18_ACTIVE and isinstance(data, np.ndarray))

in #4379, you could use something like that here:

xarray/xarray/core/formatting.py

Line 554 in 3337b6f

if is_duck_array(a_mapping[k]) or is_duck_array(b_mapping[k]):

too, e.g. with:

def is_duck_array_or_ndarray(array):
    return is_duck_array(array) or (not IS_NEP18_ACTIVE and isinstance(data, np.ndarray))

xarray/core/utils.py

* Add explicit check for NumPy array to is_duck_array * Replace is_duck_array_or_ndarray checks with is_duck_array

rpmanser · 2020-08-28T03:37:36Z

I meant is_duck_array_or_ndarray, not is_duck_dask_array_or_ndarray. I don't suppose there's a way to fix that at this point?

keewis · 2020-08-28T09:30:14Z

you mean the commit message? If so, you can use git commit --amend to change it (make sure you don't have anything staged because that would be added to the commit) and force-push. It's not that important, though, I doubt anyone really reads the commit messages.

dcherian

Thanks @rpmanser

xarray/core/variable.py

Co-authored-by: keewis <keewis@users.noreply.github.com>

keewis

I have two comments, otherwise looks good to me.

xarray/tests/test_units.py

xarray/core/variable.py

keewis · 2020-08-29T23:14:12Z

xarray/core/rolling.py

@@ -389,7 +389,7 @@ def _bottleneck_reduce(self, func, **kwargs):
                valid = (slice(None),) * axis + (slice(-shift, None),)
            padded = padded.pad({self.dim[0]: (0, -shift)}, mode="constant")

-        if isinstance(padded.data, dask_array_type):
+        if is_duck_dask_array(padded.data):
            raise AssertionError("should not be reachable")


@pydata/xarray: why is that here? If it is never reachable, it should be fine to just not do the branching (and if it is necessary, it should be fine to use assert not is_duck_dask_array(padded.data), "should not be reachable").

Also, if we raise the code below will never be reached, so we should be able to remove it.

I think it's from #3040 . We would like to use this code path but there are bugs IIRC

then I guess we should raise a NotImplementedError or a ValueError instead of a AssertionError and improve the error message?

We could comment it out and add a TODO note. The if condition in Line 420 makes sure that this is never reached.

…ssue4208

xarray/tests/test_dask.py

keewis

LGTM. The reason for requires_pint_0_15 would be nice but I don't feel strongly about it so it's up to you.

Thanks for being patient, @rpmanser.

rpmanser · 2020-08-31T21:31:51Z

LGTM. The reason for requires_pint_0_15 would be nice but I don't feel strongly about it so it's up to you.

Thanks for being patient, @rpmanser.

No worries, thanks for the reviews!

dcherian · 2020-09-02T18:28:05Z

Things seem to have died down here so let's test it out.

Thanks @rpmanser This is an amazing first PR. Thanks for your efforts and welcome to xarray!

rpmanser · 2020-09-02T18:32:45Z

@dcherian No problem! Thanks for all your help!

Change isinstance checks to duck Dask Array checks pydata#4208

1d00df3

jthielen reviewed Jul 14, 2020

View reviewed changes

Use is_dask_collection in is_duck_dask_array

1ec28ec

Use is_dask_collection in is_duck_dask_array

817c35d

rpmanser added 2 commits July 17, 2020 11:30

Merge branch 'master' of https://github.com/pydata/xarray into issue4208

03bfaa7

Revert to isinstance checks according to review discussion

bd770e1

dcherian reviewed Jul 23, 2020

View reviewed changes

dcherian mentioned this pull request Jul 23, 2020

Support for duck Dask Arrays #4208

Closed

dcherian reviewed Jul 23, 2020

View reviewed changes

xarray/core/duck_array_ops.py Outdated Show resolved Hide resolved

rpmanser added 3 commits July 27, 2020 15:08

Merge branch 'master' of https://github.com/pydata/xarray into issue4208

299b518

Move is_duck_dask_array to pycompat.py and use tokenize for comparisons

8ca52f6

isort

e405c57

rpmanser changed the title ~~WIP: Change isinstance checks to duck Dask Array checks #4208~~ Change isinstance checks to duck Dask Array checks #4208 Jul 27, 2020

rpmanser marked this pull request as ready for review July 27, 2020 21:25

dcherian reviewed Aug 14, 2020

View reviewed changes

xarray/tests/test_units.py Outdated Show resolved Hide resolved

dcherian reviewed Aug 14, 2020

View reviewed changes

rpmanser added 2 commits August 20, 2020 19:35

Merge branch 'master' of https://github.com/pydata/xarray into issue4208

5b95264

Implement is_duck_array to replace is_array_like

dcffe07

* Rename `is_array_like` to `is_duck_array` * `is_duck_array` checks for `__array_function__` and `__array_ufunc__` in addition to previous checks * Replace checks for `is_duck_dask_array` and `__array_function__` with `is_duck_array`

dcherian reviewed Aug 21, 2020

View reviewed changes

xarray/testing.py Show resolved Hide resolved

dcherian reviewed Aug 21, 2020

View reviewed changes

xarray/core/duck_array_ops.py Show resolved Hide resolved

Skip numpy duck array tests when NEP18 is not active

aacef50

rpmanser added 3 commits August 27, 2020 16:51

Merge branch 'master' of https://github.com/pydata/xarray into issue4208

4bb8ca3

Add is_duck_array_or_ndarray function to utils

f2cbf1e

Use is_duck_array_or_ndarray for duck array checks without NEP18

d6189a9

shoyer reviewed Aug 28, 2020

View reviewed changes

xarray/core/utils.py Outdated Show resolved Hide resolved

Remove is_duck_dask_array_or_ndarray, replace checks with is_duck_array

70fc50c

* Add explicit check for NumPy array to is_duck_array * Replace is_duck_array_or_ndarray checks with is_duck_array

dcherian approved these changes Aug 28, 2020

View reviewed changes

keewis reviewed Aug 28, 2020

View reviewed changes

xarray/core/variable.py Outdated Show resolved Hide resolved

keewis reviewed Aug 28, 2020

View reviewed changes

xarray/core/variable.py Outdated Show resolved Hide resolved

Remove is_duck_array check for deep copy

91c283a

Co-authored-by: keewis <keewis@users.noreply.github.com>

keewis reviewed Aug 29, 2020

View reviewed changes

rpmanser added 4 commits August 31, 2020 10:16

Merge branch 'master' of https://github.com/pydata/xarray into issue4208

3cbcbb2

Use is_duck_array check in load

a51eb66

Move duck dask array tokenize tests from test_units.py to test_dask.py

256d43d

Merge branch 'issue4208' of https://github.com/rpmanser/xarray into i…

80ddaa0

…ssue4208

keewis reviewed Aug 31, 2020

View reviewed changes

xarray/tests/test_dask.py Outdated Show resolved Hide resolved

keewis reviewed Aug 31, 2020

View reviewed changes

xarray/tests/test_dask.py Outdated Show resolved Hide resolved

keewis reviewed Aug 31, 2020

View reviewed changes

xarray/tests/test_dask.py Outdated Show resolved Hide resolved

keewis approved these changes Aug 31, 2020

View reviewed changes

Use _importorskip to require pint >=0.15 instead of pytest.mark.skipif

4e7409c

rpmanser force-pushed the issue4208 branch from 4fcc482 to 4e7409c Compare August 31, 2020 21:27

dcherian merged commit dc2dd89 into pydata:master Sep 2, 2020

This was referenced Sep 15, 2020

deepcopying variable raises TypeError: h5py objects cannot be pickled (Dataset.sortby) #4425

Closed

Fix for h5py deepcopy issues #4426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change isinstance checks to duck Dask Array checks #4208 #4221

Change isinstance checks to duck Dask Array checks #4208 #4221

rpmanser commented Jul 13, 2020 •

edited

Loading

jthielen left a comment

jthielen commented Jul 14, 2020

keewis commented Jul 14, 2020

jthielen commented Jul 14, 2020 •

edited

Loading

pep8speaks commented Jul 14, 2020 •

edited

Loading

rpmanser commented Jul 15, 2020

jthielen commented Jul 17, 2020

dcherian commented Jul 17, 2020

rpmanser commented Jul 20, 2020

dcherian left a comment

keewis commented Aug 27, 2020

rpmanser commented Aug 28, 2020

keewis commented Aug 28, 2020

dcherian left a comment

keewis left a comment

keewis Aug 29, 2020

dcherian Aug 30, 2020

keewis Aug 30, 2020

dcherian Aug 30, 2020

keewis left a comment •

edited

Loading

rpmanser commented Aug 31, 2020

dcherian commented Sep 2, 2020

rpmanser commented Sep 2, 2020

Change isinstance checks to duck Dask Array checks #4208 #4221

Change isinstance checks to duck Dask Array checks #4208 #4221

Conversation

rpmanser commented Jul 13, 2020 • edited Loading

jthielen left a comment

Choose a reason for hiding this comment

jthielen commented Jul 14, 2020

keewis commented Jul 14, 2020

jthielen commented Jul 14, 2020 • edited Loading

pep8speaks commented Jul 14, 2020 • edited Loading

Comment last updated at 2020-08-31 21:28:01 UTC

rpmanser commented Jul 15, 2020

jthielen commented Jul 17, 2020

dcherian commented Jul 17, 2020

rpmanser commented Jul 20, 2020

dcherian left a comment

Choose a reason for hiding this comment

keewis commented Aug 27, 2020

rpmanser commented Aug 28, 2020

keewis commented Aug 28, 2020

dcherian left a comment

Choose a reason for hiding this comment

keewis left a comment

Choose a reason for hiding this comment

keewis Aug 29, 2020

Choose a reason for hiding this comment

dcherian Aug 30, 2020

Choose a reason for hiding this comment

keewis Aug 30, 2020

Choose a reason for hiding this comment

dcherian Aug 30, 2020

Choose a reason for hiding this comment

keewis left a comment • edited Loading

Choose a reason for hiding this comment

rpmanser commented Aug 31, 2020

dcherian commented Sep 2, 2020

rpmanser commented Sep 2, 2020

rpmanser commented Jul 13, 2020 •

edited

Loading

jthielen commented Jul 14, 2020 •

edited

Loading

pep8speaks commented Jul 14, 2020 •

edited

Loading

keewis left a comment •

edited

Loading