test: prefetch config validation cannot cover LocalVLLMModelConfig in CI
- 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
Assessment
This issue has not been assessed yet.