Skip to content

NEW: Add Event to public API #501

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.

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

Merged
merged 1 commit into from
Mar 12, 2025
Merged

Conversation

carterbox
Copy link
Contributor

@carterbox carterbox commented Mar 7, 2025

Downstream packages that want to use Event to type hint their APIs should be able to import Event without accessing private modules.

#469 (comment)

I understand that Events should not be constructed directly, but having __new__() raise a RuntimeError should be sufficient to prevent that.

Copy link
Contributor

copy-pr-bot bot commented Mar 7, 2025

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.

Downstream packages that want to use Event to type hint their APIs
should be able to import Event without accessing private modules.
@carterbox carterbox force-pushed the dching/importable-event branch from f039f14 to 9449cab Compare March 7, 2025 21:01
@carterbox
Copy link
Contributor Author

Force-pushed to rebase on latest main.

@leofang
Copy link
Member

leofang commented Mar 11, 2025

btw we are on code freeze now, we could try to squeeze this in if it does not impact functionality and is an easy fix.

I understand that Events should not be constructed directly, but having __new__() raise a RuntimeError should be sufficient to prevent that.

What are your thoughts on

  1. move Event from cuda_core/docs/source/api_private.rst to cuda_core/docs/source/api.rst?
  2. apply the same treatment to everything else (ex: Stream)?

This was not done because I worried that people just try to construct them directly, as they are not meant to be an entry point of this module. However, it does feel weird that important objects like these are not explicitly documented, and it takes a few clicks to get to their docs. Typing is one of the straws, too, if not the last.

@leofang leofang requested a review from rwgk March 11, 2025 23:52
@leofang leofang added documentation Improvements or additions to documentation triage Needs the team's attention P1 Medium priority - Should do cuda.core Everything related to the cuda.core module labels Mar 11, 2025
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@rwgk
Copy link
Collaborator

rwgk commented Mar 12, 2025

/ok to test

This comment has been minimized.

@rwgk
Copy link
Collaborator

rwgk commented Mar 12, 2025

Per offline chat with @leofang: I'll merge this now and will work on api.rst changes separately, after we lift our code freeze.

@rwgk rwgk merged commit f5dc7e3 into NVIDIA:main Mar 12, 2025
77 checks passed
Copy link

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

@carterbox
Copy link
Contributor Author

However, it does feel weird that important objects like these are not explicitly documented, and it takes a few clicks to get to their docs. Typing is one of the straws, too, if not the last.

I think this is the main issue. Objects like Event are referenced and you have to use them, but finding the documentation on how to use them has friction.

@rwgk
Copy link
Collaborator

rwgk commented Mar 12, 2025

However, it does feel weird that important objects like these are not explicitly documented, and it takes a few clicks to get to their docs. Typing is one of the straws, too, if not the last.

I think this is the main issue. Objects like Event are referenced and you have to use them, but finding the documentation on how to use them has friction.

I agree (based on offline chat, I think Leo, too). Did you see my previous comment? — "I'll merge this now and will work on api.rst changes separately, after we lift our code freeze."

I'll send that PR to you for review.

@carterbox
Copy link
Contributor Author

Did you see my previous comment?

Yes. I was just expressing my agreement. No rush from me on this. I didn't even expect this PR to be merged until next month.

I'll send that PR to you for review.

I'd be happy to review that.

@carterbox carterbox deleted the dching/importable-event branch March 12, 2025 16:26
@rwgk
Copy link
Collaborator

rwgk commented Mar 12, 2025

I didn't even expect this PR to be merged until next month.

Oh, I assumed that it's helpful to you (and in general) to have public access to Event sooner rather than later.

@leofang leofang added this to the cuda.core beta 3 milestone Mar 22, 2025
@leofang leofang removed the triage Needs the team's attention label Mar 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda.core Everything related to the cuda.core module documentation Improvements or additions to documentation P1 Medium priority - Should do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants