Skip to content

Conversation

hmellor
Copy link
Member

@hmellor hmellor commented Sep 19, 2025

Closes #18953

Notable changes:

  • Several places were importing QuantizationConfg from config.__init__ which is not where it is defined. Even worse, at runtime, QuantizationConfig from config.__init__.py is Any.
  • I've gone though every import of QuantizationConfig making sure it's imported from the right place.
  • This required a lot of type checking changes to gptq_utils.

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
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 is a great refactoring that improves the organization of the configuration files by moving them into their own modules. The code is now much cleaner and more modular. However, I've found a few critical issues that need to be addressed before merging. Specifically, there's an incorrect import for the logger, a circular dependency that would break the application, and a breaking API change due to not re-exporting the main VllmConfig class. My review comments provide details on how to fix these issues.

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@mergify mergify bot added llama Related to Llama models speculative-decoding tpu Related to Google TPUs labels Sep 20, 2025
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Copy link

mergify bot commented Sep 21, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @hmellor.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Sep 21, 2025
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@mergify mergify bot removed the needs-rebase label Sep 22, 2025
Copy link

mergify bot commented Sep 22, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @hmellor.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Sep 22, 2025
@hmellor
Copy link
Member Author

hmellor commented Sep 24, 2025

DeviceConfig, ObservabilityConfig, SpeechToTextConfig split into #25564 to reduce the surface for conflicts

@hmellor hmellor changed the title Move remaining configs from config/__init__.py to their own files MoveVllmConfig from config/__init__.py to `config/vllm.py Sep 24, 2025
@hmellor hmellor changed the title MoveVllmConfig from config/__init__.py to `config/vllm.py MoveVllmConfig from config/__init__.py to config/vllm.py Sep 24, 2025
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@mergify mergify bot removed the needs-rebase label Sep 29, 2025
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@hmellor hmellor enabled auto-merge (squash) September 29, 2025 17:53
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 29, 2025
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@vllm-bot vllm-bot merged commit 61aedb5 into vllm-project:main Sep 30, 2025
57 checks passed
@hmellor hmellor deleted the extract-all-configs branch September 30, 2025 05:33
pdasigi pushed a commit to pdasigi/vllm that referenced this pull request Oct 2, 2025
…project#25271)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
yewentao256 pushed a commit that referenced this pull request Oct 3, 2025
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: yewentao256 <zhyanwentao@126.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llama Related to Llama models ready ONLY add when PR is ready to merge/full CI is needed speculative-decoding tpu Related to Google TPUs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: vLLM configuration refactoring and modularization
3 participants