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

New Collections protocol breaks xarray objects #9058

Closed
dcherian opened this issue May 9, 2022 · 3 comments · Fixed by #9062
Closed

New Collections protocol breaks xarray objects #9058

dcherian opened this issue May 9, 2022 · 3 comments · Fixed by #9062
Labels
bug Something is broken core p1 Affects a large population and inhibits work

Comments

@dcherian
Copy link
Contributor

dcherian commented May 9, 2022

The recent merge of the new Collections protocol (#8674) broke Xarray's tests (pydata/xarray#6578)

Xarray objects return None from __dask_graph__ if there no dask arrays being wrapped.

It seems like the current version of is_dask_collection only checks for the presence of __dask_graph__. Xarray objects are a case where this check is not sufficient (they look like dask collections in general, you can call dask.visualize and dask.compute on xarray objects).

It isn't clear to me that there is a good solution other than explicitly checking the return value in is_dask_collection but if there is something we could do on the Xarray side, please let us know!

@github-actions github-actions bot added the needs triage Needs a response from a contributor label May 9, 2022
@jrbourbeau
Copy link
Member

Thanks for reporting @dcherian (and hooray for upstream CI testing 🎉 ). I've not had a chance to dig into this yet, but plan to do so tomorrow. cc'ing @douglasdavis @ian-r-rose in the meantime

@jrbourbeau jrbourbeau added core bug Something is broken p1 Affects a large population and inhibits work and removed needs triage Needs a response from a contributor labels May 10, 2022
@ian-r-rose
Copy link
Collaborator

Thanks for flagging this @dcherian! We had some discussion about exactly this change here, and were not aware of any downstream usages of returning None. At the time it seemed relatively safe since there are many places in dask's codebase that don't check for None and happily assume the result is a graph. But evidently not.

It's simple enough fix to roll back that change, though after we do it it might be nice if we could continue discussing how to tighten the protocol a bit.

@douglasdavis
Copy link
Member

I agree with @ian-r-rose. I just created a PR (#9062) to revert the change to is_dask_collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken core p1 Affects a large population and inhibits work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants