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

Fix the execution of ANISymmetryFunctions on a non-default CUDA stream #37

Merged
merged 6 commits into from
Nov 22, 2021

Conversation

raimis
Copy link
Contributor

@raimis raimis commented Nov 11, 2021

ANISymmetryFunctions give incorrect result if executed on a non-default stream.

  • Implement CudaANISymmetryFunctions::setStream and CudaANISymmetryFunctions::getStream
  • Set the CUDA stream in NNPOps::ANISymmetryFunctions::Holder
  • Add tests
  • Document

@raimis raimis self-assigned this Nov 11, 2021
@raimis raimis marked this pull request as draft November 11, 2021 15:13
@raimis
Copy link
Contributor Author

raimis commented Nov 11, 2021

This is important for the capture of the CUDA graphs, as it has to be done on a non-default stream (https://pytorch.org/docs/1.10.0/notes/cuda.html#cuda-graphs).

@raimis
Copy link
Contributor Author

raimis commented Nov 11, 2021

I think, I found a solution.

@raimis raimis changed the title ANISymmetryFunctions give incorrect result if executed on a non-default stream Fix the execution of ANISymmetryFunctions on a non-default CUDA stream Nov 11, 2021
@raimis
Copy link
Contributor Author

raimis commented Nov 11, 2021

@peastman Do you have a better suggestion how to implement CudaANISymmetryFunctions::setStream?

@peastman
Copy link
Member

This looks like a reasonable approach. One very minor change I would suggest is not to cache the stream in a field of the object. Instead of

stream = torch::cuda::getCurrentCUDAStream(positions.device().index()).stream();
cudaSymFunc->setStream(stream);

I would write

cudaSymFunc->setStream(torch::cuda::getCurrentCUDAStream(positions.device().index()).stream());

And then the same in backward(). Probably forward() and backward() will always be called with the same stream current, but I don't know if that's guaranteed, so it's better not to assume.

@peastman
Copy link
Member

One other potential issue. By calling getCurrentCUDAStream(positions.device().index()) does that assume the positions are on the GPU? That doesn't need to be the case. CudaANISymmetryFunctions allows the input and output arrays to be on either the CPU or GPU.

@raimis
Copy link
Contributor Author

raimis commented Nov 15, 2021

One other potential issue. By calling getCurrentCUDAStream(positions.device().index()) does that assume the positions are on the GPU? That doesn't need to be the case. CudaANISymmetryFunctions allows the input and output arrays to be on either the CPU or GPU.

It is guaranteed. The tensor is copied to GPU before.

        const Tensor positions = positions_.to(tensorOptions);

@raimis raimis marked this pull request as ready for review November 19, 2021 11:04
@raimis raimis requested a review from peastman November 19, 2021 11:04
@raimis
Copy link
Contributor Author

raimis commented Nov 19, 2021

@peastman this is ready.

Copy link
Member

@peastman peastman left a comment

Choose a reason for hiding this comment

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

Looks good!

@raimis raimis merged commit a696de4 into openmm:master Nov 22, 2021
@raimis raimis mentioned this pull request Jan 21, 2022
4 tasks
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.

2 participants