Prevent kernel launch with no configuration, remove autotuner #5061
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
The autotuner has been deprecated since Numba 0.40.
Any unconfigured calls were using the default of one thread and one block, so this is made explicit.
This patch prevents the launch of a CUDA kernel with no configuration as this causes confusion for (especially) first time users. Current behaviour is that if no launch config is specified then a default everything-set-to-1 config is used, new behaviour is that if no launch config is specified then an exception is raised pointing users to the syntax and documentation.
The `normalize_kernel_dimensions` function validates that the kernel has been configured, so it must be called in a CUDAKernel call (in addition to an AutoJitCUDAKernel call) to ensure that the kernel has been configured.
stuartarchibald
added
the
Pending BuildFarm
|
Removing [WIP] as it passed the CI tests. |
gmarkall
changed the title
|
running |
|
Something about this PR is causing CUDA to be initialized before the test runner forks to run tests in parallel: |
|
Ah - I'm not in the habit of running CUDA tests with |
|
Looking at the diff, I can see the problem - |
Now that the TestJitErrors class contains test cases that call CUDA functions, it requires the SerialMixin, otherwise it will be executed in a child process after the parent already used CUDA (which is not supported) when testing in parallel.
|
This should be resolved now - I have tested with I believe the test runner always initializes CUDA before the test runner forks to run tests in parallel, as it uses CUDA during test discovery, and this seems to me to be the reason that all the CUDA tests have the |
|
This has passed internal CI. Thanks for the fix! |
seibert
added
BuildFarm Passed
gmarkall
added a commit
to gmarkall/numba
that referenced
this pull request
Jan 13, 2020
This test used CUDA functionality, so it needs the SerialMixin to prevent it running in a child process after the test runner already initialized CUDA in the parent process. It is moved into its own class to add the SerialMixin, to preserve the ability to run other tests from the same class to run in parallel. It also uses a CUDA kernel without a launch configuration, which will soon (pending PR numba#5061) be an error, so we add a launch configuration to it.
|
I'd somehow accidentally committed my Valgrind suppressions file to this branch - have now removed it. |
|
Thanks for implementing this and fixing up my patch @gmarkall. Any chance you could resolve the merge conflicts when you have a few minutes please? Thanks. @seibert this is removing the |
stuartarchibald
added
the
4 - Waiting on author
Out of band @sklam suggested outright removal as-is would be fine, I'm inclined to agree, deprecation notices have been served for a very long time. |
|
I've just merged master into this PR, and just waiting to see what CI does - on my machine with a GPU, all tests pass as expected for the |
stuartarchibald
added
4 - Waiting on CI
stuartarchibald
approved these changes
Feb 27, 2020
|
Farm build ID |
|
Close/Open as CI got stuck due to github API having issues. |
|
Build farm passed. |
stuartarchibald
added
5 - Ready to merge
|
Thanks for doing the merge @gmarkall, merge-in looks good. |
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.
This is based on @stuartarchibald's PR #4468, with additions:
CUDAKernelobjects (those with an explicit list of types to compile) also raise when not configured, as well asAutoJitCUDAKernelobjects.