Skip to content

Commit be03c9b

Browse files
committed
refactor(cli): address review on gym env validate (docs, coverage, real tests)
@cmunley1 review: - Examples now lead with `gym env validate --environment <env>` / `--benchmark <benchmark>`; reword so the model config is clearly optional (NO_MODEL injects a dummy policy_model). - Remove the `# pragma: no cover` from validate() — it is unit-tested. - Strengthen TestValidate to run REAL validation (valid config -> ok; unknown cross-ref -> clean exit 1) instead of mocking the parser; keep one fast decorator-path unit test. The real test surfaced that ServerRefNotFoundError (a cross-ref error) was a plain ValueError, so it escaped exit_cleanly_on_config_error and dumped a traceback. Fold it into the ConfigError family so cross-reference errors are reported cleanly by validate (and gym env start). Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
1 parent 14f76f8 commit be03c9b

3 files changed

Lines changed: 46 additions & 14 deletions

File tree

nemo_gym/cli/env.py

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -865,22 +865,26 @@ def dump_config(): # pragma: no cover
865865

866866

867867
@exit_cleanly_on_config_error
868-
def validate(): # pragma: no cover
868+
def validate():
869869
"""Validate a config without starting Ray or any server subprocess.
870870
871871
Runs the full config parse — config_paths resolution (missing/malformed), server cross-reference
872872
validation, mandatory `???` values, and schema — then exits 0 (valid) or, via
873873
`exit_cleanly_on_config_error`, 1 with a clean traceback-free message. No Ray, no servers, so it
874874
returns in well under a second instead of after Ray bootstrap.
875875
876-
Model *values* aren't required: like `gym list`/`env compose`, a dummy `policy_model` is injected
877-
so model interpolations (e.g. `${policy_base_url}`) resolve — validation is about config
878-
well-formedness; the real model is supplied by the `--model*` flags at run time.
876+
No model config is required: a dummy `policy_model` is injected (the `NO_MODEL` parser config, as
877+
in `gym list` / `env compose`) so model interpolations (e.g. `${policy_base_url}`) resolve —
878+
validation is about config well-formedness, not the model. Pass a model config / `--model-type`
879+
as well if you want it validated too.
879880
880881
Examples:
881882
882883
```bash
883-
gym env validate --config resources_servers/<env>/configs/<env>.yaml --config responses_api_models/<model>/configs/<model>.yaml
884+
gym env validate --environment <env>
885+
gym env validate --benchmark <benchmark>
886+
# or by explicit config path(s):
887+
gym env validate --config resources_servers/<env>/configs/<env>.yaml
884888
```
885889
"""
886890
global_config_dict = get_global_config_dict(

nemo_gym/config_types.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -137,10 +137,6 @@ def is_server_ref(config_dict: DictConfig) -> Optional[ServerRef]:
137137
return None
138138

139139

140-
class ServerRefNotFoundError(ValueError):
141-
"""A server cross-reference points to an instance that is not defined in the merged config."""
142-
143-
144140
class ConfigError(Exception):
145141
"""Base for user-facing configuration errors.
146142
@@ -167,6 +163,10 @@ class ConfigMissingValuesError(ConfigError, ValueError):
167163
"""One or more required config values are still unset (OmegaConf '???') after merging."""
168164

169165

166+
class ServerRefNotFoundError(ConfigError, ValueError):
167+
"""A server cross-reference points to an instance that is not defined in the merged config."""
168+
169+
170170
########################################
171171
# Dataset configs for handling and upload/download
172172
########################################

tests/unit_tests/test_cli.py

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
# limitations under the License.
1515
import json
1616
import shutil
17+
import sys
1718
import tomllib
1819
from importlib import import_module
1920
from pathlib import Path
@@ -292,15 +293,42 @@ def test_config_error_base_catches_subclasses(self) -> None:
292293

293294

294295
class TestValidate:
295-
def test_valid_config_prints_ok(self, monkeypatch: MonkeyPatch, capsys) -> None:
296-
monkeypatch.setattr(nemo_gym.cli.env, "get_global_config_dict", lambda **k: OmegaConf.create({}))
297-
296+
def _validate_config(self, monkeypatch: MonkeyPatch, tmp_path: Path, config_yaml: str) -> None:
297+
"""Run the real `validate()` against a config file (no mocking of the parse path)."""
298+
config_file = tmp_path / "config.yaml"
299+
config_file.write_text(config_yaml)
300+
# chdir to a clean dir so a repo-local env.yaml isn't picked up; clear the parse cache.
301+
monkeypatch.chdir(tmp_path)
302+
monkeypatch.setattr(sys, "argv", ["gym", f"+config_paths=[{config_file}]"])
303+
monkeypatch.setattr(nemo_gym.global_config, "_GLOBAL_CONFIG_DICT", None)
298304
validate()
299305

306+
def test_valid_config_passes(self, monkeypatch: MonkeyPatch, tmp_path, capsys) -> None:
307+
# A well-formed config (real parse, real checks) -> "valid".
308+
self._validate_config(
309+
monkeypatch,
310+
tmp_path,
311+
"my_server:\n resources_servers:\n my_server:\n entrypoint: app.py\n domain: other\n",
312+
)
300313
assert "valid" in capsys.readouterr().out.lower()
301314

302-
def test_config_error_exits_nonzero(self, monkeypatch: MonkeyPatch) -> None:
303-
# A ConfigError raised during the parse is turned into a clean exit(1), no traceback.
315+
def test_unknown_cross_reference_exits_nonzero(self, monkeypatch: MonkeyPatch, tmp_path) -> None:
316+
# An agent referencing a resources server that isn't defined -> ServerRefNotFoundError ->
317+
# clean exit(1) (real cross-reference validation, not a mock).
318+
bad = (
319+
"my_agent:\n"
320+
" responses_api_agents:\n"
321+
" a:\n"
322+
" entrypoint: app.py\n"
323+
" resources_server:\n type: resources_servers\n name: does_not_exist\n"
324+
" model_server:\n type: responses_api_models\n name: policy_model\n"
325+
)
326+
with raises(SystemExit) as exc_info:
327+
self._validate_config(monkeypatch, tmp_path, bad)
328+
assert exc_info.value.code == 1
329+
330+
def test_config_error_becomes_clean_exit(self, monkeypatch: MonkeyPatch) -> None:
331+
# Fast unit check of the decorator path: any ConfigError -> exit(1), no traceback.
304332
def _raise(**kwargs):
305333
raise NoServerInstancesError("nothing configured to run")
306334

0 commit comments

Comments
 (0)