-
Notifications
You must be signed in to change notification settings - Fork 12
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
Move compilation of CUDA code to NVRTC #131
Conversation
Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping |
this should be fixed now. |
static CacheMapCUDA<C, T> sph_cache; | ||
static std::mutex cache_mutex; | ||
|
||
// Check if instance exists in cache, if not create and store it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frostedoyster should we purge the instances after some point? Or do we let this map grow unbounded? (This would be for a separate PR!)
# Install this one manually. Listing it in the deps list above does not install jaxlib. | ||
# Note: jax[cuda12] is not available on Windows and MacOS. | ||
bash -c 'command -v nvcc && python -m pip install -U "jax[cuda12]" -f https://storage.googleapis.com/jax-releases/jax_cuda_releases.html || python -m pip install -U jax[cpu]' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the CUDA version of JAX to run the examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the immediate downside and it does allow for catching more bugs on the CSCS CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, it won't run the cuda code through jax, and we woul have indeed missed the bugs from last week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there is a separate test section above which is already using cuda, this is for the examples. If there is stuff in there that acts like a test, we should maybe move it to the tests instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is fine for now, we can revisit later if this becomes a problem.
Co-authored-by: Guillaume Fraux <[email protected]>
Co-authored-by: Guillaume Fraux <[email protected]>
Co-authored-by: Guillaume Fraux <[email protected]>
sphericart/src/sphericart_cuda.cpp
Outdated
"Failed to load libcuda.so. Try running \"find /usr -name libcuda.so\" and " | ||
"appending the directory to your $LD_LIBRARY_PATH environment variable." | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, nice that this is now the only place with an error mesage! Could it be updated to not reference /usr
since libcuda
can be elsewhere? This was already done on the other messages but these fell through!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated it, do you agree with the new message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, looks a lot better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your work @nickjbrowning!
No description provided.