Skip to content

[Misc] unify variable for LLM instance v2 #21356

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 1 commit into from
Jul 22, 2025
Merged

[Misc] unify variable for LLM instance v2 #21356

merged 1 commit into from
Jul 22, 2025

Conversation

andyxning
Copy link
Contributor

@andyxning andyxning commented Jul 22, 2025 •
edited by github-actions bot
Loading

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.

Fix #20996 (comment)

Purpose

This is a follow up about PR #20996 and is used to fix comment: #20996 (comment)

Test Plan

NA

Test Result

NA

(Optional) Documentation Update

@andyxning andyxning mentioned this pull request Jul 22, 2025
Merged
4 tasks
@andyxning
[Misc] unify variable for LLM instance v2
8c2bb04 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
@andyxning andyxning force-pushed the unify_variable_for_LLM_instance_v2 branch from 4b997b3 to 8c2bb04 Compare July 22, 2025 07:25
@github-actions GitHub Actions
Copy link

👋 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.

🚀

gemini-code-assist[bot]
gemini-code-assist bot reviewed Jul 22, 2025
View reviewed changes
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 continues the refactoring from a previous PR to unify the variable name for the LLM instance, changing llm.model to llm.llm in a test file. While the renaming is consistent, it appears to have propagated an incorrect attribute access path. I've pointed out a likely AttributeError and suggested a fix. It's important to ensure tests are run to catch such issues.

tests/models/language/generation/test_gemma.py
lambda self: self.model_runner.model.language_model.model.
normalizer.cpu().item())
config = llm.model.llm_engine.model_config.hf_config.text_config
config = llm.llm.llm_engine.model_config.hf_config.text_config
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

It seems there's a potential AttributeError here. Based on the LLMEngine implementation, an LLMEngine instance has a model_config attribute directly, but not an llm_engine attribute.

Assuming llm.llm is an instance of LLMEngine, the path to the config should be llm.llm.model_config.hf_config.text_config. The extra .llm_engine seems incorrect and will likely cause the test to fail.

config = llm.llm.model_config.hf_config.text_config

tests/models/language/generation/test_gemma.py
lambda self: self.model_runner.model.model.normalizer.cpu(
).item())
config = llm.model.llm_engine.model_config.hf_config
config = llm.llm.llm_engine.model_config.hf_config
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

Similar to the case above, it appears there's an incorrect attribute access here. The LLMEngine class does not have an llm_engine attribute.

If llm.llm is an LLMEngine instance, the correct way to access the model config would be directly via llm.llm.model_config.

config = llm.llm.model_config.hf_config

@DarkLight1337
Copy link
Member

Can you check the latest nightly run which runs all tests: https://buildkite.com/vllm/ci/builds?query=Nightly and fix any issues resulting from the naming change?

@andyxning
Copy link
Contributor Author

Will try this later. Let's first fix this problem.

Can you check the latest nightly run which runs all tests: https://buildkite.com/vllm/ci/builds?query=Nightly and fix any issues resulting from the naming change?

@DarkLight1337
Copy link
Member

I prefer fixing them all at once since CI is broken until then anyway

@andyxning
Copy link
Contributor Author

OK，will do this later today.

@andyxning
Copy link
Contributor Author

Seems that there is only one error is related to VllmRunner' object has no attribute 'model'. Others failures are not related with refactor. /cc @DarkLight1337

hmellor
hmellor approved these changes Jul 22, 2025
View reviewed changes
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.

The current changes LGTM.

@andyxning you can check the latest nightly build (https://buildkite.com/vllm/ci/builds?branch=main&query=nightly) to find any other failing tests related to the renaming.

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) July 22, 2025 09:58
@DarkLight1337
Copy link
Member

Seems that there is only one error is related to VllmRunner' object has no attribute 'model'. Others failures are not related with refactor.

Thanks for checking this! LGTM then

@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Jul 22, 2025
@andyxning
Copy link
Contributor Author

