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: using numba.core.errors.Numba<Error> instead of Error in a Numba typing context. #2659

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

jpivarski
Copy link
Member

This is the only update needed to pass tests with Numba 0.58.0rc1. However, that version of Numba still raises a warning if

export NUMBA_CAPTURED_ERRORS=new_style

is not explicitly set (should probably be changed in Numba itself). Most of the exceptions changed were TypeErrors, but a few were ValueErrors that really ought to have been type errors. (Not finding a record field by name is a type error in compiled code.)

Vector is already up-to-date with respect to TypeError versus numba.TypingError.

@jpivarski jpivarski requested a review from ianna August 18, 2023 18:32
@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #2659 (01e1dd9) into main (01cbec2) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 32.00%.

Additional details and impacted files
Files Changed Coverage Δ
src/awkward/_connect/numba/builder.py 84.95% <15.00%> (+0.03%) ⬆️
src/awkward/_connect/numba/layout.py 83.68% <16.66%> (+0.02%) ⬆️
src/awkward/index.py 89.94% <42.85%> (-2.09%) ⬇️
src/awkward/_connect/numba/arrayview_cuda.py 35.29% <50.00%> (+4.04%) ⬆️
src/awkward/_connect/numba/layoutbuilder.py 87.27% <50.00%> (+0.01%) ⬆️
src/awkward/_connect/numba/arrayview.py 93.37% <100.00%> (+0.01%) ⬆️
src/awkward/operations/ak_from_dlpack.py 62.06% <100.00%> (+6.89%) ⬆️

@jpivarski jpivarski temporarily deployed to docs-preview August 18, 2023 18:42 — with GitHub Actions Inactive
@agoose77
Copy link
Collaborator

agoose77 commented Aug 21, 2023

From the deprecation notice,, it seems to me that we should be using the Numba-aliased errors. I replaced our changes to NumbaTypingError with the appropriate specific error class. Hopefully this works ...

I also therefore am mixing import styles, but I think in time we are moving to from XXX import YYY.

@agoose77 agoose77 changed the title fix: using numba.TypingError for normal/expected errors in a Numba typing context. fix: using numba.core.errors.Numba<Error> instead of Error in a Numba typing context. Aug 21, 2023
@agoose77 agoose77 temporarily deployed to docs-preview August 21, 2023 12:47 — with GitHub Actions Inactive
@jpivarski
Copy link
Member Author

I'm not really sure what the intended difference is between nb.TypingError and nb.NumbaTypeError. They both participate in the new-style of NUMBA_CAPTURED_ERRORS:

>>> issubclass(numba.TypingError, numba.NumbaError)
True
>>> issubclass(numba.NumbaTypeError, numba.NumbaError)
True

Oh, it turns out that nb.NumbaTypeError is just a subclass of nb.TypingError.

>>> numba.TypingError.mro()
[<class 'numba.core.errors.TypingError'>, <class 'numba.core.errors.NumbaError'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>]
>>> numba.NumbaTypeError.mro()
[<class 'numba.core.errors.NumbaTypeError'>, <class 'numba.core.errors.TypingError'>, <class 'numba.core.errors.NumbaError'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>]

I'm on board with from XXX import YYY, and in that style, NumbaTypeError is less ambiguous than TypingError would be. So I think these are fine counter-edits.

@ianna, what do you think?

Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

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

I like it, it is more expressive. Thanks!

@jpivarski
Copy link
Member Author

Okay, then I'll merge!

@jpivarski jpivarski enabled auto-merge (squash) August 21, 2023 16:14
@jpivarski jpivarski temporarily deployed to docs-preview August 21, 2023 16:24 — with GitHub Actions Inactive
@jpivarski jpivarski merged commit a42e44c into main Aug 21, 2023
33 checks passed
@jpivarski jpivarski deleted the jpivarski/adapt-to-numba-0.58rc1 branch August 21, 2023 16:27
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