NVIDIA-NeMo / NVIDIA-NeMo/RL

test: prefetch config validation cannot cover LocalVLLMModelConfig in CI

Open
#3,806 0 comments 0 reactions 1 assignee Claimed by @terrykong View on GitHub
Automation bug
Dominant language
Python
Stars
2k
Forks
561
Avg merge
4d 5h
Merged PRs (30d)
145

Description

`tests/unit/environments/test_nemo_gym_prefetch_configs.py` validates every standalone model-server block in `examples/nemo_gym/prefetch_*.yaml` against Gym's real pydantic classes, so schema drift fails at PR time instead of in the rl-gym image build. It covers `VLLMModelConfig` but **cannot** cover `LocalVLLMModelConfig`, and that gap is permanent in CI rather than incidental.

**Why it skips.** Gym's `responses_api_models/local_vllm_model/app.py` imports `vllm.entrypoints.openai.api_server` at module scope, so `LocalVLLMModelConfig` is unimportable without vllm. `tests/unit/L0_Unit_Tests_Nemo_Gym.sh:20` is the only shard collecting `nemo_gym`-marked tests and runs `uv run --extra nemo_gym`; `vllm` is a separate extra. So both `prefetch_local_vllm_model` cases skip on every CI run — the `2 skipped` in the test's own output.

**What's unguarded.** `vllm_serve_env_vars` is required only by `LocalVLLMModelConfig`. It was one of the three missing fields fixed in #3785; the other two (`return_token_id_information`, `uses_reasoning_parser`) live on `VLLMModelConfig` and *are* guarded. Deleting `vllm_serve_env_vars` today leaves CI green at `7 passed, 2 skipped` and breaks the gym image build later — the exact failure mode the test exists to prevent.

**Why the obvious fix is wrong.** Adding `--extra vllm` to the shard would not error: the `conflicts` table in `pyproject.toml` pairs nemo-rl extras against *nemo-gym's* `vllm` extra, not against nemo-rl's own. It would silently install nemo-rl's vLLM 0.25.1 rather than the 0.24.0 Gym pins for this server, so the guard would validate against the wrong version — worse than a visible skip.

**Option that does work.** `LocalVLLMModelConfig`'s field annotations reference no vllm type; `app.py` imports only four *functions* (`FlexibleArgumentParser`, `cli_env_setup`, `make_arg_parser`, `validate_parsed_serve_args`). Stubbing those in `sys.modules` allows import and validation without vllm. Verified in an env where `importlib.util.find_spec("vllm") is None`:

```
imported OK: LocalVLLMModelConfig
required fields: [..., 'return_token_id_information', 'uses_reasoning_parser',
'vllm_serve_env_vars', 'vllm_serve_kwargs']
GREEN: full block validates
RED: dropping vllm_serve_env_vars -> ValidationError mentions vllm_serve_env_vars: True
```

Not taken in #3785 because it monkeypatches a Gym internal: if upstream moves a vllm type into those annotations, the stub keeps the test passing instead of skipping, trading a visible skip for a possibly invisible false green.

**Possible resolutions**

1. Accept the gap; this issue records it (status quo).
2. Land the `sys.modules` stub, with a guard asserting the stub only covers the `api_server` import.
3. Ask Gym to move the `vllm` import inside the functions that need it, making `LocalVLLMModelConfig` importable without the dependency — the clean upstream fix.

Found in review of #3785 by @yuki-97.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.