-
Notifications
You must be signed in to change notification settings - Fork 222
[skyrl-train] Upgrade vllm to 0.13.0 (and torch to 2.9.0) for both vllm and mcore extras #887
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.
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
Conversation
There was a problem hiding this 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.
| # 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| # 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 |
Upgrading vllm to latest (minor changes).
Needed for #885 and should subsume #882
Upgrades transformer-engine from 2.9.0 -> 2.10.0 for the megatron backend due to incompatibility with triton 3.5 (required by torch 2.9.0)
Keeps the vllm_engine.py path backwards compatible for versions of vllm < 0.11.2 (important for flash-rl integration)