From 8d186d1eb0849b1d0420df6b4d839bceae78c189 Mon Sep 17 00:00:00 2001 From: Ananth Subramaniam Date: Fri, 2 Oct 2026 05:50:03 -0700 Subject: [PATCH 1/3] feat(config): front agents without an environment server with a legacy relay Rollout collection reaches agents only through an environment server, and config validation has rejected any agent that no environment server names since #3724. That breaks every configuration written before environment servers, including out-of-tree training configs, on upgrade. Instead of failing, generate the `legacy_agent` relay that scripts/add_legacy_agent_environment_servers.py would declare, named `_environment_server`, and log one deprecation warning listing every generated relay. Only agents with no environment server get one, so a generated relay never makes routing ambiguous or rewrites a reference. Setting `error_on_agent_without_environment_server: true` restores the error. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Ananth Subramaniam --- .../pages/troubleshooting/configuration.mdx | 53 +++++++++++ nemo_gym/global_config.py | 57 +++++++++--- tests/unit_tests/test_global_config.py | 87 +++++++++++++++++-- 3 files changed, 180 insertions(+), 17 deletions(-) diff --git a/fern/versions/latest/pages/troubleshooting/configuration.mdx b/fern/versions/latest/pages/troubleshooting/configuration.mdx index 27512a9583b..7855f764bf0 100644 --- a/fern/versions/latest/pages/troubleshooting/configuration.mdx +++ b/fern/versions/latest/pages/troubleshooting/configuration.mdx @@ -172,6 +172,59 @@ Bypassing validation means invalid servers won't start. Use this only for debugg +### Agent Without an Environment Server + +``` +Agents without an environment server are deprecated; generated legacy_agent relays for +`my_agent` -> `my_agent_environment_server`. Declare them in your config +(scripts/add_legacy_agent_environment_servers.py writes them), or set +error_on_agent_without_environment_server=true to make this an error. +``` + +**When**: An agent instance has no environment server naming it in `agent_server`. Rollout +collection reaches agents only through an environment server, so Gym generates a `legacy_agent` +relay in front of the agent and warns. Configs written before environment servers keep running. + +**Fix**: Declare the environment server in your config. The migration script adds one beside every +agent that lacks one: + +```bash +python scripts/add_legacy_agent_environment_servers.py --check # report only +python scripts/add_legacy_agent_environment_servers.py # write the blocks +``` + +For a config outside the repository, add the block by hand: + +```yaml +my_agent_environment_server: + environment_servers: + legacy_agent: + entrypoint: app.py + agent_server: + type: responses_api_agents + name: my_agent +``` + +If you rename an agent with `_inherit_from`, point the existing environment server at the new name +instead of adding a second one. Two environment servers in front of one agent make agent-routed +rows ambiguous. + +#### Require Declared Environment Servers + +To make a missing environment server an error instead (`AgentWithoutEnvironmentServerError`): + +```yaml +# In env.yaml +error_on_agent_without_environment_server: true +``` + +```bash +# Or via command line +gym env start \ + --config config.yaml \ + +error_on_agent_without_environment_server=true +``` + --- ## Connection Errors diff --git a/nemo_gym/global_config.py b/nemo_gym/global_config.py index 0ac944f2b64..d91b5cd1806 100644 --- a/nemo_gym/global_config.py +++ b/nemo_gym/global_config.py @@ -125,6 +125,9 @@ ENVIRONMENT_SERVER_NAME_KEY_NAME = "environment_server_name" ENVIRONMENT_SERVER_ROUTES_KEY_NAME = "environment_server_routes" ENVIRONMENT_ROUTING_MODE_KEY_NAME = "environment_routing_mode" +# When set, an agent that no environment server fronts fails config validation instead of getting a generated +# legacy_agent relay. +ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME = "error_on_agent_without_environment_server" NEMO_GYM_RESERVED_TOP_LEVEL_KEYS = [ CONFIG_PATHS_KEY_NAME, ENTRYPOINT_KEY_NAME, @@ -1067,10 +1070,17 @@ def _carry_over_agent_bindings(original: DictConfig, composed: DictConfig) -> No elif OmegaConf.is_missing(original, key): composed[key] = MISSING - def _raise_on_agent_without_environment_server(self, global_config_dict: DictConfig) -> None: - """Reject an agent that rollout collection would otherwise have to call directly. + def _front_agents_without_environment_server(self, global_config_dict: DictConfig) -> None: + """Give every agent an environment server, since rollout collection reaches agents only through one. - Runs after composition, so every agent left is one a run can dispatch to. + Runs after composition, so every agent left is one a run can dispatch to. An agent that no environment + server names gets a generated `legacy_agent` relay, the same block + scripts/add_legacy_agent_environment_servers.py declares, and one warning lists every generated relay. + This keeps configs written before environment servers running. With + `error_on_agent_without_environment_server` set, such an agent is an error instead. + + Only agents with no environment server are touched, so a generated relay can never make an agent's + routing ambiguous, and an existing reference is never rewritten. """ with_environment_server = set() for instance in global_config_dict.values(): @@ -1084,23 +1094,48 @@ def _raise_on_agent_without_environment_server(self, global_config_dict: DictCon if isinstance(reference, DictConfig): with_environment_server.add(reference.get("name")) - without_environment_server = [ + without_environment_server = sorted( agent.name for agent in self._agent_instances(global_config_dict) if not self._is_unbound_agent(agent.server_config) and agent.name not in with_environment_server and agent.server_config.get("entrypoint") is not None - ] + ) if not without_environment_server: return - listing = "\n".join(f" - {name}" for name in sorted(without_environment_server)) - raise AgentWithoutEnvironmentServerError( - f"""Agent instance(s) have no environment server, so rollout collection cannot reach them: + if global_config_dict.get(ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME, False): + listing = "\n".join(f" - {name}" for name in without_environment_server) + raise AgentWithoutEnvironmentServerError( + f"""Agent instance(s) have no environment server, so rollout collection cannot reach them: {listing} Declare one for each, naming the agent in its `{AGENT_SERVER_REF_KEY_NAME}` reference, or run -scripts/add_legacy_agent_environment_servers.py to update your config.""" +scripts/add_legacy_agent_environment_servers.py to update your config. Unset +{ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME} to generate a legacy_agent relay for each instead.""" + ) + + generated = {} + for agent_name in without_environment_server: + server_name = f"{agent_name}_environment_server" + while server_name in global_config_dict: + server_name = f"{server_name}_environment_server" + with open_dict(global_config_dict): + global_config_dict[server_name] = { + ENVIRONMENT_SERVER_TYPE_KEY_NAME: { + "legacy_agent": { + "entrypoint": "app.py", + AGENT_SERVER_REF_KEY_NAME: {"type": AGENT_SERVER_TYPE_KEY_NAME, "name": agent_name}, + } + } + } + generated[agent_name] = server_name + + relays = ", ".join(f"`{agent}` -> `{server}`" for agent, server in generated.items()) + logger.warning( + f"Agents without an environment server are deprecated; generated legacy_agent relays for {relays}. " + "Declare them in your config (scripts/add_legacy_agent_environment_servers.py writes them), or set " + f"{ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME}=true to make this an error." ) def raise_on_missing_values(self, global_config_dict: DictConfig) -> None: @@ -1311,8 +1346,8 @@ def parse(self, parse_config: Optional[GlobalConfigDictParserConfig] = None) -> # a '???' in a deleted or overwritten branch is not reported. Otherwise the first unset # value surfaces as an opaque MissingMandatoryValue deep in the pipeline. self.raise_on_missing_values(global_config_dict) - # NOTE(martas): this is for catching agents not attached to an environment server - self._raise_on_agent_without_environment_server(global_config_dict) + # NOTE(martas): this is for agents not attached to an environment server; see the method for strict mode + self._front_agents_without_environment_server(global_config_dict) # TODO @bxyu-nvidia: We need a better way of handling dummy model configs with open_dict(global_config_dict): diff --git a/tests/unit_tests/test_global_config.py b/tests/unit_tests/test_global_config.py index 24e43067624..6ad0069c396 100644 --- a/tests/unit_tests/test_global_config.py +++ b/tests/unit_tests/test_global_config.py @@ -56,6 +56,7 @@ get_first_server_config_dict, get_global_config_dict, ) +from nemo_gym.rollout_collection import _environment_servers_by_agent from nemo_gym.secret_utils import recursively_hide_secrets from nemo_gym.server_utils import ( DictConfig, @@ -2083,10 +2084,9 @@ def test_raise_on_no_server_instances_passes_with_a_server(self) -> None: config = DictConfig({"my_server": {"resources_servers": {"x": {"entrypoint": "app.py", "domain": "other"}}}}) parser.raise_on_no_server_instances(config) - def test_config_without_environment_server_is_rejected(self) -> None: - # A pre-migration config would otherwise run, silently dispatching straight to the agent. - parser = GlobalConfigDictParser() - config = DictConfig( + @staticmethod + def _agent_without_environment_server_config(**extra) -> DictConfig: + return DictConfig( { "mcqa": {"resources_servers": {"mcqa": {"entrypoint": "app.py", "domain": "other"}}}, "mcqa_simple_agent": { @@ -2097,11 +2097,35 @@ def test_config_without_environment_server_is_rejected(self) -> None: } } }, + **extra, } ) + + def test_agent_without_environment_server_gets_a_legacy_relay(self, caplog: LogCaptureFixture) -> None: + # A config written before environment servers keeps running: collection reaches the agent through a + # generated relay, never directly, and the user is told what was generated and how to stop relying on it. + config = self._agent_without_environment_server_config() + with caplog.at_level("WARNING"): + GlobalConfigDictParser()._front_agents_without_environment_server(config) + + assert OmegaConf.to_container(config["mcqa_simple_agent_environment_server"]) == { + "environment_servers": { + "legacy_agent": { + "entrypoint": "app.py", + "agent_server": {"type": "responses_api_agents", "name": "mcqa_simple_agent"}, + } + } + } + assert _environment_servers_by_agent(config) == {"mcqa_simple_agent": ["mcqa_simple_agent_environment_server"]} + assert "`mcqa_simple_agent` -> `mcqa_simple_agent_environment_server`" in caplog.text + assert "error_on_agent_without_environment_server=true" in caplog.text + + def test_agent_without_environment_server_is_rejected_when_strict(self) -> None: + config = self._agent_without_environment_server_config(error_on_agent_without_environment_server=True) with raises(AgentWithoutEnvironmentServerError) as exc_info: - parser._raise_on_agent_without_environment_server(config) + GlobalConfigDictParser()._front_agents_without_environment_server(config) assert "mcqa_simple_agent" in str(exc_info.value) + assert "mcqa_simple_agent_environment_server" not in config config["mcqa_environment_server"] = { "environment_servers": { @@ -2111,7 +2135,56 @@ def test_config_without_environment_server_is_rejected(self) -> None: } } } - parser._raise_on_agent_without_environment_server(config) + GlobalConfigDictParser()._front_agents_without_environment_server(config) + + def test_agent_with_environment_server_gets_no_relay(self, caplog: LogCaptureFixture) -> None: + # A second server in front of the same agent would make agent-routed rows ambiguous. + config = self._agent_without_environment_server_config( + mcqa_environment_server={ + "environment_servers": { + "legacy_agent": { + "entrypoint": "app.py", + "agent_server": {"type": "responses_api_agents", "name": "mcqa_simple_agent"}, + } + } + } + ) + with caplog.at_level("WARNING"): + GlobalConfigDictParser()._front_agents_without_environment_server(config) + + assert "mcqa_simple_agent_environment_server" not in config + assert _environment_servers_by_agent(config) == {"mcqa_simple_agent": ["mcqa_environment_server"]} + assert "legacy_agent relays" not in caplog.text + + def test_generated_relay_name_avoids_existing_entries(self) -> None: + config = self._agent_without_environment_server_config( + mcqa_simple_agent_environment_server={"note": "an unrelated top-level entry"} + ) + GlobalConfigDictParser()._front_agents_without_environment_server(config) + + assert config["mcqa_simple_agent_environment_server"] == {"note": "an unrelated top-level entry"} + assert _environment_servers_by_agent(config) == { + "mcqa_simple_agent": ["mcqa_simple_agent_environment_server_environment_server"] + } + + def test_parse_runs_a_config_without_environment_servers(self) -> None: + # End to end through parse(): the generated relay resolves its agent reference and is assigned an address + # like any declared server. + resolved = GlobalConfigDictParser().parse( + GlobalConfigDictParserConfig( + initial_global_config_dict=OmegaConf.merge( + GlobalConfigDictParserConfig.NO_MODEL_GLOBAL_CONFIG_DICT, + self._agent_without_environment_server_config(), + ), + skip_load_from_cli=True, + skip_load_from_dotenv=True, + offline=True, + ) + ) + + relay = resolved["mcqa_simple_agent_environment_server"]["environment_servers"]["legacy_agent"] + assert relay["agent_server"] == {"type": "responses_api_agents", "name": "mcqa_simple_agent"} + assert "host" in relay and "port" in relay @mark.parametrize("resources_server", ["reasoning_gym", "tavily_search"]) def test_langchain_deepagents_configs_have_environment_servers(self, resources_server: str) -> None: @@ -2134,6 +2207,8 @@ def test_langchain_deepagents_configs_have_environment_servers(self, resources_s "search_judge_model_base_url": "http://example.invalid/v1", "search_judge_model_api_key": "test-key", "search_judge_model_name": "test-model", + # A generated relay has the same name, so require the declared one. + "error_on_agent_without_environment_server": True, }, ), skip_load_from_cli=True, From 4f537585f1eb3c2ff249ea48a10f15f9ba18c8d2 Mon Sep 17 00:00:00 2001 From: Ananth Subramaniam Date: Sun, 4 Oct 2026 18:39:13 -0700 Subject: [PATCH 2/3] feat(config): deprecate agents without an environment server and fix the migration script Make the generated legacy_agent relay an explicit deprecation: the warning says a future release will reject these configs, prints the exact block to add for each agent, and explains how to run the migration script or make the error take effect now. Generated relays are named the way the migration script declares them, so the printed block matches what the script writes. Fix scripts/add_legacy_agent_environment_servers.py so users can migrate their own configs: - Accept config files and directories outside the repository; it could only rewrite the repository's own configs. - Skip an agent that any environment server already names. It used to add a second server for such agents (for example the one benchmarks/swebench/pro/hermes.yaml fronts with single_agent_turn_legacy), which makes agent routing ambiguous. - Handle an agent renamed only through `_inherit_from` by inheriting the source agent's server; such renames were skipped. - Report configs that cannot be parsed (exit 2) instead of skipping them. A dangling environment server agent reference, left when an agent is renamed with `_inherit_from`, now points at the script. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Ananth Subramaniam --- .../pages/troubleshooting/configuration.mdx | 42 ++-- nemo_gym/global_config.py | 92 +++++++-- .../add_legacy_agent_environment_servers.py | 183 ++++++++++++++---- ...st_add_legacy_agent_environment_servers.py | 129 ++++++++++++ tests/unit_tests/test_global_config.py | 64 ++++-- 5 files changed, 425 insertions(+), 85 deletions(-) create mode 100644 tests/unit_tests/test_add_legacy_agent_environment_servers.py diff --git a/fern/versions/latest/pages/troubleshooting/configuration.mdx b/fern/versions/latest/pages/troubleshooting/configuration.mdx index 7855f764bf0..722f1d7062a 100644 --- a/fern/versions/latest/pages/troubleshooting/configuration.mdx +++ b/fern/versions/latest/pages/troubleshooting/configuration.mdx @@ -175,39 +175,51 @@ Bypassing validation means invalid servers won't start. Use this only for debugg ### Agent Without an Environment Server ``` -Agents without an environment server are deprecated; generated legacy_agent relays for -`my_agent` -> `my_agent_environment_server`. Declare them in your config -(scripts/add_legacy_agent_environment_servers.py writes them), or set -error_on_agent_without_environment_server=true to make this an error. +DEPRECATED: agents without an environment server: `my_simple_agent`. +Rollout collection reaches an agent only through an environment server. This run generated a +legacy_agent environment server for each agent above, which relays /run to the agent unchanged, so +results are not affected. A future release will reject these configs with +AgentWithoutEnvironmentServerError. ``` **When**: An agent instance has no environment server naming it in `agent_server`. Rollout collection reaches agents only through an environment server, so Gym generates a `legacy_agent` -relay in front of the agent and warns. Configs written before environment servers keep running. +relay in front of the agent and warns. Your config keeps running for now, but **a future release will +make this an error**, so migrate it. -**Fix**: Declare the environment server in your config. The migration script adds one beside every -agent that lacks one: +**Fix**: Declare the environment servers in your config. From a NeMo Gym checkout, run the migration +script on your config files or directories: ```bash -python scripts/add_legacy_agent_environment_servers.py --check # report only -python scripts/add_legacy_agent_environment_servers.py # write the blocks +# Report what would change +python scripts/add_legacy_agent_environment_servers.py path/to/configs/ --check + +# Add the blocks +python scripts/add_legacy_agent_environment_servers.py path/to/configs/ ``` -For a config outside the repository, add the block by hand: +The script appends a block to each config that needs one and leaves the existing content unchanged, +comments included. +It skips agents that an environment server already names, and agents that only override fields of +an agent defined elsewhere. Run without paths, it updates this repository's configs. It exits with +1 under `--check` when something needs adding, and with 2 when a file cannot be read. + +You can also add the block by hand. The warning prints the exact block for each agent, at the same +level as the agents: ```yaml -my_agent_environment_server: +my_environment_server: environment_servers: legacy_agent: entrypoint: app.py agent_server: type: responses_api_agents - name: my_agent + name: my_simple_agent ``` -If you rename an agent with `_inherit_from`, point the existing environment server at the new name -instead of adding a second one. Two environment servers in front of one agent make agent-routed -rows ambiguous. +If you rename an agent with `_inherit_from`, the agent's environment server must be renamed with it, +because `_inherit_from` moves the source. The script writes a server that inherits the source agent's +server. Do not add a second server in front of an agent: it makes agent-routed rows ambiguous. #### Require Declared Environment Servers diff --git a/nemo_gym/global_config.py b/nemo_gym/global_config.py index d91b5cd1806..6e2ff9eaf83 100644 --- a/nemo_gym/global_config.py +++ b/nemo_gym/global_config.py @@ -15,6 +15,7 @@ import logging import re import sys +import textwrap from argparse import ArgumentParser from collections import Counter, defaultdict from copy import deepcopy @@ -587,6 +588,15 @@ def validate_and_populate_defaults( else: available = ", ".join(repr(n) for n in sorted(same_type_names)) or "(none)" hint = f"Available {maybe_server_ref.type}: {available}" + if ( + field_name == AGENT_SERVER_REF_KEY_NAME + and server_instance_config.get_server_ref().type == ENVIRONMENT_SERVER_TYPE_KEY_NAME + ): + hint += ( + "\nIf the agent was renamed with `_inherit_from`, its environment server must be renamed " + "with it: run `python scripts/add_legacy_agent_environment_servers.py ` " + "from a NeMo Gym checkout, or point this server's agent_server.name at the new name." + ) raise ServerRefNotFoundError( f"""In server instance '{server_instance_config.name}', field '{field_name}' references {maybe_server_ref.type}/'{maybe_server_ref.name}', which is not defined in the merged config. {hint}""" @@ -1111,32 +1121,24 @@ def _front_agents_without_environment_server(self, global_config_dict: DictConfi {listing} Declare one for each, naming the agent in its `{AGENT_SERVER_REF_KEY_NAME}` reference, or run -scripts/add_legacy_agent_environment_servers.py to update your config. Unset -{ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME} to generate a legacy_agent relay for each instead.""" +`python scripts/add_legacy_agent_environment_servers.py ` from a NeMo Gym checkout. +Unset {ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME} +to generate a legacy_agent relay for each instead, with a deprecation warning.""" ) + agent_types = {agent.name: agent.agent_type for agent in self._agent_instances(global_config_dict)} generated = {} for agent_name in without_environment_server: - server_name = f"{agent_name}_environment_server" + server_name = legacy_environment_server_name(agent_name, agent_types[agent_name]) + if server_name in global_config_dict or server_name in generated.values(): + server_name = f"{agent_name}{LEGACY_ENVIRONMENT_SERVER_SUFFIX}" while server_name in global_config_dict: - server_name = f"{server_name}_environment_server" + server_name = f"{server_name}{LEGACY_ENVIRONMENT_SERVER_SUFFIX}" with open_dict(global_config_dict): - global_config_dict[server_name] = { - ENVIRONMENT_SERVER_TYPE_KEY_NAME: { - "legacy_agent": { - "entrypoint": "app.py", - AGENT_SERVER_REF_KEY_NAME: {"type": AGENT_SERVER_TYPE_KEY_NAME, "name": agent_name}, - } - } - } + global_config_dict[server_name] = legacy_environment_server_block(agent_name) generated[agent_name] = server_name - relays = ", ".join(f"`{agent}` -> `{server}`" for agent, server in generated.items()) - logger.warning( - f"Agents without an environment server are deprecated; generated legacy_agent relays for {relays}. " - "Declare them in your config (scripts/add_legacy_agent_environment_servers.py writes them), or set " - f"{ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME}=true to make this an error." - ) + logger.warning(agents_without_environment_server_deprecation(generated)) def raise_on_missing_values(self, global_config_dict: DictConfig) -> None: """Fail fast with one actionable error listing every unset '???' value. @@ -1753,6 +1755,60 @@ def model_type_for(global_config_dict: DictConfig, model_server_name: Optional[s return str(next(iter(models))) +LEGACY_ENVIRONMENT_SERVER_SUFFIX = "_environment_server" + + +def legacy_environment_server_name(agent_name: str, agent_type: str) -> str: + """Name an agent's legacy_agent relay after its environment, as the migration script does. + + Stripping the agent type (or a trailing `_agent`) keeps the name stable across agent swaps: + `workplace_assistant_simple_agent` -> `workplace_assistant_environment_server`. + """ + stem = agent_name.removesuffix(f"_{agent_type}").removesuffix(agent_type).rstrip("_") + if stem == agent_name: + stem = agent_name.removesuffix("_agent").rstrip("_") + return f"{stem or agent_type}{LEGACY_ENVIRONMENT_SERVER_SUFFIX}" + + +def legacy_environment_server_block(agent_name: str) -> dict[str, Any]: + """The legacy_agent environment server config that relays to one agent.""" + return { + ENVIRONMENT_SERVER_TYPE_KEY_NAME: { + "legacy_agent": { + "entrypoint": "app.py", + AGENT_SERVER_REF_KEY_NAME: {"type": AGENT_SERVER_TYPE_KEY_NAME, "name": agent_name}, + } + } + } + + +def agents_without_environment_server_deprecation(generated: Mapping[str, str]) -> str: + """Explain the deprecation and how to migrate, with the exact config to add for each agent.""" + blocks = "\n".join( + OmegaConf.to_yaml({server: legacy_environment_server_block(agent)}) for agent, server in generated.items() + ) + agents = ", ".join(f"`{agent}`" for agent in generated) + return f"""DEPRECATED: agents without an environment server: {agents}. +Rollout collection reaches an agent only through an environment server. This run generated a +legacy_agent environment server for each agent above, which relays /run to the agent unchanged, so +results are not affected. A future release will reject these configs with +AgentWithoutEnvironmentServerError. + +To migrate, declare the environment servers in your config. Either: + +1. Run the migration script from a NeMo Gym checkout on your config files or directories: + + python scripts/add_legacy_agent_environment_servers.py path/to/config.yaml [more paths] [--check] + + --check reports what would change without writing. + +2. Or add these blocks yourself, at the same level as the agents: + +{textwrap.indent(blocks, " ")} +To make this an error now, set {ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME}: true in your config or pass ++{ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME}=true.""" + + def pairing_override_enabled(global_config_dict: DictConfig) -> bool: """True when a declared agent/model compatibility guard has been explicitly waived.""" return bool(global_config_dict.get(ALLOW_UNSUPPORTED_PAIRING_KEY_NAME)) or getenv( diff --git a/scripts/add_legacy_agent_environment_servers.py b/scripts/add_legacy_agent_environment_servers.py index 34c0684d280..b2011dfc30e 100644 --- a/scripts/add_legacy_agent_environment_servers.py +++ b/scripts/add_legacy_agent_environment_servers.py @@ -5,12 +5,20 @@ Rollout collection dispatches to environment servers, so every agent that still owns its episode through `run()` needs one in front of it. Unbound agent templates get none: they are swap sources, -and composition replaces them before anything runs. +and composition replaces them before anything runs. An agent that an environment server already +names gets none either, since a second server would make agent-routed rows ambiguous. A server is named after the environment stem rather than the agent, so swapping the agent leaves the name alone. + # Every config in this repository python scripts/add_legacy_agent_environment_servers.py [--check] + + # Your own configs (files or directories) + python scripts/add_legacy_agent_environment_servers.py my_configs/ my_run.yaml [--check] + +This repository's configs are always indexed, so a config that renames one of their agents with +`_inherit_from` gets a server that inherits (and so replaces) that agent's server. """ from __future__ import annotations @@ -47,6 +55,10 @@ """ +class UnreadableConfigError(ValueError): + """A config file cannot be read or parsed.""" + + def agent_type_of(instance: dict) -> str | None: agents = instance.get("responses_api_agents") if not isinstance(agents, dict) or len(agents) != 1: @@ -54,6 +66,11 @@ def agent_type_of(instance: dict) -> str | None: return next(iter(agents)) +def is_rename(instance: dict) -> bool: + """True for an instance that only renames another one through `_inherit_from`.""" + return set(instance) == {"_inherit_from"} and isinstance(instance["_inherit_from"], str) + + def needs_environment_server(instance: dict) -> bool: """True for an agent instance a run dispatches to, so it needs a server in front of it. @@ -63,6 +80,8 @@ def needs_environment_server(instance: dict) -> bool: a run can start, and declaring a server for it strands the reference in every run that merges the overlay without the agent. """ + if is_rename(instance): + return True agent_type = agent_type_of(instance) agent = instance.get("responses_api_agents", {}).get(agent_type) if agent_type else None if not isinstance(agent, dict): @@ -80,7 +99,7 @@ def server_name(instance_name: str, agent_type: str) -> str: return f"{stem}{SUFFIX}" if stem else f"{agent_type}{SUFFIX}" -def config_files() -> list[Path]: +def repo_config_files() -> list[Path]: found = [] for root in ROOTS: for path in sorted((REPO / root).rglob("*.yaml")): @@ -89,45 +108,100 @@ def config_files() -> list[Path]: return found +def config_files_in(paths: list[Path]) -> list[Path]: + """Expand files and directories into YAML files, in a stable order.""" + found = [] + for path in paths: + if path.is_dir(): + found.extend( + p + for pattern in ("*.yaml", "*.yml") + for p in sorted(path.rglob(pattern)) + if ".venv" not in p.parts and "site-packages" not in p.parts + ) + else: + found.append(path) + return found + + def load(path: Path) -> dict | None: + """Load the Gym config in one file, or None when the file is not a mapping. + + Raises for a file that cannot be parsed, so a migration never skips a config silently. + """ try: document = yaml.safe_load(path.read_text()) - except Exception: - return None + except (OSError, yaml.YAMLError) as error: + raise UnreadableConfigError(f"{path}: {error}") from error return document if isinstance(document, dict) else None -def server_names_for(document: dict) -> dict[str, str]: +def fronted_agents(document: dict) -> set[str]: + """Agents that some environment server in the document already names.""" + names = set() + for instance in document.values(): + servers = instance.get("environment_servers") if isinstance(instance, dict) else None + for server in servers.values() if isinstance(servers, dict) else (): + reference = server.get("agent_server") if isinstance(server, dict) else None + if isinstance(reference, dict) and reference.get("name"): + names.add(reference["name"]) + return names + + +def agent_types_in(document: dict, known_types: dict[str, str]) -> dict[str, str]: + """Map each agent instance in the document to its agent type. + + A rename has no `responses_api_agents` body of its own; it takes its source's type. + """ + types = {} + for name, instance in document.items(): + if not isinstance(instance, dict): + continue + agent_type = known_types.get(instance["_inherit_from"]) if is_rename(instance) else agent_type_of(instance) + if agent_type is not None: + types[name] = agent_type + return types + + +def server_names_for(document: dict, known_types: dict[str, str] | None = None) -> dict[str, str]: """Map each bound agent instance in one document to its server name. Two harnesses for one environment share a stem, so a tie falls back to the full instance name. """ stems: dict[str, str] = {} - for name, instance in document.items(): - agent_type = agent_type_of(instance) if isinstance(instance, dict) else None - if agent_type and needs_environment_server(instance): + for name, agent_type in agent_types_in(document, known_types or {}).items(): + if needs_environment_server(document[name]): stems[name] = server_name(name, agent_type) - taken = [s for s in stems.values()] + taken = list(stems.values()) return {name: stem if taken.count(stem) == 1 else f"{name}{SUFFIX}" for name, stem in stems.items()} -def bound_agents_in_repo() -> dict[str, str]: - """Map every bound agent instance in the repo to the server name declared beside it.""" - index: dict[str, str] = {} - for path in config_files(): - document = load(path) - if document is not None: - index.update(server_names_for(document)) - return index - +def index(documents: list[dict]) -> tuple[dict[str, str], dict[str, str]]: + """Index agent types and server names across documents. -def stanzas_for(document: dict, bound_agents: dict[str, str]) -> list[str]: + Returns ``(agent_types, servers)``: each agent instance's type, and the server name declared + (or that this script would declare) beside it. Renaming configs look their source up here. + """ + agent_types: dict[str, str] = {} + for document in documents: + agent_types.update(agent_types_in(document, agent_types)) + # A rename may precede the document defining its source. + for document in documents: + agent_types.update(agent_types_in(document, agent_types)) + servers: dict[str, str] = {} + for document in documents: + servers.update(server_names_for(document, agent_types)) + return agent_types, servers + + +def stanzas_for(document: dict, bound_agents: dict[str, str], known_types: dict[str, str] | None = None) -> list[str]: blocks: list[str] = [] declared: set[str] = set() - servers = server_names_for(document) + fronted = fronted_agents(document) + servers = server_names_for(document, known_types) for name, instance in document.items(): server = servers.get(name) - if server is None or server in document or server in declared: + if server is None or name in fronted or server in document or server in declared: continue declared.add(server) source = instance.get("_inherit_from") @@ -139,30 +213,67 @@ def stanzas_for(document: dict, bound_agents: dict[str, str]) -> list[str]: return blocks -def main() -> int: - parser = argparse.ArgumentParser(description=__doc__) +def append_blocks(text: str, blocks: list[str]) -> str: + """Return ``text`` with ``blocks`` appended, leaving the existing content, comments included, unchanged.""" + return (text if text.endswith("\n") or not text else text + "\n") + "".join(blocks) + + +def display(path: Path) -> str: + try: + return str(path.resolve().relative_to(REPO)) + except ValueError: + return str(path) + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + parser.add_argument( + "paths", + nargs="*", + type=Path, + help="Config files or directories to update. Default: this repository's configs.", + ) parser.add_argument("--check", action="store_true", help="Report what is missing without writing.") - args = parser.parse_args() + args = parser.parse_args(argv) + + targets = config_files_in(args.paths) if args.paths else repo_config_files() + unreadable: list[str] = [] + documents: dict[Path, dict] = {} + for path in targets: + try: + document = load(path) + except UnreadableConfigError as error: + unreadable.append(str(error)) + continue + if document is not None: + documents[path] = document - bound_agents = bound_agents_in_repo() - changed, added = 0, 0 - for path in config_files(): - document = load(path) - if document is None: + repo_documents = [] + for path in repo_config_files(): + try: + document = load(path) + except UnreadableConfigError: continue - blocks = stanzas_for(document, bound_agents) + if document is not None: + repo_documents.append(document) + agent_types, bound_agents = index(repo_documents + list(documents.values())) + + changed, added = 0, 0 + for path, document in documents.items(): + blocks = stanzas_for(document, bound_agents, agent_types) if not blocks: continue if not args.check: - text = path.read_text() - if not text.endswith("\n"): - text += "\n" - path.write_text(text + "".join(blocks)) + path.write_text(append_blocks(path.read_text(), blocks)) changed += 1 added += len(blocks) - print(f"{'would add' if args.check else 'added'} {len(blocks)} to {path.relative_to(REPO)}") + print(f"{'would add' if args.check else 'added'} {len(blocks)} to {display(path)}") - print(f"\nfiles touched: {changed}, blocks added: {added}") + for message in unreadable: + print(f"could not update {message}", file=sys.stderr) + print(f"\nfiles touched: {changed}, blocks added: {added}, unreadable files: {len(unreadable)}") + if unreadable: + return 2 return 1 if (args.check and changed) else 0 diff --git a/tests/unit_tests/test_add_legacy_agent_environment_servers.py b/tests/unit_tests/test_add_legacy_agent_environment_servers.py new file mode 100644 index 00000000000..bac3add6d6b --- /dev/null +++ b/tests/unit_tests/test_add_legacy_agent_environment_servers.py @@ -0,0 +1,129 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""The environment server migration script, run on configs outside the repository.""" + +import importlib.util +from pathlib import Path + +import pytest +import yaml + +from nemo_gym.global_config import legacy_environment_server_name + + +SCRIPT = Path(__file__).resolve().parents[2] / "scripts" / "add_legacy_agent_environment_servers.py" +_spec = importlib.util.spec_from_file_location("add_legacy_agent_environment_servers", SCRIPT) +migration = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(migration) + +AGENT_CONFIG = """\ +my_resources: + resources_servers: + mcqa: + entrypoint: app.py + domain: other +my_simple_agent: + responses_api_agents: + simple_agent: + entrypoint: app.py + resources_server: + type: resources_servers + name: my_resources +""" + + +def _environment_servers(document: dict) -> dict[str, str]: + """Map each agent to the environment servers that name it.""" + fronting: dict[str, list[str]] = {} + for name, instance in document.items(): + for server in (instance.get("environment_servers") or {}).values() if isinstance(instance, dict) else (): + fronting.setdefault(server["agent_server"]["name"], []).append(name) + return fronting + + +def test_migrates_a_config_outside_the_repository(tmp_path: Path) -> None: + config = tmp_path / "my_run.yaml" + config.write_text(AGENT_CONFIG) + + assert migration.main([str(tmp_path)]) == 0 + + document = yaml.safe_load(config.read_text()) + assert _environment_servers(document) == {"my_simple_agent": ["my_environment_server"]} + assert config.read_text().startswith(AGENT_CONFIG) # existing content, comments included, is untouched + + +def test_check_reports_without_writing(tmp_path: Path, capsys: pytest.CaptureFixture) -> None: + config = tmp_path / "my_run.yaml" + config.write_text(AGENT_CONFIG) + + assert migration.main(["--check", str(config)]) == 1 + + assert config.read_text() == AGENT_CONFIG + assert f"would add 1 to {config}" in capsys.readouterr().out + + +def test_leaves_an_agent_that_any_environment_server_fronts(tmp_path: Path) -> None: + # A second server in front of the agent would make agent-routed rows ambiguous. + config = tmp_path / "my_run.yaml" + config.write_text( + AGENT_CONFIG + + """\ +my_episodes: + environment_servers: + single_agent_turn_legacy: + agent_server: + type: responses_api_agents + name: my_simple_agent +""" + ) + before = config.read_text() + + assert migration.main([str(config)]) == 0 + + assert config.read_text() == before + + +def test_rename_inherits_the_renamed_agents_server(tmp_path: Path) -> None: + # `_inherit_from` moves the source agent, so the source's server must move with it. + config = tmp_path / "my_run.yaml" + config.write_text("renamed_agent:\n _inherit_from: workplace_assistant_simple_agent\n") + + assert migration.main([str(config)]) == 0 + + server = yaml.safe_load(config.read_text())["renamed_environment_server"] + assert server["_inherit_from"] == "workplace_assistant_environment_server" + assert server["environment_servers"]["legacy_agent"]["agent_server"]["name"] == "renamed_agent" + + +def test_reports_an_unreadable_config(tmp_path: Path, capsys: pytest.CaptureFixture) -> None: + (tmp_path / "broken.yaml").write_text("key: [unclosed\n") + + assert migration.main([str(tmp_path)]) == 2 + + assert "broken.yaml" in capsys.readouterr().err + + +@pytest.mark.parametrize( + ("agent_name", "agent_type"), + [ + ("workplace_assistant_simple_agent", "simple_agent"), + ("gpqa_mcqa_hermes_agent", "hermes_agent"), + ("single_step_tool_use_with_argument_comparison_swe", "tool_simulation_agent"), + ("simple_agent", "simple_agent"), + ], +) +def test_server_names_match_the_relays_gym_generates(agent_name: str, agent_type: str) -> None: + # The deprecation warning prints a block to paste; it must match what this script writes. + assert migration.server_name(agent_name, agent_type) == legacy_environment_server_name(agent_name, agent_type) diff --git a/tests/unit_tests/test_global_config.py b/tests/unit_tests/test_global_config.py index 5363be411a4..9902499f051 100644 --- a/tests/unit_tests/test_global_config.py +++ b/tests/unit_tests/test_global_config.py @@ -2144,12 +2144,13 @@ def _agent_without_environment_server_config(**extra) -> DictConfig: def test_agent_without_environment_server_gets_a_legacy_relay(self, caplog: LogCaptureFixture) -> None: # A config written before environment servers keeps running: collection reaches the agent through a - # generated relay, never directly, and the user is told what was generated and how to stop relying on it. + # generated relay, never directly, and the warning tells the user exactly how to migrate. config = self._agent_without_environment_server_config() with caplog.at_level("WARNING"): GlobalConfigDictParser()._front_agents_without_environment_server(config) - assert OmegaConf.to_container(config["mcqa_simple_agent_environment_server"]) == { + # Named as the migration script would declare it, so pasting the logged block matches the script. + assert OmegaConf.to_container(config["mcqa_environment_server"]) == { "environment_servers": { "legacy_agent": { "entrypoint": "app.py", @@ -2157,16 +2158,21 @@ def test_agent_without_environment_server_gets_a_legacy_relay(self, caplog: LogC } } } - assert _environment_servers_by_agent(config) == {"mcqa_simple_agent": ["mcqa_simple_agent_environment_server"]} - assert "`mcqa_simple_agent` -> `mcqa_simple_agent_environment_server`" in caplog.text - assert "error_on_agent_without_environment_server=true" in caplog.text + assert _environment_servers_by_agent(config) == {"mcqa_simple_agent": ["mcqa_environment_server"]} + assert "DEPRECATED: agents without an environment server: `mcqa_simple_agent`" in caplog.text + assert "AgentWithoutEnvironmentServerError" in caplog.text + assert "python scripts/add_legacy_agent_environment_servers.py path/to/config.yaml" in caplog.text + assert "mcqa_environment_server:\n" in caplog.text + assert "name: mcqa_simple_agent" in caplog.text + assert "error_on_agent_without_environment_server: true" in caplog.text def test_agent_without_environment_server_is_rejected_when_strict(self) -> None: config = self._agent_without_environment_server_config(error_on_agent_without_environment_server=True) with raises(AgentWithoutEnvironmentServerError) as exc_info: GlobalConfigDictParser()._front_agents_without_environment_server(config) assert "mcqa_simple_agent" in str(exc_info.value) - assert "mcqa_simple_agent_environment_server" not in config + assert "scripts/add_legacy_agent_environment_servers.py" in str(exc_info.value) + assert "mcqa_environment_server" not in config config["mcqa_environment_server"] = { "environment_servers": { @@ -2181,7 +2187,7 @@ def test_agent_without_environment_server_is_rejected_when_strict(self) -> None: def test_agent_with_environment_server_gets_no_relay(self, caplog: LogCaptureFixture) -> None: # A second server in front of the same agent would make agent-routed rows ambiguous. config = self._agent_without_environment_server_config( - mcqa_environment_server={ + mcqa_served={ "environment_servers": { "legacy_agent": { "entrypoint": "app.py", @@ -2193,20 +2199,18 @@ def test_agent_with_environment_server_gets_no_relay(self, caplog: LogCaptureFix with caplog.at_level("WARNING"): GlobalConfigDictParser()._front_agents_without_environment_server(config) - assert "mcqa_simple_agent_environment_server" not in config - assert _environment_servers_by_agent(config) == {"mcqa_simple_agent": ["mcqa_environment_server"]} - assert "legacy_agent relays" not in caplog.text + assert "mcqa_environment_server" not in config + assert _environment_servers_by_agent(config) == {"mcqa_simple_agent": ["mcqa_served"]} + assert "DEPRECATED" not in caplog.text def test_generated_relay_name_avoids_existing_entries(self) -> None: config = self._agent_without_environment_server_config( - mcqa_simple_agent_environment_server={"note": "an unrelated top-level entry"} + mcqa_environment_server={"note": "an unrelated top-level entry"} ) GlobalConfigDictParser()._front_agents_without_environment_server(config) - assert config["mcqa_simple_agent_environment_server"] == {"note": "an unrelated top-level entry"} - assert _environment_servers_by_agent(config) == { - "mcqa_simple_agent": ["mcqa_simple_agent_environment_server_environment_server"] - } + assert config["mcqa_environment_server"] == {"note": "an unrelated top-level entry"} + assert _environment_servers_by_agent(config) == {"mcqa_simple_agent": ["mcqa_simple_agent_environment_server"]} def test_parse_runs_a_config_without_environment_servers(self) -> None: # End to end through parse(): the generated relay resolves its agent reference and is assigned an address @@ -2223,10 +2227,38 @@ def test_parse_runs_a_config_without_environment_servers(self) -> None: ) ) - relay = resolved["mcqa_simple_agent_environment_server"]["environment_servers"]["legacy_agent"] + relay = resolved["mcqa_environment_server"]["environment_servers"]["legacy_agent"] assert relay["agent_server"] == {"type": "responses_api_agents", "name": "mcqa_simple_agent"} assert "host" in relay and "port" in relay + def test_dangling_environment_server_agent_reference_suggests_migration(self) -> None: + # Renaming an agent with `_inherit_from` moves it, stranding the environment server that named it. + config = OmegaConf.merge( + GlobalConfigDictParserConfig.NO_MODEL_GLOBAL_CONFIG_DICT, + self._agent_without_environment_server_config( + renamed_agent={"_inherit_from": "mcqa_simple_agent"}, + mcqa_environment_server={ + "environment_servers": { + "legacy_agent": { + "entrypoint": "app.py", + "agent_server": {"type": "responses_api_agents", "name": "mcqa_simple_agent"}, + } + } + }, + ), + ) + with raises( + ServerRefNotFoundError, match="renamed with `_inherit_from`.*add_legacy_agent_environment_servers" + ): + GlobalConfigDictParser().parse( + GlobalConfigDictParserConfig( + initial_global_config_dict=config, + skip_load_from_cli=True, + skip_load_from_dotenv=True, + offline=True, + ) + ) + @mark.parametrize("resources_server", ["reasoning_gym", "tavily_search"]) def test_langchain_deepagents_configs_have_environment_servers(self, resources_server: str) -> None: agent_name = f"{resources_server}_langchain_deepagents_agent_model_server" From 2a1177468973458461fa7e4fec970fb96de3aec2 Mon Sep 17 00:00:00 2001 From: Ananth Subramaniam Date: Sun, 4 Oct 2026 18:43:00 -0700 Subject: [PATCH 3/3] docs: put one sentence per line in the environment server deprecation text Comments, docstrings, the deprecation warning, the strict-mode error, and the troubleshooting page now keep each sentence on its own line instead of reflowing clauses across lines. Long sentences are split. The warning quoted in the docs matches the new text. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Ananth Subramaniam --- .../pages/troubleshooting/configuration.mdx | 40 +++++++------- nemo_gym/global_config.py | 52 ++++++++++--------- .../add_legacy_agent_environment_servers.py | 25 +++++---- tests/unit_tests/test_global_config.py | 11 ++-- 4 files changed, 68 insertions(+), 60 deletions(-) diff --git a/fern/versions/latest/pages/troubleshooting/configuration.mdx b/fern/versions/latest/pages/troubleshooting/configuration.mdx index 722f1d7062a..04fbfc6440a 100644 --- a/fern/versions/latest/pages/troubleshooting/configuration.mdx +++ b/fern/versions/latest/pages/troubleshooting/configuration.mdx @@ -176,19 +176,19 @@ Bypassing validation means invalid servers won't start. Use this only for debugg ``` DEPRECATED: agents without an environment server: `my_simple_agent`. -Rollout collection reaches an agent only through an environment server. This run generated a -legacy_agent environment server for each agent above, which relays /run to the agent unchanged, so -results are not affected. A future release will reject these configs with -AgentWithoutEnvironmentServerError. +Rollout collection reaches an agent only through an environment server. +This run generated a legacy_agent environment server for each agent above. +Each one relays /run to its agent unchanged, so results are not affected. +A future release will reject these configs with AgentWithoutEnvironmentServerError. ``` -**When**: An agent instance has no environment server naming it in `agent_server`. Rollout -collection reaches agents only through an environment server, so Gym generates a `legacy_agent` -relay in front of the agent and warns. Your config keeps running for now, but **a future release will -make this an error**, so migrate it. +**When**: An agent instance has no environment server naming it in `agent_server`. +Rollout collection reaches agents only through an environment server. +Gym therefore generates a `legacy_agent` relay in front of the agent and logs this warning. +Your config keeps running for now, but a future release will make this an error, so migrate it. -**Fix**: Declare the environment servers in your config. From a NeMo Gym checkout, run the migration -script on your config files or directories: +**Fix**: Declare the environment servers in your config. +From a NeMo Gym checkout, run the migration script on your config files or directories: ```bash # Report what would change @@ -198,14 +198,14 @@ python scripts/add_legacy_agent_environment_servers.py path/to/configs/ --check python scripts/add_legacy_agent_environment_servers.py path/to/configs/ ``` -The script appends a block to each config that needs one and leaves the existing content unchanged, -comments included. -It skips agents that an environment server already names, and agents that only override fields of -an agent defined elsewhere. Run without paths, it updates this repository's configs. It exits with -1 under `--check` when something needs adding, and with 2 when a file cannot be read. +The script appends a block to each config that needs one and leaves the existing content unchanged, comments included. +It skips agents that an environment server already names. +It also skips agents that only override fields of an agent defined elsewhere. +Run without paths, it updates this repository's configs. +It exits with 1 under `--check` when something needs adding, and with 2 when a file cannot be read. -You can also add the block by hand. The warning prints the exact block for each agent, at the same -level as the agents: +You can also add the block by hand. +The warning prints the exact block for each agent; add it at the same level as the agents: ```yaml my_environment_server: @@ -217,9 +217,9 @@ my_environment_server: name: my_simple_agent ``` -If you rename an agent with `_inherit_from`, the agent's environment server must be renamed with it, -because `_inherit_from` moves the source. The script writes a server that inherits the source agent's -server. Do not add a second server in front of an agent: it makes agent-routed rows ambiguous. +If you rename an agent with `_inherit_from`, the agent's environment server must be renamed with it, because `_inherit_from` moves the source. +The script writes a server that inherits the source agent's server. +Do not add a second server in front of an agent, because that makes agent-routed rows ambiguous. #### Require Declared Environment Servers diff --git a/nemo_gym/global_config.py b/nemo_gym/global_config.py index 6e2ff9eaf83..ddf080b0a73 100644 --- a/nemo_gym/global_config.py +++ b/nemo_gym/global_config.py @@ -126,8 +126,8 @@ ENVIRONMENT_SERVER_NAME_KEY_NAME = "environment_server_name" ENVIRONMENT_SERVER_ROUTES_KEY_NAME = "environment_server_routes" ENVIRONMENT_ROUTING_MODE_KEY_NAME = "environment_routing_mode" -# When set, an agent that no environment server fronts fails config validation instead of getting a generated -# legacy_agent relay. +# When set, an agent without an environment server fails config validation. +# When unset, Gym generates a legacy_agent relay for the agent and logs a deprecation warning. ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME = "error_on_agent_without_environment_server" NEMO_GYM_RESERVED_TOP_LEVEL_KEYS = [ CONFIG_PATHS_KEY_NAME, @@ -593,9 +593,10 @@ def validate_and_populate_defaults( and server_instance_config.get_server_ref().type == ENVIRONMENT_SERVER_TYPE_KEY_NAME ): hint += ( - "\nIf the agent was renamed with `_inherit_from`, its environment server must be renamed " - "with it: run `python scripts/add_legacy_agent_environment_servers.py ` " - "from a NeMo Gym checkout, or point this server's agent_server.name at the new name." + "\nIf the agent was renamed with `_inherit_from`, its environment server must be renamed with it." + "\nTo fix this automatically, run " + "`python scripts/add_legacy_agent_environment_servers.py ` from a NeMo Gym checkout." + "\nOr point this server's agent_server.name at the agent's new name." ) raise ServerRefNotFoundError( f"""In server instance '{server_instance_config.name}', field '{field_name}' references {maybe_server_ref.type}/'{maybe_server_ref.name}', which is not defined in the merged config. @@ -1083,14 +1084,15 @@ def _carry_over_agent_bindings(original: DictConfig, composed: DictConfig) -> No def _front_agents_without_environment_server(self, global_config_dict: DictConfig) -> None: """Give every agent an environment server, since rollout collection reaches agents only through one. - Runs after composition, so every agent left is one a run can dispatch to. An agent that no environment - server names gets a generated `legacy_agent` relay, the same block - scripts/add_legacy_agent_environment_servers.py declares, and one warning lists every generated relay. - This keeps configs written before environment servers running. With - `error_on_agent_without_environment_server` set, such an agent is an error instead. + Runs after composition, so every agent left is one a run can dispatch to. + An agent that no environment server names gets a generated `legacy_agent` relay. + The relay is the same block that scripts/add_legacy_agent_environment_servers.py declares. + One deprecation warning lists every generated relay and how to declare it. + This keeps configs written before environment servers running. + With `error_on_agent_without_environment_server` set, such an agent is an error instead. - Only agents with no environment server are touched, so a generated relay can never make an agent's - routing ambiguous, and an existing reference is never rewritten. + Only agents with no environment server are touched. + A generated relay therefore never makes an agent's routing ambiguous, and no existing reference is rewritten. """ with_environment_server = set() for instance in global_config_dict.values(): @@ -1120,10 +1122,9 @@ def _front_agents_without_environment_server(self, global_config_dict: DictConfi f"""Agent instance(s) have no environment server, so rollout collection cannot reach them: {listing} -Declare one for each, naming the agent in its `{AGENT_SERVER_REF_KEY_NAME}` reference, or run -`python scripts/add_legacy_agent_environment_servers.py ` from a NeMo Gym checkout. -Unset {ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME} -to generate a legacy_agent relay for each instead, with a deprecation warning.""" +Declare one for each, naming the agent in its `{AGENT_SERVER_REF_KEY_NAME}` reference. +To add them automatically, run `python scripts/add_legacy_agent_environment_servers.py ` from a NeMo Gym checkout. +Unset {ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME} to generate a legacy_agent relay for each instead, with a deprecation warning.""" ) agent_types = {agent.name: agent.agent_type for agent in self._agent_instances(global_config_dict)} @@ -1348,7 +1349,7 @@ def parse(self, parse_config: Optional[GlobalConfigDictParserConfig] = None) -> # a '???' in a deleted or overwritten branch is not reported. Otherwise the first unset # value surfaces as an opaque MissingMandatoryValue deep in the pipeline. self.raise_on_missing_values(global_config_dict) - # NOTE(martas): this is for agents not attached to an environment server; see the method for strict mode + # NOTE(martas): this is for catching agents not attached to an environment server self._front_agents_without_environment_server(global_config_dict) # TODO @bxyu-nvidia: We need a better way of handling dummy model configs @@ -1761,8 +1762,9 @@ def model_type_for(global_config_dict: DictConfig, model_server_name: Optional[s def legacy_environment_server_name(agent_name: str, agent_type: str) -> str: """Name an agent's legacy_agent relay after its environment, as the migration script does. - Stripping the agent type (or a trailing `_agent`) keeps the name stable across agent swaps: - `workplace_assistant_simple_agent` -> `workplace_assistant_environment_server`. + The agent type, or else a trailing `_agent`, is stripped from the agent name. + For example, `workplace_assistant_simple_agent` becomes `workplace_assistant_environment_server`. + The name then stays the same when the agent is swapped for another type. """ stem = agent_name.removesuffix(f"_{agent_type}").removesuffix(agent_type).rstrip("_") if stem == agent_name: @@ -1789,10 +1791,10 @@ def agents_without_environment_server_deprecation(generated: Mapping[str, str]) ) agents = ", ".join(f"`{agent}`" for agent in generated) return f"""DEPRECATED: agents without an environment server: {agents}. -Rollout collection reaches an agent only through an environment server. This run generated a -legacy_agent environment server for each agent above, which relays /run to the agent unchanged, so -results are not affected. A future release will reject these configs with -AgentWithoutEnvironmentServerError. +Rollout collection reaches an agent only through an environment server. +This run generated a legacy_agent environment server for each agent above. +Each one relays /run to its agent unchanged, so results are not affected. +A future release will reject these configs with AgentWithoutEnvironmentServerError. To migrate, declare the environment servers in your config. Either: @@ -1805,8 +1807,8 @@ def agents_without_environment_server_deprecation(generated: Mapping[str, str]) 2. Or add these blocks yourself, at the same level as the agents: {textwrap.indent(blocks, " ")} -To make this an error now, set {ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME}: true in your config or pass -+{ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME}=true.""" +To make this an error now, set {ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME}: true in your config. +You can also pass +{ERROR_ON_AGENT_WITHOUT_ENVIRONMENT_SERVER_KEY_NAME}=true on the command line.""" def pairing_override_enabled(global_config_dict: DictConfig) -> bool: diff --git a/scripts/add_legacy_agent_environment_servers.py b/scripts/add_legacy_agent_environment_servers.py index b2011dfc30e..a61f33dd1f0 100644 --- a/scripts/add_legacy_agent_environment_servers.py +++ b/scripts/add_legacy_agent_environment_servers.py @@ -3,10 +3,11 @@ """Declare a legacy-agent environment server beside every bound agent instance. -Rollout collection dispatches to environment servers, so every agent that still owns its episode -through `run()` needs one in front of it. Unbound agent templates get none: they are swap sources, -and composition replaces them before anything runs. An agent that an environment server already -names gets none either, since a second server would make agent-routed rows ambiguous. +Rollout collection dispatches to environment servers. +Every agent that still owns its episode through `run()` therefore needs one in front of it. +Unbound agent templates get none: they are swap sources, and composition replaces them before anything runs. +An agent that an environment server already names gets none either. +A second server in front of it would make agent-routed rows ambiguous. A server is named after the environment stem rather than the agent, so swapping the agent leaves the name alone. @@ -17,8 +18,9 @@ # Your own configs (files or directories) python scripts/add_legacy_agent_environment_servers.py my_configs/ my_run.yaml [--check] -This repository's configs are always indexed, so a config that renames one of their agents with -`_inherit_from` gets a server that inherits (and so replaces) that agent's server. +This repository's configs are always indexed. +A config that renames one of their agents with `_inherit_from` therefore gets a server that inherits that agent's server. +Inheriting moves the source server, so the renamed agent ends up with exactly one server. """ from __future__ import annotations @@ -127,7 +129,7 @@ def config_files_in(paths: list[Path]) -> list[Path]: def load(path: Path) -> dict | None: """Load the Gym config in one file, or None when the file is not a mapping. - Raises for a file that cannot be parsed, so a migration never skips a config silently. + Raises for a file that cannot be read or parsed, so a migration never skips a config silently. """ try: document = yaml.safe_load(path.read_text()) @@ -151,7 +153,8 @@ def fronted_agents(document: dict) -> set[str]: def agent_types_in(document: dict, known_types: dict[str, str]) -> dict[str, str]: """Map each agent instance in the document to its agent type. - A rename has no `responses_api_agents` body of its own; it takes its source's type. + A rename has no `responses_api_agents` body of its own. + It takes the type of the agent it renames. """ types = {} for name, instance in document.items(): @@ -179,8 +182,10 @@ def server_names_for(document: dict, known_types: dict[str, str] | None = None) def index(documents: list[dict]) -> tuple[dict[str, str], dict[str, str]]: """Index agent types and server names across documents. - Returns ``(agent_types, servers)``: each agent instance's type, and the server name declared - (or that this script would declare) beside it. Renaming configs look their source up here. + Returns ``(agent_types, servers)``. + ``agent_types`` maps each agent instance to its type. + ``servers`` maps each agent instance to the server name declared beside it, or the one this script would declare. + Renaming configs look up the agent they rename here. """ agent_types: dict[str, str] = {} for document in documents: diff --git a/tests/unit_tests/test_global_config.py b/tests/unit_tests/test_global_config.py index 9902499f051..9e958604aea 100644 --- a/tests/unit_tests/test_global_config.py +++ b/tests/unit_tests/test_global_config.py @@ -2143,8 +2143,9 @@ def _agent_without_environment_server_config(**extra) -> DictConfig: ) def test_agent_without_environment_server_gets_a_legacy_relay(self, caplog: LogCaptureFixture) -> None: - # A config written before environment servers keeps running: collection reaches the agent through a - # generated relay, never directly, and the warning tells the user exactly how to migrate. + # A config written before environment servers keeps running. + # Collection reaches the agent through a generated relay, never directly. + # The warning tells the user exactly how to migrate. config = self._agent_without_environment_server_config() with caplog.at_level("WARNING"): GlobalConfigDictParser()._front_agents_without_environment_server(config) @@ -2213,8 +2214,8 @@ def test_generated_relay_name_avoids_existing_entries(self) -> None: assert _environment_servers_by_agent(config) == {"mcqa_simple_agent": ["mcqa_simple_agent_environment_server"]} def test_parse_runs_a_config_without_environment_servers(self) -> None: - # End to end through parse(): the generated relay resolves its agent reference and is assigned an address - # like any declared server. + # End to end through parse(), the generated relay resolves its agent reference. + # It is assigned an address like any declared server. resolved = GlobalConfigDictParser().parse( GlobalConfigDictParserConfig( initial_global_config_dict=OmegaConf.merge( @@ -2248,7 +2249,7 @@ def test_dangling_environment_server_agent_reference_suggests_migration(self) -> ), ) with raises( - ServerRefNotFoundError, match="renamed with `_inherit_from`.*add_legacy_agent_environment_servers" + ServerRefNotFoundError, match="(?s)renamed with `_inherit_from`.*add_legacy_agent_environment_servers" ): GlobalConfigDictParser().parse( GlobalConfigDictParserConfig(