[train][tests] Add tests for `RemoteInferenceClient` by SumanthRH · Pull Request #1211 · NovaSky-AI/SkyRL

SumanthRH · 2026-02-25T06:56:14Z

What does this PR do?

Adds generation, error handling and chat template related tests for the new inference servers codepath . With this PR, we should have test coverage for all the features around basic full param training with the inference servers (with E2E tests on the same in progress).

Changes

Error handling: Previously, if errors were raised by vllm servers, the error message was not propagated to the client because we didn't read the body. This PR makes a change so that RemoteInferenceClient will read the error message details before raising a HTTP error.
Served model name support: Propagated cfg.generator.served_model_name to vLLM args for the new inference server codepath
Add generation, error handling and chat template tests: Added all relevant tests from the test_inference_engine_client_http_endpoint.py for the new inference server codepath.

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

…ngine-test

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

gemini-code-assist

Code Review

This pull request introduces significant improvements to the RemoteInferenceClient by enhancing error handling to propagate detailed messages from vLLM servers. It also adds support for served_model_name in the vLLM CLI arguments, allowing for more flexible model identification. Crucially, new tests have been added to cover generation, error handling, and chat template functionalities for the new inference server codepath, increasing test coverage and robustness. Minor updates to the CI script and test utilities ensure these new features are properly integrated and tested.

...kends/skyrl_train/gpu/gpu_ci/inference_servers/test_remote_inference_client_chat_template.py

...backends/skyrl_train/gpu/gpu_ci/inference_servers/test_remote_inference_client_generation.py

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

skyrl/backends/skyrl_train/inference_servers/remote_inference_client.py

skyrl/backends/skyrl_train/inference_servers/utils.py

tests/backends/skyrl_train/gpu/conftest.py

ci/gpu_ci_run_skyrl_train.sh

tests/backends/skyrl_train/gpu/utils.py

kouroshHakha · 2026-02-25T19:33:07Z

tests/backends/skyrl_train/gpu/utils.py

        router = None
        server_group = None
-        if _SKYRL_USE_NEW_INFERENCE:
+        if use_new_inference_servers or (use_new_inference_servers is None and _SKYRL_USE_NEW_INFERENCE):


why not keep the _SKYRL_USE_NEW_INFERENCE as the gate and control it in the tests?

or if you want to change this part, I would just do

if use_new_inference_servers is True: ...

Then in the caller of those other places I would do

InferenceEngineState.create(..., use_new_inference_servers=os.environ.get("_SKYRL_USE_NEW_INFERENCE", False))

These are tests specifically written for the new inference codepath. I don't think it makes sense to plan for both possible values of _SKYRL_USE_NEW_INFERENCE .

That is the main reason why the tests use the new inference codepath by default and are placed in inference_servers/

kouroshHakha · 2026-02-25T19:41:06Z

...kends/skyrl_train/gpu/gpu_ci/inference_servers/test_remote_inference_client_chat_template.py

+            model=MODEL_QWEN3,
+            sleep_level=1,
+            engine_init_kwargs={"chat_template": TEMPLATE_PATH} if use_custom_template else None,
+            use_new_inference_servers=True,


do we need this? Can we just run all the inference_servers/ tests with the env var?

Can we just run all the inference_servers/ tests with the env var?

inference_servers/ tests are meant to test the new codepath in any case. They are written specifically for the new codepath, so I don't think it makes sense to have any gating with the env var.

it's more like a nit. but either way you'd want to change these tests after this path becomes the default, because this component in particular is shared between the old and new path. So the question is which one is simpler. I think all the inference_servers/ tests should have the env var set automatically so the underlying modules change behavior based on the env var and you don't do anything special at the interfaces.

...kends/skyrl_train/gpu/gpu_ci/inference_servers/test_remote_inference_client_chat_template.py

...backends/skyrl_train/gpu/gpu_ci/inference_servers/test_remote_inference_client_generation.py

...kends/skyrl_train/gpu/gpu_ci/inference_servers/test_remote_inference_client_chat_template.py

...backends/skyrl_train/gpu/gpu_ci/inference_servers/test_remote_inference_client_generation.py

kouroshHakha · 2026-02-25T19:46:27Z

...backends/skyrl_train/gpu/gpu_ci/inference_servers/test_remote_inference_client_generation.py

+        assert not ("error" in output or output.get("object", "") == "error"), f"Error in output: {output}"
+    assert len(outputs) == num_samples
+    for response_data in outputs:
+        if test_type == "litellm":


when do we need litellm test_type?

Please see the new tests. The helper here is from the older tests for inference engine client's http endpoint.

I've ported over the litellm tests as well now, which makes this codepath useful in the new test file

it feels irrelevant to skyrl to test this in general. As long as you test openAI compatibility, isn't litellm compatible with that?

