Change isinstance checks to duck Dask Array checks #4208 by rpmanser · Pull Request #4221 · pydata/xarray

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

ClosesSupport 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.

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__") orisinstance(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) andnotisinstance(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) andnotisinstance(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?

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`

dcherian

I took a second pass and found one issue.

LGTM otherwise.

shoyer · 2020-08-27T04:37:29Z

yes, the failing test is definitely due to NEP18: there's a numpy array in an attribute, but since numpy 1.15 doesn't define __array_function__, is_duck_array returns False. We could extend is_duck_array with an isinstance check for numpy.ndarray, or we could take your suggestion and xfail the test. Not sure which is better.

I would suggest adding an explicit check for NumPy arrays. Otherwise we would need to bump our minimum supported NumPy version.

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

we went with

xarray/xarray/core/variable.py

Line 938 in edd5c1e

or (notIS_NEP18_ACTIVEandisinstance(data, np.ndarray))

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

xarray/xarray/core/formatting.py

Line 554 in 3337b6f

ifis_duck_array(a_mapping[k]) oris_duck_array(b_mapping[k]):

too, e.g. with:

defis_duck_array_or_ndarray(array):
returnis_duck_array(array) or (notIS_NEP18_ACTIVEandisinstance(data, np.ndarray))

* 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

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

keewis

I have two comments, otherwise looks good to me.

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


- 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

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 checkspydata#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

Russell Manser added 2 commits July 17, 2020 11:30

Merge branch 'master' ofhttps://github.com/pydata/xarrayinto issue4208
03bfaa7

Revert to isinstance checks according to review discussion
bd770e1

dcherian reviewed Jul 23, 2020
View reviewed changes

Comment threadxarray/backends/common.py
Comment threadxarray/backends/common.py Outdated
Comment threadxarray/core/duck_array_ops.py Outdated
Comment threadxarray/core/duck_array_ops.py
Comment threadxarray/core/nanops.py
Comment threadxarray/core/variable.py

dcherian mentioned this pull request Jul 23, 2020
Support for duck Dask Arrays #4208
Closed

dcherian reviewed Jul 23, 2020
View reviewed changes

Comment threadxarray/core/duck_array_ops.py Outdated

Russell Manser added 3 commits July 27, 2020 15:08

Merge branch 'master' ofhttps://github.com/pydata/xarrayinto 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 #4208Jul 27, 2020

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

dcherian reviewed Aug 14, 2020
View reviewed changes

Comment threadxarray/tests/test_units.py Outdated

dcherian reviewed Aug 14, 2020
View reviewed changes

Russell Manser added 2 commits August 20, 2020 19:35

Merge branch 'master' ofhttps://github.com/pydata/xarrayinto 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

Comment threadxarray/testing.py

dcherian reviewed Aug 21, 2020
View reviewed changes

Comment threadxarray/core/duck_array_ops.py

Skip numpy duck array tests when NEP18 is not active
aacef50

dcherian approved these changes Aug 23, 2020
View reviewed changes

Comment threadxarray/core/variable.py Outdated

Russell Manser added 3 commits August 27, 2020 16:51

Merge branch 'master' ofhttps://github.com/pydata/xarrayinto 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

Comment threadxarray/core/utils.py Outdated

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

Comment threadxarray/core/variable.py

keewis reviewed Aug 28, 2020
View reviewed changes

Comment threadxarray/core/variable.py Outdated

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

Russell Manser added 4 commits August 31, 2020 10:16

Merge branch 'master' ofhttps://github.com/pydata/xarrayinto 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' ofhttps://github.com/rpmanser/xarrayinto i…
80ddaa0
…ssue4208

keewis reviewed Aug 31, 2020
View reviewed changes

Comment threadxarray/tests/test_dask.py Outdated

keewis reviewed Aug 31, 2020
View reviewed changes

Comment threadxarray/tests/test_dask.py Outdated

keewis reviewed Aug 31, 2020
View reviewed changes

Comment threadxarray/tests/test_dask.py Outdated

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 4e7409cCompare August 31, 2020 21:27

dcherian merged commit dc2dd89 into pydata:masterSep 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

Uh oh!

Uh oh!

Conversation

rpmanser commented Jul 13, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jthielen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jthielen commented Jul 14, 2020

Uh oh!

keewis commented Jul 14, 2020

Uh oh!

jthielen commented Jul 14, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Jul 14, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

rpmanser commented Jul 15, 2020

Uh oh!

jthielen commented Jul 17, 2020

Uh oh!

dcherian commented Jul 17, 2020

Uh oh!

rpmanser commented Jul 20, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcherian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dcherian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shoyer commented Aug 27, 2020

Uh oh!

keewis commented Aug 27, 2020

Uh oh!

Uh oh!

rpmanser commented Aug 28, 2020

Uh oh!

keewis commented Aug 28, 2020

Uh oh!

dcherian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keewisAug 29, 2020

Choose a reason for hiding this comment

Uh oh!

dcherianAug 30, 2020

Choose a reason for hiding this comment

Uh oh!

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