feat(evaluator): pre-flight Gym config with gym env validate and take overrides as a dict

packages/nemo_evaluator_sdk/examples/gym/README.md

@@ -39,7 +39,7 @@ Working from a Gym checkout also makes its components take precedence over the p
 
 Useful flags: `--resources-server`, `--agent`, `--model-type` (`inference_provider` for OpenAI-compatible **chat** endpoints; `openai_model` uses the OpenAI **Responses API** and 500s against chat-only endpoints), `--num-repeats`, `--dataset`, `--output-dir`.
 
-For the full set of knobs the underlying `gym env start` / `gym eval run` commands accept, see the [NeMo Gym documentation](https://github.com/NVIDIA-NeMo/Gym). Anything `GymRuntimeConfig` does not expose as a field can be passed through with its `env_overrides` escape hatch (Hydra `+key=value` overrides applied to `gym env start`).
+For the full set of knobs the underlying `gym env start` / `gym eval run` commands accept, see the [NeMo Gym documentation](https://github.com/NVIDIA-NeMo/Gym). Anything `GymRuntimeConfig` does not expose as a field can be passed through with its `env_overrides` escape hatch — nested data such as `{"model": {"temperature": 0.7}}`, flattened to Hydra's override grammar and applied to `gym env start`.
 
 Each run writes its bundle to a fresh temporary directory by default. Pass `--output-dir` to choose one, but give every run its own: the runner refuses to reuse a directory that already holds Gym rollout output (Gym appends to its failures sidecar, so reusing one would mix runs) and raises rather than clearing a prior run's results.
 

packages/nemo_evaluator_sdk/tests/agent_eval/test_gym_runtime.py

@@ -14,6 +14,7 @@
 import asyncio
 import json
 import logging
+import shutil
 from collections import Counter, deque
 from pathlib import Path
 
@@ -22,17 +23,23 @@
     _LOG_TAIL_LINES,
     NG_ROLLOUT_INDEX,
     NG_TASK_INDEX,
+    GymAgentTaskRunner,
     GymRewardMetric,
+    GymRuntimeConfig,
     _aggregate_metrics_path_for,
     _canonical_row_hash,
     _content_text,
     _drain_pumps,
     _ensure_fresh_output,
+    _flatten_overrides,
+    _gym_executable,
+    _hydra_scalar,
     _materialize_dataset,
     _pump_stream,
     _read_run_aggregations,
     _render_instruction,
     _require_full_coverage,
+    _selection_args,
     _source_datasets,
     _trials_from_rollouts,
     discover_gym_tasks,
@@ -578,3 +585,162 @@ def test_aggregate_metrics_sidecar_with_invalid_utf8_is_skipped_not_raised(tmp_p
     _aggregate_metrics_path_for(rollouts_path).write_bytes(b'[{"agent_ref": {"name": "a"}, "x": \xff}]')
 
     assert _read_run_aggregations(rollouts_path) is None
+
+
+# ---------------------------------------------------------------------------
+# Config pre-flight: override serialization, selection args, `gym env validate`
+# ---------------------------------------------------------------------------
+
+
+def test_hydra_scalars_use_hydra_spellings_not_python_ones() -> None:
+    # `str(None)` is "None" and `str(True)` is "True" — both of which Hydra reads back as *strings*,
+    # silently setting the literal text instead of a null or a boolean.
+    assert _hydra_scalar(None) == "null"
+    assert _hydra_scalar(True) == "true"
+    assert _hydra_scalar(False) == "false"
+    assert _hydra_scalar([1, None]) == "[1,null]"
+    assert _hydra_scalar(0.7) == "0.7"
+
+
+def test_hydra_strings_are_quoted_so_the_grammar_cannot_retype_them() -> None:
+    # Hydra's grammar is typed: unquoted, "true" parses as a bool, "null" as None, "1.5" as a float,
+    # and "a,b" as a *sweep* — so a string override would silently become something else.
+    assert _hydra_scalar("true") == "'true'"
+    assert _hydra_scalar("null") == "'null'"
+    assert _hydra_scalar("1.5") == "'1.5'"
+    assert _hydra_scalar("a,b") == "'a,b'"
+    assert _hydra_scalar("A[B") == "'A[B'"  # unquoted this does not parse at all
+    assert _hydra_scalar("") == "''"
+    # Only the quote is escaped: Hydra does not decode `\\` inside a quoted value, so escaping
+    # backslashes would double them.
+    assert _hydra_scalar("he'llo") == "'he\\'llo'"
+    assert _hydra_scalar("back\\slash") == "'back\\slash'"
+    # Interpolation survives quoting — the override sets the text, OmegaConf resolves it on read.
+    assert _hydra_scalar("${policy_base_url}") == "'${policy_base_url}'"
+
+
+def test_hydra_rejects_a_value_it_cannot_express() -> None:
+    # A trailing backslash escapes the closing quote and leaves the value unterminated; there is no
+    # spelling that avoids it, so say so rather than emit something unparseable.
+    with pytest.raises(ValueError, match="ends with a backslash"):
+        _hydra_scalar("ends\\")
+
+
+def test_hydra_dicts_nested_in_a_list_use_the_grammars_bare_keys() -> None:
+    # A mapping reached through a list has no dotted path to flatten onto, so it has to be spelled
+    # inline. `str()` would emit Python's repr — `{'b': 1}` — whose quoted keys Hydra's `dictKey`
+    # rule has no form for and rejects outright.
+    assert _hydra_scalar({"b": 1}) == "{b:1}"
+    assert _hydra_scalar([{"b": 1}, {"c": "true"}]) == "[{b:1},{c:'true'}]"
+    # The typed spellings hold at depth: values recurse through the same function.
+    assert _hydra_scalar({"b": None, "c": True, "d": "a,b"}) == "{b:null,c:true,d:'a,b'}"
+    assert _hydra_scalar({"b": {"c": [1, "x"]}}) == "{b:{c:[1,'x']}}"
+    assert _hydra_scalar({}) == "{}"
+
+
+@pytest.mark.parametrize("key", ["true", "NULL", "1.5", "inf", "b:c", "b,c", "b}c", "b'c", "", 1])
+def test_hydra_rejects_dict_keys_it_would_retype_or_fail_to_parse(key: object) -> None:
+    # Keys are emitted bare because the grammar has no quoted form, which leaves them at the mercy
+    # of the lexer: `{true:1}` keys on the boolean True and `{b:c:1}` does not parse. Neither is what
+    # the caller wrote, and the second is not even loud, so refuse both.
+    with pytest.raises(ValueError, match="dict key"):
+        _hydra_scalar([{key: 1}])
+
+
+def test_flatten_overrides_produces_forcing_dotted_paths() -> None:
+    flattened = _flatten_overrides({"a": {"b": {"c": 1}}, "d": "x"})
+    # `++` not `+`: a bare `+` fails on a key the merged config already defines, which is precisely
+    # the case an override exists for.
+    assert flattened == ["++a.b.c=1", "++d='x'"]
+    assert _flatten_overrides({}) == []
+
+
+def test_flatten_overrides_keeps_an_empty_mapping_instead_of_dropping_it() -> None:
+    # An empty mapping has no leaves to descend to, so recursing emits nothing and the override
+    # vanishes — the caller asked to clear `a` and the run would silently keep the config's value.
+    assert _flatten_overrides({"a": {}}) == ["++a={}"]
+
+
+def test_flatten_overrides_serializes_a_list_of_dicts() -> None:
+    assert _flatten_overrides({"a": {"b": [{"c": 1}]}}) == ["++a.b=[{c:1}]"]
+
+
+def _config(**kwargs: object) -> GymRuntimeConfig:
+    return GymRuntimeConfig(agent="simple_agent", agent_config="cfg.yaml", resources_server="mcqa", **kwargs)  # type: ignore[arg-type]
+
+
+def test_selection_binds_the_resources_server_by_default(tmp_path: Path) -> None:
+    selection = _selection_args(_config(), tmp_path)
+    assert "--resources-server" in selection and "mcqa" in selection
+    assert "+simple_agent.responses_api_agents.simple_agent.resources_server.name=mcqa" in selection
+
+
+def test_selection_omits_the_binding_when_the_caller_binds_it_themselves(tmp_path: Path) -> None:
+    # gdpval registers its server as `gdpval_resources_server`, so the automatic binding is wrong for
+    # it and the caller supplies their own. Emitting both would leave the config ambiguous.
+    selection = _selection_args(
+        _config(
+            bind_resources_server=False,
+            env_overrides={
+                "simple_agent": {"responses_api_agents": {"simple_agent": {"resources_server": {"name": "other"}}}}
+            },
+        ),
+        tmp_path,
+    )
+    assert not [arg for arg in selection if arg.startswith("+simple_agent.")]
+    assert "++simple_agent.responses_api_agents.simple_agent.resources_server.name='other'" in selection
+
+
+def test_selection_redirects_hydra_output_under_the_run_work_dir(tmp_path: Path) -> None:
+    # Gym writes `outputs/<date>/<time>/` relative to cwd, and the subprocesses inherit ours so Gym
+    # can find env.yaml — so without this every run litters the caller's directory.
+    selection = _selection_args(_config(), tmp_path)
+    assert f"hydra.run.dir={tmp_path / 'gym_hydra'}" in selection
+
+
+def _stub_gym(tmp_path: Path, *, exit_code: int, message: str) -> str:
+    """A stand-in for the `gym` CLI that records its argv and exits how the test wants."""
+    script = tmp_path / "stub-gym"
+    script.write_text(
+        "#!/usr/bin/env python3\n"
+        "import pathlib, sys\n"
+        f"pathlib.Path({str(tmp_path / 'argv.txt')!r}).write_text('\\n'.join(sys.argv[1:]))\n"
+        f"print({message!r})\n"
+        f"sys.exit({exit_code})\n",
+        encoding="utf-8",
+    )
+    script.chmod(0o755)
+    return str(script)
+
+
+@pytest.mark.asyncio
+async def test_validate_config_passes_the_selection_and_logs_the_report(tmp_path: Path) -> None:
+    runner = GymAgentTaskRunner(config=_config())
+    gym = _stub_gym(tmp_path, exit_code=0, message="Config is valid.")
+
+    await runner._validate_config(gym, ["--resources-server", "mcqa"], {}, tmp_path)
+
+    assert (tmp_path / "argv.txt").read_text().splitlines() == ["env", "validate", "--resources-server", "mcqa"]
+    # Kept next to the run's other logs so a passing pre-flight is still auditable afterwards.
+    assert (tmp_path / "gym_validate.log").read_text().strip() == "Config is valid."
+
+
+@pytest.mark.asyncio
+async def test_validate_config_raises_with_gyms_own_report(tmp_path: Path) -> None:
+    # The whole point of the pre-flight: surface Gym's diagnosis before a Ray cluster and several
+    # uvicorn servers start, rather than as a readiness timeout that says nothing about the cause.
+    complaint = "Error: references resources_servers/'gdpval', which is not defined"
+    runner = GymAgentTaskRunner(config=_config())
+    gym = _stub_gym(tmp_path, exit_code=1, message=complaint)
+
+    with pytest.raises(RuntimeError) as excinfo:
+        await runner._validate_config(gym, [], {}, tmp_path)
+
+    assert complaint in str(excinfo.value)
+    assert "mcqa" in str(excinfo.value)
+
+
+def test_gym_executable_reports_how_to_install_when_absent(monkeypatch: pytest.MonkeyPatch) -> None:
+    monkeypatch.setattr(shutil, "which", lambda _: None)
+    with pytest.raises(RuntimeError, match="own environment"):
+        _gym_executable()

packages/nemo_evaluator_sdk/tests/agent_eval/test_run_metadata.py

@@ -5,6 +5,7 @@
 
 from __future__ import annotations
 
+import json
 from typing import cast
 
 from nemo_evaluator_sdk.agent_eval.evaluator import _describe_target
@@ -195,24 +196,35 @@ def test_gym_redacts_credential_looking_env_overrides() -> None:
             agent="a",
             agent_config="c",
             resources_server="r",
-            env_overrides=[
-                "+model.api_key=sk-should-not-be-recorded",
-                "+env.HF_TOKEN=hf_should-not-be-recorded",
-                "+agent.temperature=0.7",
-                "--flagged-without-a-value",
-            ],
+            env_overrides={
+                "model": {"api_key": "sk-should-not-be-recorded", "temperature": 0.7},
+                "env": {"HF_TOKEN": "hf_should-not-be-recorded"},
+                "agent": {"nested": {"secret": "deep-should-not-be-recorded"}},
+                "models": [
+                    {"name": "m", "api_key": "sk-in-a-list-should-not-be-recorded"},
+                ],
+                "api_keys": ["sk-whole-list-should-not-be-recorded"],
+            },
         )
     )
 
     recorded = runner.runner_info().config["env_overrides"]
 
-    assert recorded == [
-        "+model.api_key=<redacted>",
-        "+env.HF_TOKEN=<redacted>",
-        "+agent.temperature=0.7",  # not credential-shaped, kept verbatim for reproducibility
-        "--flagged-without-a-value",  # no value to leak
-    ]
-    assert not any("sk-" in entry or "hf_" in entry for entry in recorded)
+    assert recorded == {
+        "model": {
+            "api_key": "<redacted>",
+            "temperature": 0.7,  # not credential-shaped, kept verbatim for reproducibility
+        },
+        "env": {"HF_TOKEN": "<redacted>"},
+        # Matching is on the full dotted path, so a credential stays redacted at any depth.
+        "agent": {"nested": {"secret": "<redacted>"}},
+        # A mapping inside a list reaches Gym just as a nested one does, so it is walked too. The
+        # index contributes no path segment: the key marks the credential, not the position.
+        "models": [{"name": "m", "api_key": "<redacted>"}],
+        # A credential-shaped key wins over descending into it — the whole list goes.
+        "api_keys": "<redacted>",
+    }
+    assert "should-not-be-recorded" not in json.dumps(recorded)
 
 
 def test_model_provenance_records_the_endpoint_and_invocation_params() -> None:

plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.py

@@ -173,10 +173,11 @@ class GymRunnerTarget(BaseModel):
         description="Auto-bind the agent's `resources_server.name` via a Hydra override. Set False for "
         "self-contained agents that already bind their own resources-server.",
     )