litellm supports additional sampling parameters that vllm supports, and some of our first party integrations like Harbor rely on this. So I believe it is good to have the tests here.

...backends/skyrl_train/gpu/gpu_ci/inference_servers/test_remote_inference_client_generation.py

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

…ngine-test

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

devin-ai-integration

Devin Review found 1 new potential issue.

View 8 additional findings in Devin Review.

skyrl/backends/skyrl_train/inference_servers/remote_inference_client.py

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

SumanthRH · 2026-02-26T01:23:54Z

I've now refactored the test file test_new_inference_generation.py into two groups of tests:

Group A: Generation and error handling tests that interact directly with the router's OpenAI-compatible endpoints via requests or LiteLLM.

We use Litellm for passing some sampling parameters that are not supported by OpenAI's chat completion client.

Group B: Generation and error handling tests that use the RemoteInferenceClient.

Both of these are important right now.

Once we make progress in the migration #845 , we can remove the generation APIs in the remote inference client and have Generators interact directly with the router's HTTP endpoints. I've added a TODO for this case.

CharlieFRuan

Some minor comments / questions. Thank you Sumanth!

skyrl/backends/skyrl_train/inference_servers/utils.py

CharlieFRuan · 2026-02-26T02:04:59Z

tests/backends/skyrl_train/gpu/gpu_ci/inference_servers/test_new_inference_generation.py

+uv run --isolated --extra dev --extra fsdp pytest tests/backends/skyrl_train/gpu/gpu_ci/inference_servers/test_new_inference_generation.py -m vllm -v
+"""
+
+# TODO (sumanthrh) (RemoteInferenceClient data-plane-deprecation): Remove the tests in Group B once we migrate all generation interactions to the router's HTTP API.


Does this mean SkyRLGymGenerator would post HTTP requests as well?

Yes that is correct! This would make things more natural for users to also bring in any generators

CharlieFRuan · 2026-02-26T02:07:32Z

tests/backends/skyrl_train/gpu/gpu_ci/inference_servers/test_new_inference_generation.py

+
+
+@pytest.mark.vllm
+def test_error_handling(vllm_server: InferenceEngineState):


Comparing agaainst the test_http_endpoint_error_handling in tests/backends/skyrl_train/gpu/gpu_ci/test_inference_engine_client_http_endpoint.py:

We guard stream==False in the old path. Do we / should we support streaming in the new codepath?

There's no reason why streaming shouldn't work for the router's HTTP endpoint. Let me add a test for this to be sure!

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

SumanthRH added 10 commits February 24, 2026 23:17

Add E2E tests for remote inference client

268b325

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

x

056ca27

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

remove changes

33f901c

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

Merge remote-tracking branch 'upstream/main' into migrate-inference-e…

f884dda

…ngine-test

x

a177fec

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

x

9ba51b6

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

add new chat template tests

4c70a3b

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

x

2c8ee02

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

x

3f637d1

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

x

842d494

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

gemini-code-assist bot reviewed Feb 25, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

x

920157b

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

SumanthRH requested review from CharlieFRuan and kouroshHakha February 25, 2026 18:49

kouroshHakha requested changes Feb 25, 2026

View reviewed changes

kouroshHakha reviewed Feb 25, 2026

View reviewed changes

SumanthRH added 2 commits February 25, 2026 22:16

x

74a0c1c

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

Merge remote-tracking branch 'upstream/main' into migrate-inference-e…

a8aeff0

…ngine-test

This comment was marked as resolved.

Sign in to view

SumanthRH added 2 commits February 25, 2026 22:34

x

3d2d02b

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

x

deb61e1

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

devin-ai-integration bot reviewed Feb 25, 2026

View reviewed changes

skyrl/backends/skyrl_train/inference_servers/remote_inference_client.py Show resolved Hide resolved

SumanthRH added 5 commits February 26, 2026 00:33

refactor tests into router level and remote client level

ee4a4e2

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

comments

b8a5803

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

x

6cbdabe

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

x

d41803b

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

migrate chat template test after merge from main

9e1f336

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

SumanthRH requested a review from kouroshHakha February 26, 2026 01:24

CharlieFRuan reviewed Feb 26, 2026

View reviewed changes

SumanthRH added 2 commits February 26, 2026 06:44

add more comprehensive litellm tests for the Harbor integration

972b199

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>

add a note

3d0c985

Signed-off-by: SumanthRH <sumanthrh99@gmail.com>



		@pytest.mark.vllm
		def test_error_handling(vllm_server: InferenceEngineState):

Conversation

SumanthRH commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Changes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SumanthRH Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SumanthRH commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CharlieFRuan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SumanthRH commented Feb 25, 2026 •

edited

Loading

SumanthRH Feb 25, 2026 •

edited

Loading

SumanthRH commented Feb 26, 2026 •

edited

Loading