diff --git a/packages/nemo_evaluator_sdk/examples/gym/README.md b/packages/nemo_evaluator_sdk/examples/gym/README.md index ca04ba40b9..f140d755b1 100644 --- a/packages/nemo_evaluator_sdk/examples/gym/README.md +++ b/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. diff --git a/packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.py b/packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.py index ce623ac894..66183fdd54 100644 --- a/packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.py +++ b/packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.py @@ -28,10 +28,14 @@ means a caller can run a **subset** of tasks without Gym rolling out the rest. **Execution** is the two-step Gym flow (the one that reads a dataset directly -without triggering Gym's split-driven data-prep): ``gym env start`` brings up the -resources-server + agent + model servers, then ``gym eval run --no-serve --input -`` collects rollouts against them. The runtime shells out -to the ``gym`` CLI on PATH, so this SDK never imports ``nemo_gym``. Subprocess +without triggering Gym's split-driven data-prep), preceded by a pre-flight: +``gym env validate`` merges the composed config and reports unset ``???`` values, +bad paths, and dangling cross-references without starting anything; then ``gym env +start`` brings up the resources-server + agent + model servers, and ``gym eval run +--no-serve --input `` collects rollouts against them. Both +commands receive the identical selection arguments, so what is validated is what +runs. The runtime shells out to the ``gym`` CLI on PATH, so this SDK never imports +``nemo_gym``. Subprocess output is streamed to log files under the run's work dir *and* mirrored to this module's logger at ``DEBUG``, so callers choose terminal visibility through ordinary ``logging`` configuration. @@ -95,6 +99,11 @@ #: once, and that path is what the subprocesses run — so a child whose PATH differs from ours cannot #: end up executing a different Gym. _GYM_CLI = "gym" +#: Bound on `gym env validate`. It merges config without starting anything and returns in about a +#: second; this only exists so a wedged invocation cannot stall the run before it begins. +_VALIDATE_TIMEOUT_S = 120.0 +#: Where Gym's Hydra run directories are redirected, relative to the run's work dir. +_HYDRA_SUBDIR = "gym_hydra" #: Gym's index fields on each rollout record. ``_ng_task_index`` is the only join back to the input #: rows that survives a round-trip: Gym mutates ``responses_create_params`` (even the prompt) and #: copies only a fixed allowlist of row keys onto the result, so no field we invent comes back. Gym @@ -107,34 +116,177 @@ _LOG_TAIL_LINES = 40 -#: Substrings that mark a Hydra override key as carrying a credential. Matched case-insensitively -#: against the key half of ``+key=value``. +#: Substrings that mark an override as carrying a credential. Matched case-insensitively against the +#: full dotted path, so nesting cannot hide one behind an innocuous leaf name. _SECRET_KEY_MARKERS = ("api_key", "apikey", "token", "secret", "password", "passwd", "credential") #: Stand-in written in place of a redacted override value. _REDACTED = "" -def _redact_env_overrides(overrides: Sequence[str]) -> list[str]: - """Redact credential-looking values from Hydra overrides before they are recorded as provenance. +#: Dict keys Hydra reads back unchanged. Its ``dictKey`` rule accepts no quoting, so a key is +#: whatever the lexer makes of the bare text — this is deliberately narrower than what parses. +_HYDRA_DICT_KEY = re.compile(r"[A-Za-z_][A-Za-z0-9_.-]*\Z") +#: Bare words the lexer types rather than reading as text, so they cannot serve as string keys. +_HYDRA_KEY_LITERALS = frozenset({"true", "false", "null", "inf", "nan"}) - ``env_overrides`` is a free-form escape hatch forwarded verbatim to ``gym env start``, so nothing - stops a caller passing ``+model.api_key=sk-...``. ``RunnerInfo.config`` is persisted into the run - bundle, so a value that looks like a credential must not be written there. + +def _hydra_dict(value: Mapping[str, Any]) -> str: + """Render a mapping as a Hydra dict container, ``{key:value,...}``. + + Reached for a mapping nested inside a container — ``[{"b": 1}]`` — where there is no dotted path + to flatten onto, so the dict has to be spelled inline. Values recurse, so the typed spellings + below hold at any depth. + + Keys are emitted bare, because Hydra's ``dictKey`` rule has no quoted form: ``{'b':1}`` does not + parse at all. That leaves the key at the mercy of the lexer, which types it — ``{true:1}`` keys + on the boolean ``True``, ``{1.5:1}`` on a float — and rejects ``:``, ``,``, brackets, and quotes + outright. Anything outside the conservative shape above therefore raises here rather than + silently keying the config on something the caller did not write. + """ + rendered = [] + for key, item in value.items(): + if not isinstance(key, str) or not _HYDRA_DICT_KEY.match(key) or key.casefold() in _HYDRA_KEY_LITERALS: + raise ValueError( + f"Gym config override has dict key {key!r}, which Hydra's override grammar cannot " + "express as a string: keys are unquoted, so only a leading letter or underscore " + "followed by letters, digits, '_', '.', or '-' survives the round trip. Set this " + "key through the override path instead of nesting it inside a list." + ) + rendered.append(f"{key}:{_hydra_scalar(item)}") + return "{" + ",".join(rendered) + "}" + + +def _hydra_scalar(value: Any) -> str: + """Render a leaf value the way Hydra's override grammar reads it back. + + Hydra's grammar is typed, so an unquoted string is not necessarily a string: ``true`` parses as a + boolean, ``null`` as ``None``, ``1.5`` as a float, ``a,b`` as a *sweep*, and ``A[B`` fails to + parse outright. Strings are therefore always single-quoted, which round-trips every one of those + (verified against ``hydra.core.override_parser``). Interpolations survive quoting — the override + sets the literal text and OmegaConf resolves it on read — so ``${policy_base_url}`` still works. + + Only ``'`` is escaped. Hydra does **not** decode ``\\\\`` inside a quoted value: escaping + backslashes doubles them, so they are passed through raw. + + ``None`` and booleans get their own spellings, since ``str()`` would emit ``"None"``/``"True"`` + and Hydra reads those back as text. Containers recurse for the same reason: ``str()`` on a dict + emits Python's repr, whose quoted keys Hydra rejects outright. + """ + if value is None: + return "null" + if isinstance(value, bool): + return "true" if value else "false" + if isinstance(value, Mapping): + return _hydra_dict(value) + if isinstance(value, (list, tuple)): + return "[" + ",".join(_hydra_scalar(item) for item in value) + "]" + if isinstance(value, str): + # A trailing backslash would escape the closing quote and leave the value unterminated, and + # there is no spelling that avoids it — better to say so than to emit something unparseable. + if value.endswith("\\"): + raise ValueError( + f"Gym config override value {value!r} ends with a backslash, which Hydra's override " + "grammar cannot express: it escapes the closing quote." + ) + return "'" + value.replace("'", "\\'") + "'" + return str(value) + + +def _flatten_overrides(overrides: Mapping[str, Any], _prefix: str = "") -> list[str]: + """Flatten a nested override mapping into Hydra ``++dotted.path=value`` arguments. + + Callers describe overrides as structured data — ``{"a": {"b": 1}}`` — rather than as + pre-serialized Hydra strings, so the config survives being sent somewhere as JSON. Hydra itself + only speaks the flat form, so the translation happens here, at the point of invocation. + + ``++`` rather than ``+``: it sets a key whether or not it already exists, which is what an + override means. A bare ``+`` fails on a key the merged config already defines. + """ + arguments: list[str] = [] + for key, value in overrides.items(): + path = f"{_prefix}{key}" + # An empty mapping has no leaves to descend to, so recursing would drop the override + # entirely. It is still a value the caller asked to set: emit it as ``++path={}``, which + # clears the subtree. + if isinstance(value, Mapping) and value: + arguments.extend(_flatten_overrides(value, f"{path}.")) + else: + arguments.append(f"++{path}={_hydra_scalar(value)}") + return arguments + + +def _redact_env_overrides(overrides: Mapping[str, Any], _prefix: str = "") -> dict[str, Any]: + """Redact credential-looking values from overrides before they are recorded as provenance. + + ``env_overrides`` is a free-form escape hatch forwarded to Gym, so nothing stops a caller passing + ``{"model": {"api_key": "sk-..."}}``. ``RunnerInfo.config`` is persisted into the run bundle, so a + value that looks like a credential must not be written there. The *key* is always kept — knowing that a run overrode ``model.api_key`` is useful provenance; - knowing the value is a leak. Overrides that don't parse as ``key=value`` are kept verbatim: they - carry no value to leak. + knowing the value is a leak. Matching is on the full dotted path, so a marker anywhere in it + redacts, and nesting cannot hide a credential behind an innocuous leaf name. + + Lists are walked too, since a mapping inside one — ``{"models": [{"api_key": "sk-..."}]}`` — + reaches Gym just as a nested mapping does. The index contributes no path segment: what marks a + value as a credential is the key it sits under, not where in a list it happens to fall. """ - redacted: list[str] = [] - for override in overrides: - key, sep, _ = override.partition("=") - if sep and any(marker in key.casefold() for marker in _SECRET_KEY_MARKERS): - redacted.append(f"{key}={_REDACTED}") + redacted: dict[str, Any] = {} + for key, value in overrides.items(): + path = f"{_prefix}{key}" + if isinstance(value, Mapping): + redacted[key] = _redact_env_overrides(value, f"{path}.") + elif any(marker in path.casefold() for marker in _SECRET_KEY_MARKERS): + redacted[key] = _REDACTED + elif isinstance(value, (list, tuple)): + redacted[key] = [_redact_list_item(item, path) for item in value] else: - redacted.append(override) + redacted[key] = value return redacted +def _redact_list_item(item: Any, path: str) -> Any: + """Redact inside one element of a list-valued override. See :func:`_redact_env_overrides`.""" + if isinstance(item, Mapping): + return _redact_env_overrides(item, f"{path}.") + if isinstance(item, (list, tuple)): + return [_redact_list_item(nested, path) for nested in item] + return item + + +def _selection_args(config: GymRuntimeConfig, work_dir: Path) -> list[str]: + """The environment/agent/model selection passed to Gym. + + Built once and handed verbatim to both ``gym env validate`` and ``gym env start``, so what is + validated is exactly what runs — a pre-flight against a different config would be worse than + none. + """ + selection = [ + "--config", + config.agent_config, + "--model-type", + config.model_type, + "--resources-server", + config.resources_server, + ] + if config.bind_resources_server: + # Composable (Pattern-A) agents leave resources_server.name unbound ('???'); bind it to the + # env we're running. Assumes the agent config's top-level key equals the agent name (the + # simple_agent convention) *and* that the resources-server is registered under the + # environment's own name — not universally true, so self-contained or differently-named + # servers set bind_resources_server=False and bind themselves via env_overrides. + selection.append( + f"+{config.agent}.responses_api_agents.{config.agent}.resources_server.name={config.resources_server}" + ) + selection.extend(_flatten_overrides(config.env_overrides)) + # Gym is a Hydra app, so each invocation writes a timestamped run directory — by default + # `outputs//