-    env_overrides: list[str] = Field(
-        default_factory=list,
-        description="Extra Hydra '+key=value' overrides for `gym env start` (applied after the auto-derived "
-        "resources-server binding).",
+    env_overrides: dict[str, Any] = Field(
+        default_factory=dict,
+        description="Extra config overrides for `gym env start`, as nested data — {'model': {'temperature': 0.7}} "
+        "rather than pre-serialized Hydra strings — so a spec survives being sent as JSON. Flattened to "
+        "Hydra's grammar at invocation, after the auto-derived resources-server binding.",
     )
     num_repeats: int = Field(default=1, ge=1, description="Attempts per row; each attempt becomes one trial.")
     concurrency: int = Field(

plugins/nemo-evaluator/tests/test_agent_evaluate.py

@@ -314,13 +314,17 @@ def test_resolve_target_builds_gym_runtime_from_runner_target(tmp_path: Path) ->
         num_repeats=2,
         concurrency=4,
         reward_key="score",
+        env_overrides={"model": {"temperature": 0.7}},
     )
     target, prompt_template, params = AgentEvalJob._resolve_target(gym_target, ctx)
     assert isinstance(target, GymAgentTaskRunner)
     assert target._config.agent == "simple_agent"
     assert target._config.resources_server == "mcqa"
     assert target._config.num_repeats == 2
     assert target._config.reward_key == "score"
+    # Overrides are nested data on both sides of the seam — the spec model and the runtime config
+    # must agree on the shape, or the spec validates and the runtime rejects it.
+    assert target._config.env_overrides == {"model": {"temperature": 0.7}}
     # A runner shapes its own request, so it contributes no prompt template or inference params.
     assert prompt_template is None
     assert params is None