Skip to content

Construct instance of appropriate class in Event._init #752

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Construct instance of appropriate class in Event._init #752

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cuda_core/cuda/core/experimental/_event.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ cdef class Event:

@classmethod
def _init(cls, device_id: int, ctx_handle: Context, options=None):
cdef Event self = Event.__new__(Event)
cdef Event self = Event.__new__(cls)
Copy link
Contributor Author

@shwina shwina Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This ensures that a cls instance is produced here, which could be different from Event if cls is a subclass.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this works! I was trying:

cdef cls self = Event.__new__(cls)

and getting compile time errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test for a derived class of Event to ensure that we don't accidentally regress on what this enables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can go about this in two ways:

  • Write a test that invokes MyEvent.__new__ directly and test that it produces a MyEvent (easy)
  • Write a test that monkeypatches Event to MyEvent, and test that constructing an event via Stream or Device produces a MyEvent rather than an Event (more complex, but representative of how users can actually use subclasses).

Ultimately, (2) calls down to (1) but in a way that's an implementation detail. Any preferences?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets take the easy path and we can always revisit in the future if we see value in doing so.

cdef EventOptions opts = check_or_create_options(EventOptions, options, "Event options")
flags = 0x0
self._timing_disabled = False
Expand Down
Loading