Skip to content

Fix #6121 - #6123

Merged
sklam merged 5 commits into
numba:masterfrom
gmarkall:grm-issue-6121
Aug 12, 2020
Merged

Fix #6121#6123
sklam merged 5 commits into
numba:masterfrom
gmarkall:grm-issue-6121

Conversation

@gmarkall

@gmarkall gmarkall commented Aug 12, 2020

Copy link
Copy Markdown
Member

Opening this PR to fix #6121 and iterate on it. The problem is that forall always returns a new Dispatcher, which immediately compiles the specialization. Prior to #5709 being merged, the cache of compilations was used and any previously-compiled kernel could be returned by specialize. The fix in this branch introduces a cache of specialized Dispatcher objects to work around the issue.

stuartarchibald added a commit to stuartarchibald/numba that referenced this pull request Aug 12, 2020
@stuartarchibald
Update CHANGE_LOG for numba#6123
504c355 
As title.
@stuartarchibald stuartarchibald added this to the Numba 0.51.0 milestone Aug 12, 2020
@stuartarchibald stuartarchibald added 2 - In Progress Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm CUDA CUDA related issue/PR labels Aug 12, 2020
@gmarkall
CUDA: Don't allow re-specializing Dispatchers
0052467 
This was never possible before numba#5709 - the `CUDAKernel` class that was
returned by specializing had no `specialize()` method to call.

Before numba#5709 `ForAll.__call__()` avoided re-specialization - we also
need to avoid re-specialization again so that is added back in.

Addendum: corrected an inaccurate docstring spotted whilst looking at
the code.
@sklam sklam mentioned this pull request Aug 12, 2020
Closed
42 tasks done
@stuartarchibald

stuartarchibald commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

Buildfarm ID: numba_smoketest_cuda_92 for 86241da

@gmarkall
CUDA: More dispatcher testing
a7a21df 
Add tests that check you can't specialize for a different types as well
as for the same types as the Dispatcher is already specialized for.
@gmarkall
CUDA: Add Dispatcher specialization caching tests
4f17d85 
This test takes array ordering into account, and uses multiple
arguments.

Also, the argument types in other tests should have been passed as
arguments to specialize(), not as tuples of argument types.
@stuartarchibald

stuartarchibald commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

numba_smoketest_cuda_92 passed, will try 4f17d85

@stuartarchibald

stuartarchibald commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

Buildfarm ID: numba_smoketest_cuda_93 for 4f17d85

@gmarkall gmarkall changed the title [WIP] Fix #6121 Fix #6121 Aug 12, 2020
stuartarchibald
stuartarchibald approved these changes Aug 12, 2020
View reviewed changes

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch. Fix and tests look good. The main test here is the compile time response from upstream (e.g. cuDF) and whether the build farm is happy. Waiting for confirmation by @gmarkall on the upstream testing.

@stuartarchibald stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 3 - Ready for Review labels Aug 12, 2020
@stuartarchibald

stuartarchibald commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

numba_smoketest_cuda_93 passed.

@gmarkall

gmarkall commented Aug 12, 2020

Copy link
Copy Markdown
Member Author

@stuartarchibald As far as I can tell from testing cuDF and other libraries, the issue appears to be resolved and I can't see that this introduces any new issues.

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed and removed 4 - Waiting on CI Review etc done, waiting for CI to finish Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm labels Aug 12, 2020
@stuartarchibald

stuartarchibald commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

@stuartarchibald As far as I can tell from testing cuDF and other libraries, the issue appears to be resolved and I can't see that this introduces any new issues.

Thanks for confirming. This will make 0.51.0.

@sklam
sklam merged commit d129fe9 into numba:master Aug 12, 2020
sklam added a commit that referenced this pull request Aug 12, 2020
@sklam
Merge pull request #6124 from stuartarchibald/wip/CHANGELOG_051_update2
b9efc83 
Update CHANGE_LOG for #6123
sklam added a commit that referenced this pull request Aug 12, 2020
@sklam
Merge pull request #6123 from gmarkall/grm-issue-6121
29f579b 
Fix #6121
sklam added a commit that referenced this pull request Aug 12, 2020
@sklam
Merge pull request #6124 from stuartarchibald/wip/CHANGELOG_051_update2
3a14664 
Update CHANGE_LOG for #6123
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartarchibald stuartarchibald stuartarchibald approved these changes

Assignees

No one assigned

Labels

5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed CUDA CUDA related issue/PR

Projects

None yet

Milestone

Numba 0.51.0

Development

Successfully merging this pull request may close these issues.

CUDA: Recent commit causes infinite loop (or a lot of recompiling)

3 participants

@gmarkall @stuartarchibald @sklam