Skip to content

[Perf] Enable environment cache in EngineCore to enable the feature for UniProcExecutor as well#29289

Merged
mgoin merged 4 commits intovllm-project:mainfrom
Jialin:env
Dec 10, 2025
Merged

[Perf] Enable environment cache in EngineCore to enable the feature for UniProcExecutor as well#29289
mgoin merged 4 commits intovllm-project:mainfrom
Jialin:env

Conversation

@Jialin
Copy link
Copy Markdown
Collaborator

@Jialin Jialin commented Nov 24, 2025

Purpose

Expand the coverage of the feature. Similar to #26146, we found the environment variable is beneficial to output processing as well.

Screenshot 2025-11-23 at 6 29 17 PM

Test Plan & Test Result

CI Signals


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.

@Jialin Jialin requested review from 22quinn and zhuohan123 November 24, 2025 02:29
Copy link
Copy Markdown
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 refactors the environment variable caching by moving the enable_envs_cache() call from EngineCoreProc to the base class EngineCore. This correctly extends the performance optimization to UniProcExecutor and other non-distributed execution paths. I have reviewed the change and its impact on different executors and initialization sequences. The new placement of the call is safe, occurring after environment-dependent setup and before performance-critical code paths. The change is sound and provides a good performance improvement.

@mergify mergify bot added the v1 label Nov 24, 2025
@heheda12345
Copy link
Copy Markdown
Collaborator

Also CC @mgoin

Copy link
Copy Markdown
Member

@mgoin mgoin left a comment

Choose a reason for hiding this comment

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

Nice

@mgoin mgoin added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 26, 2025
@Jialin Jialin force-pushed the env branch 2 times, most recently from 80a49b3 to 1b9858f Compare December 4, 2025 21:16
@Jialin
Copy link
Copy Markdown
Collaborator Author

Jialin commented Dec 4, 2025

Failed to repro CI test failure locally, so rebase against to confirm if it's a transient issue.

@Jialin
Copy link
Copy Markdown
Collaborator Author

Jialin commented Dec 8, 2025

The CI failure is due to multiple unit tests are running in the same process, so the later test is misusing the environment variable cached in the previous test.

To fix this, introduce disable_envs_cache and invoke in cleanup_dist_env_and_memory which should fully isolate environment variables across 2 tests.

…ons to UniProcExecutor

Signed-off-by: Jialin Ouyang <Jialin.Ouyang@gmail.com>
Signed-off-by: Jialin Ouyang <Jialin.Ouyang@gmail.com>
Signed-off-by: Jialin Ouyang <Jialin.Ouyang@gmail.com>
Signed-off-by: Jialin Ouyang <Jialin.Ouyang@gmail.com>
@mgoin mgoin merged commit 9f042ba into vllm-project:main Dec 10, 2025
52 checks passed
Majid-Taheri pushed a commit to Majid-Taheri/vllm that referenced this pull request Dec 23, 2025
…or UniProcExecutor as well (vllm-project#29289)

Signed-off-by: Jialin Ouyang <Jialin.Ouyang@gmail.com>
Signed-off-by: Ubuntu <mjtaheri68@gmail.com>
dsuhinin pushed a commit to dsuhinin/vllm that referenced this pull request Jan 21, 2026
…or UniProcExecutor as well (vllm-project#29289)

Signed-off-by: Jialin Ouyang <Jialin.Ouyang@gmail.com>
Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants