cuda-python icon indicating copy to clipboard operation
cuda-python copied to clipboard

Fix/default stream singletons

Open Monishver11 opened this issue 1 month ago • 8 comments

Description

closes #1494

This PR ensures that Stream.legacy_default() and Stream.per_thread_default() return singleton instances when called from the base Stream class.

Previously, these methods were returning new Python instances on every call, which deviated from the intended design of exposing default streams as singletons to avoid unnecessary object creation and ensure identity consistency (e.g., s1 is s2). This aligns the implementation with the intended behavior discussed in #1483

Changes

  • Implemented logic in legacy_default() and per_thread_default() to return cached cdef stream objects.
  • Initialized the internal singletons using the internal _from_handle factory to prevent circular reference issues during Cython module initialization.
  • Linked the public-facing module constants LEGACY_DEFAULT_STREAM and PER_THREAD_DEFAULT_STREAM to these same singleton instances.
  • Maintained the ability for subclasses of Stream to receive unique instances of the subclass type, ensuring that specialized stream types are not forced into the base singleton.

Testing

(cuda-python-dev) [mc10322@cuda5 cuda_core]$ pytest tests/test_stream.py -v
============================================== test session starts ==============================================
platform linux -- Python 3.10.19, pytest-9.0.2, pluggy-1.6.0 -- /scratch/mc10322/miniconda3/envs/cuda-python-dev/bin/python3.10
cachedir: .pytest_cache
rootdir: /scratch/mc10322/cuda-python/cuda_core
configfile: pytest.ini
collected 33 items

tests/test_stream.py::test_stream_init_disabled PASSED                                                    [  3%]
tests/test_stream.py::test_stream_init_with_options PASSED                                                [  6%]
tests/test_stream.py::test_stream_handle PASSED                                                           [  9%]
tests/test_stream.py::test_stream_is_nonblocking PASSED                                                   [ 12%]
tests/test_stream.py::test_stream_priority PASSED                                                         [ 15%]
tests/test_stream.py::test_stream_sync PASSED                                                             [ 18%]
tests/test_stream.py::test_stream_record PASSED                                                           [ 21%]
tests/test_stream.py::test_stream_wait_event PASSED                                                       [ 24%]
tests/test_stream.py::test_stream_wait_invalid_event PASSED                                               [ 27%]
tests/test_stream.py::test_stream_device PASSED                                                           [ 30%]
tests/test_stream.py::test_stream_context PASSED                                                          [ 33%]
tests/test_stream.py::test_stream_from_foreign_stream PASSED                                              [ 36%]
tests/test_stream.py::test_stream_from_handle PASSED                                                      [ 39%]
tests/test_stream.py::test_legacy_default_stream PASSED                                                   [ 42%]
tests/test_stream.py::test_per_thread_default_stream PASSED                                               [ 45%]
tests/test_stream.py::test_stream_subclassing PASSED                                                      [ 48%]
tests/test_stream.py::test_stream_legacy_default_subclassing PASSED                                       [ 51%]
tests/test_stream.py::test_stream_per_thread_default_subclassing PASSED                                   [ 54%]
tests/test_stream.py::test_stream_legacy_default_public_api PASSED                                        [ 57%]
tests/test_stream.py::test_stream_per_thread_default_public_api PASSED                                    [ 60%]
tests/test_stream.py::test_stream_equality_same_handle PASSED                                             [ 63%]
tests/test_stream.py::test_stream_inequality_different_handles PASSED                                     [ 66%]
tests/test_stream.py::test_stream_equality_reflexive PASSED                                               [ 69%]
tests/test_stream.py::test_stream_equality_symmetric PASSED                                               [ 72%]
tests/test_stream.py::test_stream_type_safety PASSED                                                      [ 75%]
tests/test_stream.py::test_stream_not_equal_operator PASSED                                               [ 78%]
tests/test_stream.py::test_stream_hash_consistency PASSED                                                 [ 81%]
tests/test_stream.py::test_stream_hash_equality_same_handle PASSED                                        [ 84%]
tests/test_stream.py::test_stream_hash_inequality_different_handles PASSED                                [ 87%]
tests/test_stream.py::test_stream_dict_key PASSED                                                         [ 90%]
tests/test_stream.py::test_stream_set_membership PASSED                                                   [ 93%]
tests/test_stream.py::test_builtin_stream_hash PASSED                                                     [ 96%]
tests/test_stream.py::test_default_stream_consistency PASSED                                              [100%]

============================================== 33 passed in 0.38s ===============================================

Monishver11 avatar Jan 15 '26 04:01 Monishver11

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 Jan 15 '26 04:01 copy-pr-bot[bot]

Hello @leofang. Can you please review this PR for #1494 fix?

Monishver11 avatar Jan 15 '26 19:01 Monishver11

/ok to test ff9b8af3986cb32fbba0ac3a5588ddcaaa587228

mdboom avatar Jan 15 '26 20:01 mdboom

Doc Preview CI :---: |

:rocket: View preview at
https://nvidia.github.io/cuda-python/pr-preview/pr-1496/
|
https://nvidia.github.io/cuda-python/pr-preview/pr-1496/cuda-core/
|
https://nvidia.github.io/cuda-python/pr-preview/pr-1496/cuda-bindings/
|
https://nvidia.github.io/cuda-python/pr-preview/pr-1496/cuda-pathfinder/

|

Preview will be ready when the GitHub Pages deployment is complete.

github-actions[bot] avatar Jan 15 '26 20:01 github-actions[bot]

Oh... one more thing: 3. Add a release note entry to cuda_core/docs/source/release/0.6.0-notes.rst (need to create the file and follow the established format)

leofang avatar Jan 16 '26 15:01 leofang

@leofang, thanks for the feedback. I'll add these changes shortly.

Monishver11 avatar Jan 16 '26 16:01 Monishver11

@leofang, I have made all the changes. Could you please review this again? I've checked and followed the rules for writing release notes, but let me know if any modifications are needed specifically.

Monishver11 avatar Jan 16 '26 16:01 Monishver11

@leofang one of the benefits of having these be classmethods of Stream is that we can make them continue to support being subclassed nicely without having to reach back to Stream.from_handle and knowing the underlying handles for these built-in streams. We could make sure the classmethods return a singleton.

Do you have anything specific in mind as far as why you prefer the named variables at the cuda.core level instead?

kkraus14 avatar Jan 16 '26 17:01 kkraus14

Hello @leofang. Just following up on this. I’ve addressed the three points you mentioned. Whenever you have a moment, could you please take another look? Thanks.

Monishver11 avatar Jan 20 '26 14:01 Monishver11