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

Fixes issue with Numba CUDA target altering max/min semantics. #934

Merged

Conversation

stuartarchibald
Copy link
Contributor

@stuartarchibald stuartarchibald commented Jun 30, 2020

As title.

Closes #932

@stuartarchibald
Copy link
Contributor Author

This is entirely untested and relies on numba/numba#5941, open question about what to do for version Numba 0.50. which has neither the old atomic min/max behaviour nor the new nanmin/nanmax functions.

Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Thanks! I haven't tested it, but hopefully @jonmmease and/or @philippjfr can.

datashader/transfer_functions/_cuda_utils.py Show resolved Hide resolved
def cuda_atomic_nanmax(ary, idx, val)
return cuda.atomic.max(ary, idx, val)
else:
# WHAT DO YOU WANT TO DO HERE?
Copy link
Member

@jbednar jbednar Jul 1, 2020

Choose a reason for hiding this comment

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

Suggested change
# WHAT DO YOU WANT TO DO HERE?
raise ImportError("Datashader's CUDA support requires numba!=0.50.0")

Comment on lines +102 to +103
cuda_atomic_max(data, (i, j), lower)
cuda_atomic_min(data, (i, j), upper)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cuda_atomic_max(data, (i, j), lower)
cuda_atomic_min(data, (i, j), upper)
cuda_atomic_nanmax(data, (i, j), lower)
cuda_atomic_nanmin(data, (i, j), upper)

@stuartarchibald
Copy link
Contributor Author

Thanks for the review @jbednar, once 0.51 is out I'll revisit (or someone else can, I have no particular attachment to the patch, feel free to push to my branch :)) with view to test this and make it actually work opposed to it just being a guess!

@dHannasch
Copy link

dHannasch commented Jul 12, 2020

Thanks! I haven't tested it, but hopefully @jonmmease and/or @philippjfr can.

If anyone's up for testing this, I've fixed the picky little errors at #940. (See stuartarchibald#1.)
(https://github.com/dHannasch/datashader.git@fix/numba_cuda_atomic_minmax_test_with_branch also contains some monkeying with .travis.yml, but if you're going to pip install to test manually, I don't think that will get in your way.)
You can install the prerequisite branch of Numba with pip install git+https://github.com/stuartarchibald/numba.git@fix/cuda_atomic_nanminmax.

Note however that #940 has one more change that might be important. @philippjfr asked why we couldn't just import numba.cuda.atomic.nanmin and numba.cuda.atomic.nanmax directly (that is, why do we need to wrap them in @cuda.jit defs?), and I didn't have an answer, so I tried splitting the difference and using them directly in reductions.py _append_cuda, but using the @cuda.jit wrappers in masked_clip_2d_kernel. So if something goes wonky in _append_cuda but not in masked_clip_2d_kernel, blame me, not @stuartarchibald.
(If it turns out that this is fine, then this can be integrated with the version check as suggested by @philippjfr, so as to work with 0.49 as well.)

Also, if you're going to manually test, best to just test Python 3.7 for now. There are currently unrelated problems with Dask that make the test suite fail on 2.7 and 3.6.

@stuartarchibald
Copy link
Contributor Author

xref numba/numba#6020 which is @gmarkall carrying on with some extra details on top of the original PR on the Numba side.

@AjayThorve
Copy link
Contributor

@dHannasch I tested this with numba 0.51(https://github.com/numba/numba/releases/tag/0.51.0rc1), python 3.7.8.

The version check seems to work as well. Just a couple of fixes required

@philippjfr
Copy link
Member

I'd like to go ahead and merge and then fix things up in a separate PR. Any objections?

@AjayThorve
Copy link
Contributor

I'd like to go ahead and merge and then fix things up in a separate PR. Any objections?

@philippjfr that works

@philippjfr philippjfr merged commit 401e28f into holoviz:master Aug 10, 2020
@AjayThorve AjayThorve mentioned this pull request Aug 10, 2020
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.

Numba <0.49 prevents installation with cuDF>=0.14
5 participants