Skip to content

Implement __hash__ and __eq__ for cuda.core classes - #1198

Merged
rparolin merged 38 commits into
NVIDIA:mainfrom
rparolin:rparolin/cuda.core.hashable
Nov 10, 2025
Merged

Implement __hash__ and __eq__ for cuda.core classes#1198
rparolin merged 38 commits into
NVIDIA:mainfrom
rparolin:rparolin/cuda.core.hashable

Conversation

@rparolin

@rparolin rparolin commented Oct 29, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Implement __hash__ and __eq__ for cuda.core classes

This PR implements __hash__() and __eq__() methods for cuda.core classes (Stream, Event, Context, Device), enabling them to be used as dictionary keys and in sets.

Fixes #664

@rparolin
Add hash and equality support to Stream, Event, Context, and Device c…
3254fbc 
…lasses, enabling their use as dictionary keys and in sets.
@copy-pr-bot

copy-pr-bot Bot commented Oct 29, 2025

Copy link
Copy Markdown
Contributor

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.

@rparolin
rparolin requested a review from leofang October 29, 2025 18:32
@rparolin

rparolin commented Oct 29, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test eeef948

@rparolin
rparolin marked this pull request as ready for review October 29, 2025 19:07
@rparolin rparolin added the cuda.core Everything related to the cuda.core module label Oct 29, 2025
@rparolin rparolin added this to the cuda.core beta 9 milestone Oct 29, 2025
@rparolin
rparolin requested a review from mdboom October 29, 2025 19:11
@Andy-Jost

Andy-Jost commented Oct 29, 2025

Copy link
Copy Markdown
Contributor

+1 for this change. Longer term, it would be great to achieve this sort of thing with a mix-in class. I think what gets in the way is some internal inconsistency, e.g., using _id versus _handle for handle-like members. Perhaps something to consider if we refactor or reimplement anything.

@github-actions

This comment has been minimized.

Sign in to view
oleksandr-pavlyk
oleksandr-pavlyk reviewed Oct 29, 2025
View reviewed changes
Comment thread cuda_core/cuda/core/experimental/_event.pyx Outdated
oleksandr-pavlyk
oleksandr-pavlyk reviewed Oct 29, 2025
View reviewed changes
Comment thread cuda_core/cuda/core/experimental/_event.pyx Outdated
oleksandr-pavlyk
oleksandr-pavlyk reviewed Oct 29, 2025
View reviewed changes
Comment thread cuda_core/cuda/core/experimental/_event.pyx
@rparolin

rparolin commented Oct 29, 2025

Copy link
Copy Markdown
Collaborator Author

+1 for this change. Longer term, it would be great to achieve this sort of thing with a mix-in class. I think what gets in the way is some internal inconsistency, e.g., using _id versus _handle for handle-like members. Perhaps something to consider if we refactor or reimplement anything.

What would be the benefit of using a mixin? I feel like it would require a bunch of machinary of parameterize it to the point it solves the problems you pointed out (ie. being able to handle different handle member names)

@rparolin @oleksandr-pavlyk
Update cuda_core/cuda/core/experimental/_event.pyx
714091c 
Co-authored-by: Oleksandr Pavlyk <21087696+oleksandr-pavlyk@users.noreply.github.com>
@rparolin

rparolin commented Oct 29, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test 714091c

@rparolin

rparolin commented Oct 29, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test fff600c

@rparolin

rparolin commented Oct 29, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test 8227c21

@rparolin rparolin added the P1 Medium priority - Should do label Oct 29, 2025
@rparolin rparolin self-assigned this Oct 29, 2025
leofang
leofang reviewed Oct 29, 2025
View reviewed changes
Comment thread cuda_core/cuda/core/experimental/_device.pyx Outdated
@leofang leofang added the enhancement Any code-related improvements label Oct 29, 2025
@rparolin

rparolin commented Oct 29, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test b8cc00d

mdboom
mdboom reviewed Oct 30, 2025
View reviewed changes
Comment thread cuda_core/cuda/core/experimental/_stream.pyx Outdated
leofang
leofang reviewed Nov 3, 2025
View reviewed changes
Comment thread cuda_core/cuda/core/experimental/_event.pyx Outdated
leofang
leofang requested changes Nov 3, 2025
View reviewed changes
Comment thread cuda_core/cuda/core/experimental/_stream.pyx
leofang
leofang reviewed Nov 3, 2025
View reviewed changes
Comment thread cuda_core/cuda/core/experimental/_stream.pyx Outdated
leofang
leofang reviewed Nov 3, 2025
View reviewed changes
Comment thread cuda_core/cuda/core/experimental/_stream.pyx Outdated
@rparolin

rparolin commented Nov 4, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test af5c52e

@rparolin

rparolin commented Nov 4, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test 8c9900d

@rparolin

rparolin commented Nov 4, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test 95047e4

@rparolin
rparolin requested review from Andy-Jost and leofang November 4, 2025 23:33
kkraus14
kkraus14 reviewed Nov 5, 2025
View reviewed changes
Comment thread cuda_core/cuda/core/experimental/_device.pyx Outdated
Comment thread cuda_core/cuda/core/experimental/_device.pyx Outdated
Comment thread cuda_core/cuda/core/experimental/_event.pyx
Comment thread cuda_core/cuda/core/experimental/_context.pyx
Comment thread cuda_core/tests/test_context.py
Comment on lines +14 to +16
# ============================================================================
# Context Equality Tests
# ============================================================================

@kkraus14 kkraus14 Nov 5, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would probably be difficult, but is there any way we can add a test for when we have different contexts and/or an invalid context in some way?

Comment thread cuda_core/cuda/core/experimental/_stream.pyx
mdboom
mdboom reviewed Nov 5, 2025
View reviewed changes

@mdboom mdboom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My comments aren't requirements -- I mostly defer to previous reviewers here.

Comment thread cuda_core/cuda/core/experimental/_context.pyx
Comment thread cuda_core/cuda/core/experimental/_device.pyx Outdated
Comment thread cuda_core/tests/test_comparable.py
@rparolin

rparolin commented Nov 6, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test ae74733

@rparolin

rparolin commented Nov 6, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test 98e8fe0

@rparolin
rparolin merged commit 7ca634b into NVIDIA:main Nov 10, 2025
106 of 111 checks passed
@github-actions

github-actions Bot commented Nov 10, 2025

Copy link
Copy Markdown
Doc Preview CI
Preview removed because the pull request was closed or merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leofang leofang leofang approved these changes
@cpcloud cpcloud Awaiting requested review from cpcloud
@rwgk rwgk Awaiting requested review from rwgk
@Andy-Jost Andy-Jost Awaiting requested review from Andy-Jost
@mdboom mdboom Awaiting requested review from mdboom
@kkraus14 kkraus14 Awaiting requested review from kkraus14
@oleksandr-pavlyk oleksandr-pavlyk Awaiting requested review from oleksandr-pavlyk

Assignees

@rparolin rparolin

Labels

cuda.core Everything related to the cuda.core module enhancement Any code-related improvements P1 Medium priority - Should do

Projects

None yet

Milestone

cuda.core beta 10

Development

Successfully merging this pull request may close these issues.

[FEA]: cuda.core constructs which share underlying C structure should hash to same value

8 participants

@rparolin @Andy-Jost @mdboom @cpcloud @rwgk @kkraus14 @leofang @oleksandr-pavlyk