Skip to content

Conversation

@erictang000
Copy link
Collaborator

Upgrading vllm to latest (minor changes).

Needed for #885 and should subsume #882

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 upgrades vllm to version 0.13.0 and torch to 2.9.0, which is a necessary update. The changes include updating dependencies in pyproject.toml and adapting the Python code in vllm_engine.py to accommodate breaking changes in the new vllm version. The changes are generally good, but I have one suggestion to improve the readability and robustness of the API compatibility logic.

Comment on lines 368 to 375
# vllm >= 0.11.2 removed model_config from OpenAI serving APIs
is_new_api = version.parse(vllm.__version__) >= version.parse("0.11.2")
legacy_kwargs = {} if is_new_api else {"model_config": model_config}

if is_new_api:
models = OpenAIServingModels(engine, base_model_paths)
else:
models = OpenAIServingModels(engine, model_config=model_config, base_model_paths=base_model_paths)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The logic to handle backward compatibility for different vllm API versions is functionally correct but could be simplified for better readability and maintainability. By grouping the version-specific logic, the code becomes easier to follow.

Additionally, the else block for OpenAIServingModels instantiation was changed to use keyword arguments. While this can improve explicitness, it's safer to retain the original positional arguments for the older API path to guarantee backward compatibility, as the function signature for that version might not support keyword arguments for these parameters.

This suggested change refactors the logic to be more grouped and uses a safer approach for the legacy path.

Suggested change
# vllm >= 0.11.2 removed model_config from OpenAI serving APIs
is_new_api = version.parse(vllm.__version__) >= version.parse("0.11.2")
legacy_kwargs = {} if is_new_api else {"model_config": model_config}
if is_new_api:
models = OpenAIServingModels(engine, base_model_paths)
else:
models = OpenAIServingModels(engine, model_config=model_config, base_model_paths=base_model_paths)
# vllm >= 0.11.2 removed model_config from OpenAI serving APIs
is_new_api = version.parse(vllm.__version__) >= version.parse("0.11.2")
legacy_kwargs = {}
if is_new_api:
models = OpenAIServingModels(engine, base_model_paths)
else:
models = OpenAIServingModels(engine, model_config, base_model_paths)
legacy_kwargs["model_config"] = model_config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants