Skip to content

Conversation

@fhl2000
Copy link
Contributor

@fhl2000 fhl2000 commented Sep 6, 2025

Purpose

Add design documents for the changes in #20059.

Previews:
https://vllm--24374.org.readthedocs.build/en/24374/design/cuda_graphs.html
https://vllm--24374.org.readthedocs.build/en/24374/design/torch_compile.html


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

@fhl2000 fhl2000 requested a review from hmellor as a code owner September 6, 2025 18:35
@mergify mergify bot added the documentation Improvements or additions to documentation label Sep 6, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a new design document for CUDA Graph v1, which is a great addition for understanding the new features. The document is comprehensive and well-structured. My main feedback is on the language quality of the new document, which has numerous typos and grammatical errors. I've left a single comment with a list of examples. Fixing these will significantly improve the document's quality. The update to torch_compile.md to link to this new document is appropriate.

Copy link
Member

@hmellor hmellor left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I've not looked closely at the content of the document yet but I have left some high level comments that can be actioned in the meantime.

Signed-off-by: fhl2000 <[email protected]>
Copy link
Member

@hmellor hmellor left a comment

Choose a reason for hiding this comment

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

With V0 effectively gone, we can drop V1 from the title

Copy link
Member

@hmellor hmellor left a comment

Choose a reason for hiding this comment

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

  • Could you remove the Pre: and Now: labels baked into the images and use Before: and After: in the markdown content to introduce the images?
  • Could you also not truncate PIECEWISE to PIECE.? IKt's not explained anywhere and only saves 3 characters, it's better to be explicit.

@fhl2000
Copy link
Contributor Author

fhl2000 commented Sep 15, 2025

Why did the pre-commit fail? @hmellor

Unordered list style [Expected: dash; Actual: asterisk]

I remember previously it asked for asterisk instead of dash.

@hmellor
Copy link
Member

hmellor commented Sep 15, 2025

Why did the pre-commit fail? @hmellor

Unordered list style [Expected: dash; Actual: asterisk]

I remember previously it asked for asterisk instead of dash.

Sorry this was my mistake. pre-commit wants the style to be the same throughout the document and it chooses whichever is used first. In my formatting change I added an unordered list and used a different symbol to the one you used

Signed-off-by: Harry Mellor <[email protected]>
@fhl2000
Copy link
Contributor Author

fhl2000 commented Sep 15, 2025

Is this ok? The truncated PIECE. in some places are kept for cleanliness (not much room to add)

image

Signed-off-by: Harry Mellor <[email protected]>
@hmellor
Copy link
Member

hmellor commented Sep 15, 2025

Is this ok? The truncated PIECE. in some places are kept for cleanliness (not much room to add)
image

These new diagrams do look very nice. It does look like PIECEWISE should fit in the CUDAGraphWrapper boxes. For the data flows at the bottom could the PIECEWISE be expanded to the left?

Signed-off-by: fhl <[email protected]>
@fhl2000
Copy link
Contributor Author

fhl2000 commented Sep 15, 2025

Any thoughts on this new pre-commit error?

Error: docs/design/cuda_graphs.md:63:12 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':']

Maybe just remove : or highlight it but not a heading?

--
Edited: it's fixed.

@fhl2000
Copy link
Contributor Author

fhl2000 commented Sep 20, 2025

The notes for attention ops fusion is changed, since #24281 merged!

@fhl2000
Copy link
Contributor Author

fhl2000 commented Sep 27, 2025

Have had some updates since #23046 and #25444 were merged.

Signed-off-by: fhl2000 <[email protected]>
@simon-mo
Copy link
Collaborator

@codex review

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. Breezy!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

Signed-off-by: fhl2000 <[email protected]>
@hmellor hmellor dismissed their stale review September 29, 2025 12:17

My formatting requests have been addressed

Copy link
Collaborator

@ProExpertProg ProExpertProg left a comment

Choose a reason for hiding this comment

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

Thanks for this increadible writeup! A few thoughts & suggestions below

fhl2000 and others added 4 commits October 7, 2025 09:18
Co-authored-by: Luka Govedič <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Copy link
Collaborator

@ProExpertProg ProExpertProg left a comment

Choose a reason for hiding this comment

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

Great work, thanks for writing this up!

@vllm-bot vllm-bot merged commit 63773a6 into vllm-project:main Oct 7, 2025
5 checks passed
mrasquinha-g pushed a commit to mrasquinha-g/vllm that referenced this pull request Oct 9, 2025
Signed-off-by: fhl <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
Signed-off-by: fhl <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
Signed-off-by: xuebwang-amd <[email protected]>
Dhruvilbhatt pushed a commit to Dhruvilbhatt/vllm that referenced this pull request Oct 14, 2025
Signed-off-by: fhl <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
Signed-off-by: Dhruvil Bhatt <[email protected]>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
Signed-off-by: fhl <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
alhridoy pushed a commit to alhridoy/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: fhl <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: fhl <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
Signed-off-by: xuebwang-amd <[email protected]>
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
Signed-off-by: fhl <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
devpatelio pushed a commit to SumanthRH/vllm that referenced this pull request Nov 29, 2025
Signed-off-by: fhl <[email protected]>
Signed-off-by: fhl2000 <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Co-authored-by: Luka Govedič <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants