rapidsai / rmm

RAPIDS Memory Manager
https://docs.rapids.ai/api/rmm/stable/
Apache License 2.0
478 stars 195 forks source link

Adding support for cupy.cuda.stream.ExternalStream #1559

Closed lilohuang closed 4 months ago

lilohuang commented 4 months ago

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

copy-pr-bot[bot] commented 4 months ago

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.

harrism commented 4 months ago

/ok to test

lilohuang commented 4 months ago

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

wence- commented 4 months ago

/ok to test

harrism commented 4 months ago

/ok to test

harrism commented 4 months ago

/ok to test

bdice commented 4 months ago

Can we add a test for this change?

harrism commented 4 months ago

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 commented 4 months ago

/ok to test

harrism commented 4 months ago

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?

bdice commented 4 months ago

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

wence- commented 4 months ago

/ok to test

wence- commented 4 months ago

/merge

wence- commented 4 months ago

Thanks @lilohuang!