Skip to content

[Bugfix] Remove VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE #2969

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

Jump to bottom
Merged
merged 3 commits into from
Sep 20, 2025
Merged

[Bugfix] Remove VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE #2969

merged 3 commits into from
Sep 20, 2025
+10 −14

Conversation

Lucaskabela
Copy link
Contributor

@Lucaskabela Lucaskabela commented Sep 16, 2025 •
edited by github-actions bot
Loading

What this PR does / why we need it?

This PR prepares for deleting this enviroment variable, VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE, as vllm requires fullgraph=True to run

Does this PR introduce any user-facing change?

No

How was this patch tested?

See CI

@github-actions GitHub Actions
Copy link

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

@Lucaskabela
Remove deprecated env variable
f210da4 
Signed-off-by: Lucas Kabela <[email protected]>
@Lucaskabela
format
9cbd6a6 
Signed-off-by: Lucas Kabela <[email protected]>
@Lucaskabela Lucaskabela force-pushed the lucaskabela/env_var_fullgraph_removal branch from cf2606f to 9cbd6a6 Compare September 16, 2025 23:32
@wangxiyuan wangxiyuan added ready read for review ready-for-test start test by label for PR labels Sep 17, 2025
@wangxiyuan wangxiyuan requested a review from yiz-liu September 17, 2025 00:58
@Lucaskabela Lucaskabela marked this pull request as ready for review September 17, 2025 16:10
@Lucaskabela Lucaskabela mentioned this pull request Sep 17, 2025
Merged
5 tasks
@Lucaskabela
Copy link
Contributor Author

Hi @yiz-liu - can you review this or suggest other reviewers to tag?

Yikun
Yikun approved these changes Sep 19, 2025
View reviewed changes
Copy link
Collaborator

@Yikun Yikun left a comment

Choose a reason for hiding this comment

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

LGTM, thx.

Consider the default value is True, and we don't use it in other place, so I think it's safe to merge.

I will do a double confirm and merge this morning.

[1] https://github.com/vllm-project/vllm/blob/14c1432789c9c1b66308481b2c37439d3ee6661a/vllm/envs.py#L440

@Yikun Yikun merged commit 53ecd89 into vllm-project:main Sep 20, 2025
17 checks passed
weijinqian0 pushed a commit to weijinqian0/vllm-ascend that referenced this pull request Sep 22, 2025
@Lucaskabela
[Bugfix] Remove VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE (vllm-project#2969)
5f56f2d 
### What this PR does / why we need it?
This PR prepares for deleting this enviroment variable,
`VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE`, as vllm requires `fullgraph=True`
to run

- Fixes vllm-project/vllm#21834

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
See CI 

- vLLM version: v0.10.2
- vLLM main:
vllm-project/vllm@99cc41a

---------

Signed-off-by: Lucas Kabela <[email protected]>
Mercykid-bash pushed a commit to Mercykid-bash/vllm-ascend that referenced this pull request Sep 22, 2025
@Lucaskabela @mercykid
[Bugfix] Remove VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE (vllm-project#2969)
a0718db 
### What this PR does / why we need it?
This PR prepares for deleting this enviroment variable,
`VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE`, as vllm requires `fullgraph=True`
to run

- Fixes vllm-project/vllm#21834

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
See CI

- vLLM version: v0.10.2
- vLLM main:
vllm-project/vllm@99cc41a

---------

Signed-off-by: Lucas Kabela <[email protected]>
Signed-off-by: Che Ruan <[email protected]>
Mercykid-bash pushed a commit to Mercykid-bash/vllm-ascend that referenced this pull request Sep 22, 2025
@Lucaskabela @mercykid
[Bugfix] Remove VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE (vllm-project#2969)
2561d8d 
### What this PR does / why we need it?
This PR prepares for deleting this enviroment variable,
`VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE`, as vllm requires `fullgraph=True`
to run

- Fixes vllm-project/vllm#21834

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
See CI

- vLLM version: v0.10.2
- vLLM main:
vllm-project/vllm@99cc41a

---------

Signed-off-by: Lucas Kabela <[email protected]>
Signed-off-by: Che Ruan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yikun Yikun Yikun approved these changes

@yiz-liu yiz-liu Awaiting requested review from yiz-liu

+1 more reviewer

@zou3519 zou3519 zou3519 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
ready read for review ready-for-test start test by label for PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Consider deleting envs.VLLM_TEST_DYNAMO_FULLGRAPH_CAPTURE
4 participants
@Lucaskabela @Yikun @zou3519 @wangxiyuan