Skip to content

Fix an apparent mistake in GraphBuilder.add_child #879

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

Merged
leofang merged 2 commits into from
Aug 21, 2025
Merged

Fix an apparent mistake in GraphBuilder.add_child #879

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb1c0f9
Fix an apparent mistake in GraphBuilder.add_child
oleksandr-pavlyk Aug 21, 2025
7f9c16b
Implemented fix to work with both CTK 12.9 and CTK 13.0
oleksandr-pavlyk Aug 21, 2025
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/_graph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ def add_child(self, child_graph: GraphBuilder):
[
handle_return(
driver.cuGraphAddChildGraphNode(
graph_out, deps_info_out[0], num_dependencies_out, child_graph._mnff.graph
graph_out, *deps_info_out, num_dependencies_out, child_graph._mnff.graph

@leofang leofang Aug 21, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It was done like this because during CUDA 13 bringup @rwgk wanted to make cuda-core work for both 12 and 13 (which have different signatures) without revealing anything about 13 (#722). @vzhurba01 expressed concerns that make sense to me. Now that we are bitten by this and that 13 is out, we should properly check the binding version and not try to hide the differences.

@oleksandr-pavlyk oleksandr-pavlyk Aug 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The way to hide discrepancy is actually few lines below, where we pad deps_info_update with None to match the size of deps_info_out.

Using deps_info_out[0] while using num_dependencies_out is a mistake. We could keep deps_info_out[0] by change num_dependencies_out to 1. This runs the risk of missing dependencies though.

@rwgk rwgk Aug 21, 2025

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.

This looks good to me.

We could branch with some sort of CTK < 13, but the resulting copy-paste introduces its own class of potential accidents. I'm not sure it'll be less error prone than the compact implementation that we have right now.

Where I went wrong in #722: I wasn't careful enough about reviewing the documentation for cuGraphAddChildGraphNode. I think even if I had taken the copy-paste route, I might have made this mistake (sorry). When the tests passed, I didn't look any further.

)
)
]
Expand Down
Loading