The current changes LGTM.

@andyxning you can check the latest nightly build (https://buildkite.com/vllm/ci/builds?branch=main&query=nightly) to find any other failing tests related to the renaming.

Yes. I have checked this. For now, there is only one.

@andyxning
Copy link
Contributor Author

@DarkLight1337 @hmellor Seems the ci failure is unrelated with this PR. Can you help verifying this? Seems flakes.

@DarkLight1337
Copy link
Member

Retrying

@DarkLight1337
Copy link
Member

The failure is unrelated so I'll just merge this

@vllm-bot vllm-bot merged commit 0df4d9b into vllm-project:main Jul 22, 2025
53 of 56 checks passed
@andyxning andyxning deleted the unify_variable_for_LLM_instance_v2 branch July 22, 2025 14:11
yeqcharlotte pushed a commit to yeqcharlotte/vllm that referenced this pull request Jul 23, 2025
@andyxning @yeqcharlotte
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
d5d5a0c 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
zixi-qi pushed a commit to zixi-qi/vllm that referenced this pull request Jul 23, 2025
@andyxning @zixi-qi
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
80634e8 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Signed-off-by: qizixi <qizixi@meta.com>
LyrisZhong pushed a commit to LyrisZhong/vllm that referenced this pull request Jul 23, 2025
@andyxning @LyrisZhong
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
6c64d34 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
avigny pushed a commit to avigny/vllm that referenced this pull request Jul 31, 2025
@andyxning @avigny
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
1b24d04 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Signed-off-by: avigny <47987522+avigny@users.noreply.github.com>
wenscarl pushed a commit to wenscarl/vllm that referenced this pull request Aug 4, 2025
@andyxning @wenscarl
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
a803d04 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Signed-off-by: shuw <shuw@nvidia.com>
x22x22 pushed a commit to x22x22/vllm that referenced this pull request Aug 5, 2025
@andyxning @x22x22
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
cf0b9ab 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Signed-off-by: x22x22 <wadeking@qq.com>
Pradyun92 pushed a commit to Pradyun92/vllm that referenced this pull request Aug 6, 2025
@andyxning
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
60b8181 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
npanpaliya pushed a commit to odh-on-pz/vllm-upstream that referenced this pull request Aug 6, 2025
@andyxning @npanpaliya
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
b6b13ba 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
jinzhen-lin pushed a commit to jinzhen-lin/vllm that referenced this pull request Aug 9, 2025
@andyxning @jinzhen-lin
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
5d229fe 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Signed-off-by: Jinzhen Lin <linjinzhen@hotmail.com>
paulpak58 pushed a commit to paulpak58/vllm that referenced this pull request Aug 13, 2025
@andyxning @paulpak58
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
928eb36 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Signed-off-by: Paul Pak <paulpak58@gmail.com>
taneem-ibrahim pushed a commit to taneem-ibrahim/vllm that referenced this pull request Aug 14, 2025
@andyxning @taneem-ibrahim
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
297d80f 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
diegocastanibm pushed a commit to diegocastanibm/vllm that referenced this pull request Aug 15, 2025
@andyxning @diegocastanibm
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
63177bd 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Signed-off-by: Diego-Castan <diego.castan@ibm.com>
epwalsh pushed a commit to epwalsh/vllm that referenced this pull request Aug 28, 2025
@andyxning @epwalsh
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
bbfa5a5 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
googlercolin pushed a commit to googlercolin/vllm that referenced this pull request Aug 29, 2025
@andyxning @googlercolin
[Misc] unify variable for LLM instance v2 (vllm-project#21356)
c2829d9 
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmellor hmellor hmellor approved these changes

@DarkLight1337 DarkLight1337 Awaiting requested review from DarkLight1337 DarkLight1337 is a code owner

@ywang96 ywang96 Awaiting requested review from ywang96 ywang96 is a code owner

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andyxning @DarkLight1337 @hmellor @vllm-bot