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

refactor: implement Backend registry (2 of 2) #2390

Merged
merged 6 commits into from
Apr 12, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Apr 11, 2023

TL;DR

Warning
Depends upon #2389

#2389 implements a cached lookup for nplikes. This PR implements a similar mechanism for backends. Unlike #2389, this PR implements a lookup registry; types can register a factory function that accepts a type, and returns a finder. This is used to locate backends for e.g. Content.

TL

This PR intends to clarify which objects support Backend resolution, and to improve the performance of the search. Although the existing mechanism is trivial, it is called frequently throughout the codebase, so it is important to keep it simple.
The existing implementation attempts to duck-type our way to find the backends. However, in practice we are constrained in what kind of interface we can define, such that this approach is not scalable.

This PR replaces kludgy logic with an explicit lookup mechanism that can be specialised per type.

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

This is a good internal refactoring, but as a question that must be answered before merging: is this going to affect the Coffea and dask-awkward updates?

Coffea might be using L3 nplike/backend resolution as a stepping stone during development (it has to eventually all be L2, but can be L3 while figuring out what needs to be done, independent of how to interface with it properly).

With this PR, do the Coffea tests still work?

@agoose77 agoose77 force-pushed the agoose77/refactor-nplike-of branch from fb42f0c to ac56c37 Compare April 11, 2023 20:56
@agoose77 agoose77 force-pushed the agoose77/refactor-nplike-of branch from ff4e999 to a62c587 Compare April 11, 2023 20:59
@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #2390 (a62c587) into main (eccc234) will increase coverage by 0.10%.
The diff coverage is 92.83%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_connect/rdataframe/from_rdataframe.py 0.00% <0.00%> (ø)
src/awkward/_connect/numba/arrayview_cuda.py 31.25% <50.00%> (+4.58%) ⬆️
src/awkward/operations/ak_to_buffers.py 92.30% <50.00%> (+0.64%) ⬆️
src/awkward/operations/ak_to_cupy.py 45.45% <50.00%> (+5.45%) ⬆️
src/awkward/_backends/cupy.py 66.66% <66.66%> (ø)
src/awkward/highlevel.py 75.54% <66.66%> (-0.26%) ⬇️
src/awkward/contents/content.py 75.12% <86.66%> (+0.33%) ⬆️
src/awkward/_backends/backend.py 89.28% <89.28%> (ø)
src/awkward/_backends/dispatch.py 95.38% <95.38%> (ø)
src/awkward/_backends/jax.py 97.36% <97.36%> (ø)
... and 48 more

@agoose77 agoose77 temporarily deployed to docs-preview April 11, 2023 21:15 — with GitHub Actions Inactive
@agoose77
Copy link
Collaborator Author

agoose77 commented Apr 11, 2023

With this PR, do the Coffea tests still work?

For my local install, coffea tests fail on main (both Awkward & Coffea), and if I downgrade awkward to v2.1.2, they still fail. The number of failures does not change if I bump awkward to this PR. Therefore, I'm comfortable merging it with the assumption that I'm not making things worse.

I'll ping @lgray to figure out whether I should be testing a different branch to main.

@lgray
Copy link
Contributor

lgray commented Apr 11, 2023

@agoose77 let me check - there are some xfails right now because of some strange interactions with dask that are not your fault which I have not marked.

@lgray
Copy link
Contributor

lgray commented Apr 12, 2023

@agoose77 coffea main should now pass tests if everything is ok. :-)

@agoose77
Copy link
Collaborator Author

I ran coffea's test suite (and dask-awkward) on this branch, and saw no regressions :) Merging!

@agoose77 agoose77 merged commit 83df6e5 into main Apr 12, 2023
@agoose77 agoose77 deleted the agoose77/refactor-nplike-of branch April 12, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants