Skip to content

cuda_core forward compatibility changes. #722

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 4 commits into from
Jul 1, 2025

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Jun 21, 2025

Description

Forward compatibility changes for cuda_core.

This helps us staying organized.

Copy link
Contributor

copy-pr-bot bot commented Jun 21, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@rwgk
Copy link
Collaborator Author

rwgk commented Jun 21, 2025

/ok to test

@rwgk
Copy link
Collaborator Author

rwgk commented Jun 21, 2025

/ok to test

This comment has been minimized.

@rwgk
Copy link
Collaborator Author

rwgk commented Jun 22, 2025

/ok to test

@rwgk rwgk marked this pull request as ready for review June 22, 2025 05:26
Copy link
Contributor

copy-pr-bot bot commented Jun 22, 2025

Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@rwgk rwgk requested a review from leofang June 22, 2025 05:26
@leofang leofang added this to the cuda.core parking lot milestone Jun 23, 2025
@leofang leofang added enhancement Any code-related improvements P0 High priority - Must do! cuda.core Everything related to the cuda.core module labels Jun 23, 2025
@@ -476,7 +476,7 @@ def create_conditional_handle(self, default_value=None) -> driver.CUgraphConditi
default_value = 0
flags = 0

status, _, graph, _, _ = handle_return(driver.cuStreamGetCaptureInfo(self._mnff.stream.handle))
status, _, graph, *_, _ = handle_return(driver.cuStreamGetCaptureInfo(self._mnff.stream.handle))
Copy link
Member

Choose a reason for hiding this comment

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

Q: If we're packing anyway, maybe we pack them together?

Suggested change
status, _, graph, *_, _ = handle_return(driver.cuStreamGetCaptureInfo(self._mnff.stream.handle))
status, _, graph, *_ = handle_return(driver.cuStreamGetCaptureInfo(self._mnff.stream.handle))

@leofang
Copy link
Member

leofang commented Jul 1, 2025

@rwgk let's merge #709, resolve the conflicts, rerun the CI and merge.

@vzhurba01
Copy link
Collaborator

I took a look at the graphs side and while I'm ok with it, it does feels over-engineered.
The "tricks" used with deps_info_update feel bad because it's scattered through out the code (just 2 places but still... what if we end up doing more tricks to handle other forward compatibility APIs).

Thinking out loud, what if we move these weird APIs to a compatibility file/layer? At least this way we're keeping these tricks contained in a single place. Oh and by tricks I mean logic like [None] * (len(deps_info) - 1), and the packing * which is placed for some APIs at the end while in the middle for others...

@leofang
Copy link
Member

leofang commented Jul 1, 2025

@rwgk let's merge #709, resolve the conflicts, rerun the CI and merge.

FYI, #709 is merged

@rwgk
Copy link
Collaborator Author

rwgk commented Jul 1, 2025

/ok to test

@rwgk
Copy link
Collaborator Author

rwgk commented Jul 1, 2025

I took a look at the graphs side and while I'm ok with it, it does feels over-engineered.

I was very careful to make the unpacking consistent between all the uses, even where I could have taken a shortcut (*_, _), to reduce the cognitive burden. I find that far easier to read than duplicate code (what the LLMs suggested initially). The unpacking feature seems very intuitive and complete to me.

[None] * (len(deps_info) - 1)

Yeah, it's a bit over the top, but there are only two, and the circumstances are special.

@github-project-automation github-project-automation bot moved this from Todo to In Review in CCCL Jul 1, 2025
@leofang leofang merged commit 7ccf852 into NVIDIA:main Jul 1, 2025
53 checks passed
@github-project-automation github-project-automation bot moved this from In Review to Done in CCCL Jul 1, 2025
@leofang
Copy link
Member

leofang commented Jul 1, 2025

Thanks, Ralf!

Copy link

github-actions bot commented Jul 1, 2025

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

@rwgk rwgk deleted the cuda_core_fwd_compatibility branch July 2, 2025 04:23
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 enhancement Any code-related improvements P0 High priority - Must do!
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants