[trivial] Remove prometheus test #967
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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 removes the test file for Ray Prometheus stats integration. However, the corresponding feature implementation still exists in skyrl_train/inference_engines/vllm/vllm_engine.py and its configuration in ppo_base_config.yaml. Removing tests for existing functionality is a significant risk, as it can lead to future regressions going undetected. I recommend either restoring the tests or, if the feature is deprecated, removing the associated code from the engine and configuration files as well.
I am having trouble creating individual review comments. Click here to see my feedback.
skyrl-train/tests/cpu/inf_engines/vllm/test_ray_prometheus_stats.py (1-103)
These tests cover the enable_ray_prometheus_stats functionality, which still exists in AsyncVLLMInferenceEngine and VLLMInferenceEngine. Removing the tests without removing the feature code is not recommended as it leaves the functionality untested and prone to future regressions. If this feature is no longer needed, the related code should also be removed from skyrl_train/inference_engines/vllm/vllm_engine.py (e.g., the _create_ray_prometheus_stat_loggers method and handling of enable_ray_prometheus_stats in _create_engine) and the configuration option from skyrl_train/config/ppo_base_config.yaml. Otherwise, these tests should be kept.
No description provided.