rmm icon indicating copy to clipboard operation
rmm copied to clipboard

Adding support for cupy.cuda.stream.ExternalStream

Open lilohuang opened this issue 1 year ago • 8 comments

Description

Cupy offers the cupy.cuda.stream.ExternalStream for utilizing external CUDA streams. Moreover, cupy.cuda.get_current_stream() will return an instance of cupy.cuda.stream.ExternalStream instead of cupy.cuda.stream.Stream, particularly when the current cuPy stream has been changed.

Therefore, we must verify both types of instances to avoid errors.

See details in the https://docs.cupy.dev/en/stable/user_guide/interoperability.html#cuda-stream-pointers

Checklist

  • [x] I am familiar with the Contributing Guidelines.
  • [ ] New or existing tests cover these changes.
  • [x] The documentation is up to date with these changes.

lilohuang avatar May 09 '24 05:05 lilohuang

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

copy-pr-bot[bot] avatar May 09 '24 05:05 copy-pr-bot[bot]

/ok to test

harrism avatar May 09 '24 09:05 harrism

Hi @harrism @miscco @jrhemstad,

Would you kindly inform me if there are any additional steps required for this fix to be accepted?

Thank you so much, Lilo

lilohuang avatar May 13 '24 00:05 lilohuang

/ok to test

wence- avatar May 13 '24 09:05 wence-

/ok to test

harrism avatar May 13 '24 21:05 harrism

/ok to test

harrism avatar May 13 '24 21:05 harrism

Can we add a test for this change?

bdice avatar May 13 '24 21:05 bdice

Can we add a test for this change?

Agree, that's a good suggestion. However looking, we don't appear to have any tests of this Stream Cython class.

harrism avatar May 13 '24 21:05 harrism

/ok to test

harrism avatar May 15 '24 02:05 harrism

BTW, I did not mean to negate @bdice's request for tests. Just to say that we don't have existing tests to base this off of. @bdice do you want to block merging until a test is added?

harrism avatar May 15 '24 02:05 harrism

No need to block on tests if existing functionality is untested, but this is a gap that might be worth addressing in the future.

bdice avatar May 15 '24 04:05 bdice

/ok to test

wence- avatar May 16 '24 13:05 wence-

/merge

wence- avatar May 16 '24 13:05 wence-

Thanks @lilohuang!

wence- avatar May 16 '24 16:05 wence-