Conversation
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a conflict with a native vllm endpoint by renaming /update_weights to /update_weights_skyrl. However, a critical security vulnerability was identified: these sensitive endpoints lack any form of authentication. This allows anyone with network access to the inference server to trigger model weight updates, which could lead to model hijacking or denial of service. It is highly recommended to implement authentication for these custom endpoints, especially if the servers are reachable over a network. Additionally, to improve maintainability, consider using a constant for the new endpoint path to avoid hardcoding it in multiple locations.
|
|
||
| resp = await session.post( | ||
| f"{self._url}/update_weights", | ||
| f"{self._url}/update_weights_skyrl", |
There was a problem hiding this comment.
The endpoint path /update_weights_skyrl is hardcoded. This string is also used in the corresponding server implementation. To improve maintainability and ensure consistency, consider defining this path as a constant in a shared module (e.g., in a new constants.py file) and importing it where needed. This would make future changes to the endpoint easier and less error-prone.
| Dict mapping server_url to response. | ||
| """ | ||
| return await self._call_all_servers("/update_weights", request.to_json_dict()) | ||
| return await self._call_all_servers("/update_weights_skyrl", request.to_json_dict()) |
There was a problem hiding this comment.
The endpoint path /update_weights_skyrl is hardcoded. This string is also used in the corresponding server implementation. To improve maintainability and ensure consistency, consider defining this path as a constant in a shared module (e.g., in a new constants.py file) and importing it where needed. This would make future changes to the endpoint easier and less error-prone.
| return {"status": "ok", "server_id": server_id} | ||
|
|
||
| @app.post("/update_weights") | ||
| @app.post("/update_weights_skyrl") |
There was a problem hiding this comment.
The endpoint path /update_weights_skyrl is hardcoded in this test's mock server. To ensure tests stay in sync with the application code, it would be better to import the endpoint path from the same shared constant used in the application code.
| # Verify correct endpoint called | ||
| call_args = mock_session.post.call_args | ||
| assert call_args[0][0] == f"{url}/update_weights" | ||
| assert call_args[0][0] == f"{url}/update_weights_skyrl" |
There was a problem hiding this comment.
The endpoint path /update_weights_skyrl is hardcoded in this test assertion. To ensure tests stay in sync with the application code, it would be better to import the endpoint path from the same shared constant used in the application code and use it in the assertion.
1 participant
What does this PR do?
Temporary fix for weight sync CI failures reported in #1242 to unblock release.
Changes
test_weight_sync.pywas failing because SkyRL's/update_weightsendpoint was conflicting with vLLM's/update_weightsendpoint.This PR fixes the issue by renaming our update weights endpoint to
/update_weights_skyrl.The changes have been replicated even for the remote server codepath in the old
InferenceEngineClientcodepath (generators/inference_engines).The long term fix is to migrate SkyRL's new inference servers codepath (
generators/inference_servers) to use the native vLLM API endpoints.