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

CUDA: Remove catch of NotImplementedError in target.py #6997

Merged
merged 2 commits into from May 19, 2021

Conversation

gmarkall
Copy link
Member

@gmarkall gmarkall commented May 4, 2021

Maybe this is more of a question than a PR, but I saw it added in #6948 and I'm not sure what it does. Removing it seems not to cause a problem - all tests still seem to pass without it.

Not clear why this is required - all tests pass without it.
@gmarkall gmarkall added 3 - Ready for Review CUDA CUDA related issue/PR labels May 4, 2021
@gmarkall
Copy link
Member Author

gmarkall commented May 4, 2021

From OOB discussion, it seems like this might have initially been added due to #701, which is no longer relevant because NumPy 1.6 is no longer supported, and it also appears that there is no way to trigger the NotImplementedError at import time. The PR has been updated to remove the try from the CPU target's import of numba.np.npdatetime as well.

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. I've tried a few ways to trigger the noted exception and it seems like something else is always an issue before a NotImplementedError is hit. Further, it appears on inspection that the exception cannot be raised at import time as the control flow required to do so is always in a closure held by a lowering implementation and not actually executed as part of an import sequence.

This patch should get a CPU and CUDA smoke test and assuming both come back fine I think that would further confirm its validity.

Thanks again!

@stuartarchibald stuartarchibald added 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 and removed 3 - Ready for Review labels May 5, 2021
@stuartarchibald stuartarchibald added this to the Numba 0.54 RC milestone May 5, 2021
@stuartarchibald
Copy link
Contributor

Waiting on merge of #7003

@stuartarchibald
Copy link
Contributor

Buildfarm ID: numba_smoketest_cpu_yaml_30.
Buildfarm ID: numba_smoketest_cuda_yaml_62.

@stuartarchibald
Copy link
Contributor

Buildfarm ID: numba_smoketest_cuda_yaml_62.

Passed.

@stuartarchibald
Copy link
Contributor

Buildfarm ID: numba_smoketest_cpu_yaml_30.

Passed

@stuartarchibald stuartarchibald added BuildFarm Passed For PRs that have been through the buildfarm and passed 5 - Ready to merge Review and testing done, is ready to merge 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 May 19, 2021
@sklam sklam merged commit 0c33c33 into numba:master May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants