Skip to content

Conversation

sarckk
Copy link
Collaborator

@sarckk sarckk commented Aug 5, 2025

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.

Purpose

Required fro #22628

#21044 shows an example of compiling multiple submodules within a model. We also want to provide the user the flexibility of triggering this on/off.

This PR adds a new arg enable_if to the support_torch_compile decorator, which is a function that takes in VllmConfig and returns a bool of whether to enable compile or not.

Test Plan

added new unit tests

pytest tests/compile/test_decorator.py
pytest tests/compile/piecewise/test_multiple_graphs.py

Test Result

Tests pass

cc: @zou3519 @BoyuanFeng

Copy link

github-actions bot commented Aug 5, 2025

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

@sarckk sarckk changed the title Support conditional torch.compile Support conditional torch.compile per module Aug 5, 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 introduces a compile_cond argument to the support_torch_compile decorator, allowing for conditional compilation of models. The implementation is clean and well-tested. I've found one minor issue in the new test file that should be addressed. Overall, this is a great addition that increases the flexibility of the compilation framework.

@sarckk sarckk force-pushed the conditional-compile branch 2 times, most recently from a51f22b to 8e302bb Compare August 6, 2025 18:33
@mergify mergify bot added the ci/build label Aug 6, 2025
@ProExpertProg ProExpertProg added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 11, 2025
Copy link
Collaborator

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

minor nit around the name but otherwise lgtm

@sarckk sarckk force-pushed the conditional-compile branch from 8bf5031 to d48647a Compare August 11, 2025 23:20
@DarkLight1337
Copy link
Member

Can you merge from main to fix the CI failures?

@sarckk sarckk force-pushed the conditional-compile branch 2 times, most recently from 58b56f2 to bd849d6 Compare August 14, 2025 03:49
@ProExpertProg ProExpertProg enabled auto-merge (squash) August 15, 2025 17:28
@ProExpertProg
Copy link
Collaborator

@sarckk failures appear related, could you take a look?

auto-merge was automatically disabled August 18, 2025 17:17

Head branch was pushed to by a user without write access

@sarckk sarckk force-pushed the conditional-compile branch from e11fe22 to 5d00f1e Compare August 18, 2025 17:17
@sarckk
Copy link
Collaborator Author

sarckk commented Aug 18, 2025

Fixed tests that were broken due to changes in #20059.

sarckk added 4 commits August 19, 2025 12:21
Signed-off-by: Yong Hoon Shin <[email protected]>
Signed-off-by: Yong Hoon Shin <[email protected]>
Signed-off-by: Yong Hoon Shin <[email protected]>
@ProExpertProg ProExpertProg enabled auto-merge (squash) August 19, 2025 16:24
@ProExpertProg ProExpertProg disabled auto-merge August 19, 2025 16:24
@ProExpertProg ProExpertProg enabled auto-merge (squash) August 19, 2025 16:25
@sarckk
Copy link
Collaborator Author

sarckk commented Aug 19, 2025

@ProExpertProg thanks for the rebases. CI seems quite noisy, should we keep rebasing until tests pass?

@ProExpertProg
Copy link
Collaborator

Yeah check if these failures are happening on main and restart ci

@ProExpertProg ProExpertProg merged commit dfd2382 into vllm-project:main Aug 20, 2025
71 of 72 checks passed
djmmoss pushed a commit to djmmoss/vllm that referenced this pull request Aug 21, 2025
BoyuanFeng pushed a commit to BoyuanFeng/vllm that referenced this pull request Aug 21, 2025
kylesayrs pushed a commit to neuralmagic/vllm that referenced this pull request Aug 22, 2025
shanes-cerebras pushed a commit to smsegal/vllm that referenced this pull request Aug 24, 2025
epwalsh pushed a commit to epwalsh/vllm that referenced this pull request Aug 28, 2025
juuice-lee pushed a commit to juuice-lee/vllm-moe.code that referenced this pull request Aug 28, 2025
xiao-llm pushed a commit to xiao-llm/vllm that referenced this pull request Aug 28, 2025
xiao-llm pushed a commit to xiao-llm/vllm that referenced this pull request Aug 28, 2025
zhewenl pushed a commit to zhewenl/vllm that referenced this pull request Aug 28, 2025
dumb0002 pushed a commit to dumb0002/vllm that referenced this pull request Aug 28, 2025
2015aroras pushed a commit to 2015aroras/vllm that referenced this pull request Aug 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants