Don't attempt to register overloads that aren't for this target in BaseContext and related fixes
#9454
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Plan following discussion with @stuartarchibald Wait for #9447 to be merged, then rebase and tidy this up to be ready for review, in the 0.60 milestone. |
…rent target This moves the target check from inside the `_OverloadMethodTemplate` to the `BaseContext.install_registry()` method. The effect of this change is that instead of attempting to register all overloaded method templates with all targets and then silently failing inside the registration, we only attempt to register overloaded methods that are appropriate for the target. The `_OverloadAttributeTemplate` never contained this check, which appears curious at first sight - however, there is a related bug in overloading of attributes that causes their metadata to be lost, and therefore all attributes get registered for all targets regardless of whether they are for the current target or not. This error will be addressed in the following commit.
Decorator keywords arguments other than `inline` were dropped, so other metadata such as the target were silently lost. We resolve this by passing all keyword arguments to `make_overload_attribute_template()`. Also add a test that verifies that the metadata still exists, which prevents a CUDA-only attribute overload being used on the CPU target, with a sensible TypingError as the result.
This is analagous to the similar test added for CUDA in the previous commit, but doesn't require CUDA to run so can be a part of the main test suite.
The DPU target should have been setting a target override during compilation (similar to how the CUDA target does), and it should also set the `target_backend` target option, mirroring what the default `@jit` decorator does. Not setting these doesn't seem to have had any ill effect within the testsuite, but these settings should both be made to avoid running into latent issues in future.
gmarkall
force-pushed
the
target-mismatch-refactor
branch
from
b5809ce
to
1a0b4d6
Compare
When typing contexts are refreshed, there may be registrations for invalid targets pending (e.g. from other tests that exercise error handling in the presence of invalid user code). These registrations need to be ignored - see the comments in the code for further detail. Attempting to compile a call to an overload with an invalid target now causes a `NonexistentTargetError` to be raised, so the target extension tests are updated accordingly.
gmarkall
force-pushed
the
target-mismatch-refactor
branch
from
1a0b4d6
to
a6fd2d0
Compare
|
gpuci run tests |
gmarkall
changed the title
BaseContextBaseContext and related fixes
There was a problem hiding this comment.
Thanks for fixing this @gmarkall. As we have debugged, discussed and worked on this logic through many OOB conversations I'm reasonably convinced that this fixes the issues with the use of target= in @overload_method etc and also a number of additional issues discovered in the debugging process. There's a few minor things to address, otherwise looks good.
stuartarchibald
added
4 - Waiting on author
40 tasks
This option is going to be removed by numba#9539 when it is merged.
Instead of reusing `MyDummyType` and `mydummy_type` from `test_extending`, we use the `make_dummy_type` method of the test classes to create types that are unique to the current test. A minor (inconsequential) error in the signatures in the tests is also fixed.
|
gpuci run tests |
gmarkall
added
4 - Waiting on reviewer
stuartarchibald
approved these changes
Apr 26, 2024
stuartarchibald
added
5 - Ready to merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a cascading plethora of issues discovered as a result of looking into #9432. See individual commit messages for details of fixes - the pattern is generally that each fix exposed a new issue, resolved by a subsequent commit.