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

fix: backends should have defaults for user-facing operations #1940

Merged
merged 6 commits into from
Dec 2, 2022

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Dec 2, 2022

The safe default for ak._backends.backend_of is to fail if the inputs don't have a well defined backend e.g. for lists. However, for user-facing operations, this is not correct; we want to assume NumPy for these types. A grep shows that only a few operations need this fix.

I haven't added any tests because we should really do this as part of a coverage pass for all of our operations.


📚 The documentation for this PR will be available at https://awkward-array.readthedocs.io/en/agoose77-fix-backend-default/ once Read the Docs has finished building 🔨

@agoose77 agoose77 marked this pull request as ready for review December 2, 2022 21:04
@agoose77 agoose77 requested a review from jpivarski December 2, 2022 21:06
@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Merging #1940 (6a6eb9b) into main (31ce657) will increase coverage by 0.02%.
The diff coverage is 96.29%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/operations/ak_to_backend.py 100.00% <ø> (ø)
src/awkward/_backends.py 83.33% <85.71%> (+0.76%) ⬆️
src/awkward/operations/ak_backend.py 100.00% <100.00%> (+19.23%) ⬆️
src/awkward/operations/ak_cartesian.py 89.76% <100.00%> (+0.08%) ⬆️
src/awkward/operations/ak_concatenate.py 96.07% <100.00%> (+0.03%) ⬆️
src/awkward/operations/ak_fill_none.py 95.12% <100.00%> (+0.12%) ⬆️
src/awkward/operations/ak_run_lengths.py 90.90% <100.00%> (+0.13%) ⬆️
src/awkward/operations/ak_transform.py 83.05% <100.00%> (+0.29%) ⬆️
src/awkward/operations/ak_where.py 91.83% <100.00%> (+0.17%) ⬆️
src/awkward/highlevel.py 75.50% <0.00%> (+0.33%) ⬆️

@agoose77 agoose77 marked this pull request as draft December 2, 2022 21:13
@agoose77
Copy link
Collaborator Author

agoose77 commented Dec 2, 2022

@jpivarski would you be happy for me to change ak.backend to throw an exception if the input arrays don't have a common backend? Right now it returns "mixed", which is not a valid backend name, and has less utility now that single arrays cannot be mixed.

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.

So backend_of would normally raise an exception if it can't infer a backend without an explicit default=cpu?

nplike = nplike_of(*objects, default=None)
if nplike is not None:
return _backend_for_nplike(nplike)
elif default is _UNSET:
raise ak._errors.wrap_error(ValueError("could not find backend for", objects))
else:
return default

Okay, yeah, it does. So you need to provide default=cpu for any user-facing functions.

Looks good; go ahead and merge it!

@agoose77
Copy link
Collaborator Author

agoose77 commented Dec 2, 2022

Yes. My call was that we really care about the backend, and due to interop it's very hard to tell if we get it wrong (JAX can adopt NumPy, CuPy can adopt NumPy). Better to fail loudly.

@agoose77
Copy link
Collaborator Author

agoose77 commented Dec 2, 2022

@jpivarski would you mind confirming on #1940 (comment) ? I know you're busy with lots of things at the moment, but don't want to merge this without an OK!

@jpivarski
Copy link
Member

Yeah, "mixed" no longer means anything and shouldn't be returned by anything. I'm in favor of doing things this way, though we've got to be careful to always provide a default (and it would always be cpu) in user-facing functions now.

It is better to fail loudly.

I hadn't noticed that this is a draft PR. If it's a draft because you still have a lot of user-facing functions to update, you don't need re-approval to merge it. I'm in favor of the principle.

@agoose77
Copy link
Collaborator Author

agoose77 commented Dec 2, 2022

Thanks! I'm going to make two changes; removing "mixed" from our high level API, and to associate the backend name on the backend object so this code becomes more trivial.

@jpivarski
Copy link
Member

Raising an error in mixed cases is right. We should not have any array with different backends in different nodes.

@agoose77 agoose77 marked this pull request as ready for review December 2, 2022 21:42
@agoose77
Copy link
Collaborator Author

agoose77 commented Dec 2, 2022

OK, this is done. I've added a check to ensure that we throw a ValueError if users somehow encounter a typetracer

@agoose77 agoose77 added the pr-next-release Required for the next release label Dec 2, 2022
@agoose77 agoose77 merged commit 16d482a into main Dec 2, 2022
@agoose77 agoose77 deleted the agoose77/fix-backend-default branch December 2, 2022 22:52
@jpivarski jpivarski removed the pr-next-release Required for the next release label Feb 15, 2023
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.

2 participants