[Fix] Fix failing sglang test after #234 #237
Merged
SumanthRH merged 3 commits intoNovaSky-AI:mainfrom Sep 3, 2025
Merged
Conversation
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request fixes a failing sglang test by correctly passing sampling_params to the generation functions. It also adds a safeguard to prevent a NameError during resource cleanup in the test's finally block. The changes are correct and address the issue. I have one suggestion to improve the code's robustness and style in the resource cleanup logic.
Member
Author
|
All GPU tests pass locally now. I also added |
dzorlu
referenced
this pull request
in fleet-ai/SkyRL
Feb 4, 2026
# What does this PR do? Fixes failing sglang test after #234 The failure: ```bash -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html =========================== short test summary info ============================ FAILED tests/gpu/gpu_ci/test_engine_generation.py::test_inference_engines_generation[sglang] - TypeError: run_batch_generation() missing 1 required positional argument: 'sampling_params' ====== 1 failed, 2 passed, 25 deselected, 32 warnings in 94.94s (0:01:34) ====== sys:1: DeprecationWarning: builtin type swigvarlink has no __module__ attribute ``` After fixes in this PR: ```bash ... =========================================================================================== warnings summary ============================================================================================ tests/gpu/gpu_ci/test_engine_generation.py:226 /home/ray/default/SkyRL/skyrl-train/tests/gpu/gpu_ci/test_engine_generation.py:226: PytestUnknownMarkWarning: Unknown pytest.mark.vllm - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html pytest.param("vllm", 2, marks=pytest.mark.vllm), tests/gpu/gpu_ci/test_engine_generation.py:228 /home/ray/default/SkyRL/skyrl-train/tests/gpu/gpu_ci/test_engine_generation.py:228: PytestUnknownMarkWarning: Unknown pytest.mark.sglang - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html pytest.param("sglang", 1, marks=pytest.mark.sglang), tests/gpu/gpu_ci/test_engine_generation.py:325 /home/ray/default/SkyRL/skyrl-train/tests/gpu/gpu_ci/test_engine_generation.py:325: PytestUnknownMarkWarning: Unknown pytest.mark.vllm - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html pytest.param("vllm", 2, marks=pytest.mark.vllm), tests/gpu/gpu_ci/test_engine_generation.py:327 /home/ray/default/SkyRL/skyrl-train/tests/gpu/gpu_ci/test_engine_generation.py:327: PytestUnknownMarkWarning: Unknown pytest.mark.sglang - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html pytest.param("sglang", 1, marks=pytest.mark.sglang), tests/gpu/gpu_ci/test_engine_generation.py::test_inference_engines_generation[sglang] tests/gpu/gpu_ci/test_engine_generation.py::test_token_based_generation[sglang] /home/ray/default/SkyRL/skyrl-train/tests/gpu/gpu_ci/test_engine_generation.py:32: UserWarning: The version_base parameter is not specified. Please specify a compatability version level, or None. Will assume defaults for version 1.1 with hydra.initialize_config_dir(config_dir=config_dir): tests/gpu/gpu_ci/test_engine_generation.py: 18 warnings /home/ray/.cache/uv/builds-v0/.tmpJvEBqw/lib/python3.12/site-packages/multiprocess/popen_fork.py:66: DeprecationWarning: This process (pid=613265) is multi-threaded, use of fork() may lead to deadlocks in the child. self.pid = os.fork() tests/gpu/gpu_ci/test_engine_generation.py::test_inference_engines_generation[sglang] <frozen importlib._bootstrap>:488: DeprecationWarning: builtin type SwigPyPacked has no __module__ attribute tests/gpu/gpu_ci/test_engine_generation.py::test_inference_engines_generation[sglang] <frozen importlib._bootstrap>:488: DeprecationWarning: builtin type SwigPyObject has no __module__ attribute -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html ======================================================================= 2 passed, 2 deselected, 26 warnings in 162.71s (0:02:42) ======================================================================== ``` I also added set -e to the bash scripts in `skyrl-train/ci/` - we run groups of tests one by one and you can't catch failures from previous commands otherwise. --------- Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes failing sglang test after #234
The failure:
After fixes in this PR: