Skip to content

Consolidate array Dispatch objects#7505

Merged
jrbourbeau merged 3 commits intodask:mainfrom
jrbourbeau:array-dispatch
May 24, 2021
Merged

Consolidate array Dispatch objects#7505
jrbourbeau merged 3 commits intodask:mainfrom
jrbourbeau:array-dispatch

Conversation

@jrbourbeau
Copy link
Member

This PR consolidates Dispatch objects used throughout dask.array, and their corresponding registrations, into dask/array/backends.py. This goes along with #7503, which does the same thing for dask.dataframe.

xref #7502

cc @jsignell @jakirkham @kkraus14

@jrbourbeau jrbourbeau changed the title Move array dispatch functions to a separate module Consolidate array Dispatch objects Apr 1, 2021
@jakirkham
Copy link
Member

Should we extend codeowners here as well?

cc @pentschev @quasiben (for awareness)

@jsignell
Copy link
Member

jsignell commented Apr 1, 2021

Should we extend codeowners here as well?

I think I have a lableling solution that will actually work better given the restriction described here: #7503 (comment)

Copy link
Member

@jsignell jsignell left a comment

Choose a reason for hiding this comment

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

I think I prefer to have the dispatch objects all in one file separate form any of the registrations and with no imports. That way the registrations can import them all from dispatch and then the registrations don't get imported anywhere, so it makes for clean dependencies. And we can safely import from dispatch in all the places where we explicitly use the dispatch objects.

@jsignell
Copy link
Member

jsignell commented Apr 1, 2021

#7506

@jsignell
Copy link
Member

jsignell commented Apr 1, 2021

We should hold off merging either of these until we get a chance to talk things through thoroughly with the dask-cudf folks.

@jrbourbeau
Copy link
Member Author

Yep, agreed let's hold off for right now. FWIW I think the changes here are backwards compatible.

@pentschev
Copy link
Member

Looks fine to me, also ran CuPy tests locally and they pass as well!

@jakirkham
Copy link
Member

Just noting this is still of interest, know everyone is busy with the Summit this week, but it would be good to figure out next steps here (maybe next week?). In particular Julia mentioned this PR might need some updating ( #7503 (comment) ).

@jsignell
Copy link
Member

Yeah good ping @jakirkham. The remaining work is just to make the file structure in this PR match the other one, so separate out backends and dispatch into their own files.

@github-actions github-actions bot added the array label May 20, 2021
@jrbourbeau
Copy link
Member Author

Yeah, thanks for the ping @jakirkham. I just pushed a commit which updates the file structure here to match what @jsignell did over in #7503

@jakirkham jakirkham requested a review from jsignell May 24, 2021 17:47
Copy link
Member

@jsignell jsignell left a comment

Choose a reason for hiding this comment

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

Looks good!

@jrbourbeau
Copy link
Member Author

Thanks for testing @pentschev and reviewing @jakirkham @jsignell

@jrbourbeau jrbourbeau merged commit 4799c9d into dask:main May 24, 2021
@jrbourbeau jrbourbeau deleted the array-dispatch branch May 24, 2021 22:05
rapids-bot bot pushed a commit to rapidsai/dask-cuda that referenced this pull request May 26, 2021
Requires PR ( rapidsai/cudf#8342 )

As these functions recently got relocated, handle `import`ing from the new location with a fallback to the old location.

xref: dask/dask#7503
xref: dask/dask#7505

Authors:
  - https://github.com/jakirkham

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Benjamin Zaitlen (https://github.com/quasiben)

URL: #623
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants