From e3ad6600970a317fa92a53a4f12398bd12977452 Mon Sep 17 00:00:00 2001 From: Wojciech Prazuch Date: Mon, 29 Jun 2026 11:37:40 +0200 Subject: [PATCH 1/3] feat: clean config errors + run/test built-in servers from any cwd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C4 validation hardening and the deferred gym-env-test install-root plumbing for epic #1205 (criteria C4 pre-start validation, C5 external-cwd) / nvbug 6383277. Clean errors (C4): - convert two raw ValueErrors to ConfigError subclasses so they print cleanly (no traceback) via exit_cleanly_on_config_error and gym env validate: InheritPathNotFoundError (bad _inherit_from/swap/copy path) and AlmostServerError (almost-servers present + error_on_almost_servers) - docs: release-notes referenced `+dryrun=true`; the key is `dry_run` Run/test built-in servers from any cwd (C5): - add `_resolve_server_dir` (cwd first, then the Gym install root) and a TestConfig.resolved_dir_path; gym env test (single + all), data validation, venv cleanup, and gym env packages now resolve built-in servers under the install root instead of assuming a repo checkout - gym env test (all) discovers modules under both the cwd and the install root - setup_command PYTHONPATH now includes the project root (the dir holding the server-type dirs) so generated `resources_servers.` imports resolve Tests: ConfigError assertion on the almost-server path; _resolve_server_dir and TestConfig.resolved_dir_path resolution; updated run_command PYTHONPATH expectations. Out of scope (documented in nvbug): the extra="allow" "typo field" item is not a simple forbid — server authors add arbitrary custom fields their server reads, so forbidding would break configs (a fuzzy "did you mean" warning is the only non-breaking option, deferred). Turning gym env validate into a full dataset/ prepare/runtime preflight is a separate enhancement. Signed-off-by: Wojciech Prazuch --- .../latest/pages/about/release-notes.mdx | 2 +- nemo_gym/cli/env.py | 66 ++++++++++++++----- nemo_gym/cli/setup_command.py | 14 ++-- nemo_gym/config_types.py | 9 +++ nemo_gym/global_config.py | 6 +- tests/unit_tests/test_cli.py | 24 +++++++ tests/unit_tests/test_cli_setup_command.py | 27 ++++---- tests/unit_tests/test_global_config.py | 6 +- 8 files changed, 118 insertions(+), 36 deletions(-) diff --git a/fern/versions/latest/pages/about/release-notes.mdx b/fern/versions/latest/pages/about/release-notes.mdx index f960c4762f..8d9df17b45 100644 --- a/fern/versions/latest/pages/about/release-notes.mdx +++ b/fern/versions/latest/pages/about/release-notes.mdx @@ -131,7 +131,7 @@ Added 5 new agent servers: Aviary agent, proof refinement agent, SWE agents, too ### Infrastructure & Developer Experience - PyPI compatibility: install via `pip install nemo-gym` -- Dry run mode: `ng_run +dryrun=true` to validate configs and install environments without starting servers +- Dry run mode: `ng_run +dry_run=true` to validate configs and install environments without starting servers - `ng_status` command to list running servers and their health - FastAPI worker support for higher throughput across multiple workers - Server stdout/stderr redirection with server name prefixes diff --git a/nemo_gym/cli/env.py b/nemo_gym/cli/env.py index 5513f9e621..3492225f7b 100644 --- a/nemo_gym/cli/env.py +++ b/nemo_gym/cli/env.py @@ -89,6 +89,19 @@ def wrapper(*args, **kwargs): return wrapper +def _resolve_server_dir(rel_path: Path) -> Path: + """Resolve a relative server dir (e.g. ``resources_servers/``) to an absolute path. + + Checks the current working directory first (a user's local server), then falls back to the Gym + install root (``PARENT_DIR``) where built-in servers live in both editable and wheel installs. + This lets ``gym env test`` find and run built-in servers from any cwd, not just a repo checkout. + """ + cwd_path = Path.cwd() / rel_path + if (cwd_path / "requirements.txt").exists() or (cwd_path / "pyproject.toml").exists(): + return cwd_path + return PARENT_DIR / rel_path + + class RunConfig(BaseNeMoGymCLIConfig): """ Start NeMo Gym servers for agents, models, and resources. @@ -135,6 +148,15 @@ def model_post_init(self, context): # pragma: no cover def dir_path(self) -> Path: return self._dir_path + @property + def resolved_dir_path(self) -> Path: + """Absolute server dir resolved against the cwd, then the Gym install root. + + Use this for filesystem access (reading data, running the suite); use ``dir_path`` (the + relative entrypoint) for display and example commands shown to the user. + """ + return _resolve_server_dir(self._dir_path) + class RunHelper: # pragma: no cover _head_server: uvicorn.Server @@ -459,8 +481,9 @@ def _validate_data_single(test_config: TestConfig) -> None: # pragma: no cover if test_config.dir_path.parts[0] != "resources_servers": return - # Check that the required examples and example metrics are present. - example_fpath = test_config.dir_path / "data/example.jsonl" + # Check that the required examples and example metrics are present. Read from the resolved dir + # (built-ins live under the install root) while messages reference the relative entrypoint. + example_fpath = test_config.resolved_dir_path / "data/example.jsonl" assert example_fpath.exists(), ( f"A jsonl file containing 5 examples is required for the {test_config.dir_path} resources server. The file must be found at {example_fpath}. Usually this example data is just the first 5 examples of your train dataset." ) @@ -469,7 +492,7 @@ def _validate_data_single(test_config: TestConfig) -> None: # pragma: no cover assert count == 5, f"Expected 5 examples at {example_fpath} but got {count}." server_type_name = test_config.dir_path.parts[-1] - example_metrics_fpath = test_config.dir_path / "data/example_metrics.json" + example_metrics_fpath = test_config.resolved_dir_path / "data/example_metrics.json" assert ( example_metrics_fpath.exists() ), f"""You must run the example data validation for the example data found at {example_fpath}. @@ -499,11 +522,11 @@ def _validate_data_single(test_config: TestConfig) -> None: # pragma: no cover f"Expected 5 examples in the metrics at {example_metrics_fpath}, but got {example_metrics['Number of examples']}" ) - conflict_paths = glob(str(test_config.dir_path / "data/*conflict*")) + conflict_paths = glob(str(test_config.resolved_dir_path / "data/*conflict*")) conflict_paths_str = "\n- ".join([""] + conflict_paths) assert not conflict_paths, f"Found {len(conflict_paths)} conflicting paths: {conflict_paths_str}" - example_rollouts_fpath = test_config.dir_path / "data/example_rollouts.jsonl" + example_rollouts_fpath = test_config.resolved_dir_path / "data/example_rollouts.jsonl" assert example_rollouts_fpath.exists(), f"""You must run the example data through your agent and provide the example rollouts at `{example_rollouts_fpath}`. Your commands should look something like: @@ -533,8 +556,8 @@ def _validate_data_single(test_config: TestConfig) -> None: # pragma: no cover def _test_single(test_config: TestConfig, global_config_dict: DictConfig) -> Popen: # pragma: no cover # Eventually we may want more sophisticated testing here, but this is sufficient for now. prefix = test_config.entrypoint.replace("/", "\\/") - command = f"""{setup_env_command(test_config.dir_path, global_config_dict, prefix)} && pytest""" - return run_command(command, test_config.dir_path) + command = f"""{setup_env_command(test_config.resolved_dir_path, global_config_dict, prefix)} && pytest""" + return run_command(command, test_config.resolved_dir_path) def test(): # pragma: no cover @@ -616,15 +639,26 @@ def test_all(): # pragma: no cover global_config_dict = get_global_config_dict() test_all_config = TestAllConfig.model_validate(global_config_dict) - candidate_dir_paths = [ - *glob("resources_servers/*"), - *glob("responses_api_agents/*"), - *glob("responses_api_models/*"), - ] - candidate_dir_paths = [p for p in candidate_dir_paths if "pycache" not in p] + # Discover server modules under both the cwd (a user's project) and the Gym install root + # (built-ins, which live under PARENT_DIR in editable and wheel installs). Entrypoints are kept + # relative; the cwd shadows the install root for same-named modules. This lets `gym env test` + # discover and run built-in servers from any cwd, not only a repo checkout. + server_type_dirs = ("resources_servers", "responses_api_agents", "responses_api_models") + seen_rel_paths: set[str] = set() + candidate_dir_paths: List[str] = [] + for root in (Path.cwd(), PARENT_DIR): + for server_type_dir in server_type_dirs: + for module_path in sorted((root / server_type_dir).glob("*")): + if "pycache" in module_path.name or not module_path.is_dir(): + continue + rel_path = f"{server_type_dir}/{module_path.name}" + if rel_path in seen_rel_paths: + continue + seen_rel_paths.add(rel_path) + candidate_dir_paths.append(rel_path) print(f"Found {len(candidate_dir_paths)} total modules:{_display_list_of_paths(candidate_dir_paths)}\n") dir_paths: List[Path] = list(map(Path, candidate_dir_paths)) - dir_paths = [p for p in dir_paths if (p / "README.md").exists()] + dir_paths = [p for p in dir_paths if (_resolve_server_dir(p) / "README.md").exists()] print(f"Found {len(dir_paths)} modules to test:{_display_list_of_paths(dir_paths)}\n") # Keep the full list for the total-vs-tested mismatch check below, then narrow to this shard. @@ -668,7 +702,7 @@ def test_all(): # pragma: no cover data_validation_failed.append(dir_path) if test_all_config.delete_venvs_after_each_test: - venv_path = dir_path / ".venv" + venv_path = _resolve_server_dir(dir_path) / ".venv" print(f"Deleting {venv_path} since `delete_venvs_after_each_test=true`") rmtree(venv_path, ignore_errors=True) @@ -978,7 +1012,7 @@ def pip_list(): # pragma: no cover global_config_dict = get_global_config_dict() config = PipListConfig.model_validate(global_config_dict) - dir_path = Path(config.entrypoint) + dir_path = _resolve_server_dir(Path(config.entrypoint)) venv_path = dir_path / ".venv" if not venv_path.exists(): diff --git a/nemo_gym/cli/setup_command.py b/nemo_gym/cli/setup_command.py index c1940d4933..75d47d95b6 100644 --- a/nemo_gym/cli/setup_command.py +++ b/nemo_gym/cli/setup_command.py @@ -176,12 +176,16 @@ def run_command(command: str, working_dir_path: Path, server_name: str = "") -> global_config_dict = get_global_config_dict() work_dir = f"{working_dir_path.absolute()}" + # The server dir on PYTHONPATH lets `import app` work; the project root (the dir containing + # resources_servers/, responses_api_agents/, ...) lets generated `resources_servers..app` + # style imports resolve, which otherwise fail when running from outside a repo checkout. + project_root = f"{working_dir_path.absolute().parent.parent}" custom_env = environ.copy() - py_path = custom_env.get("PYTHONPATH", None) - if py_path is not None: - custom_env["PYTHONPATH"] = f"{work_dir}:{py_path}" - else: - custom_env["PYTHONPATH"] = work_dir + py_path_entries = [work_dir, project_root] + existing_py_path = custom_env.get("PYTHONPATH") + if existing_py_path: + py_path_entries.append(existing_py_path) + custom_env["PYTHONPATH"] = ":".join(py_path_entries) custom_env["UV_CACHE_DIR"] = global_config_dict[UV_CACHE_DIR_KEY_NAME] diff --git a/nemo_gym/config_types.py b/nemo_gym/config_types.py index 4f694dd873..02842b6b08 100644 --- a/nemo_gym/config_types.py +++ b/nemo_gym/config_types.py @@ -167,6 +167,15 @@ class ServerRefNotFoundError(ConfigError, ValueError): """A server cross-reference points to an instance that is not defined in the merged config.""" +class InheritPathNotFoundError(ConfigError, ValueError): + """An `_inherit_from` / swap / copy directive references a config path that does not exist.""" + + +class AlmostServerError(ConfigError, ValueError): + """One or more server blocks are almost-servers (right shape, failed validation) and + `error_on_almost_servers` is set, so the run is aborted.""" + + ######################################## # Dataset configs for handling and upload/download ######################################## diff --git a/nemo_gym/global_config.py b/nemo_gym/global_config.py index 2942223164..8043ec47f8 100644 --- a/nemo_gym/global_config.py +++ b/nemo_gym/global_config.py @@ -38,8 +38,10 @@ from nemo_gym import CACHE_DIR, PARENT_DIR, RESULTS_DIR, WORKING_DIR from nemo_gym.config_types import ( + AlmostServerError, ConfigMissingValuesError, ConfigPathNotFoundError, + InheritPathNotFoundError, MalformedConfigPathsError, NoServerInstancesError, ServerInstanceConfig, @@ -509,7 +511,7 @@ def _recursive_index_dict_using_path(self, dict_config: DictConfig, path: List[s # absent key still errors clearly. node = dict_config._get_node(k) if isinstance(dict_config, DictConfig) else None if node is None: - raise ValueError(f"Path specified does not exist in config: {path}") + raise InheritPathNotFoundError(f"Path specified does not exist in config: {path}") # The referenced value (or an ancestor of it) is unset. Return the _MISSING_REF sentinel # so the caller makes the swap/copy/inherit target '???' too (instead of calling .pop()/ @@ -621,7 +623,7 @@ def parse(self, parse_config: Optional[GlobalConfigDictParserConfig] = None) -> Found global config dict yaml: {config_to_log_yaml}""" - raise ValueError(error_msg) + raise AlmostServerError(error_msg) server_instance_configs = self.filter_for_server_instance_configs(global_config_dict) diff --git a/tests/unit_tests/test_cli.py b/tests/unit_tests/test_cli.py index d65e860de1..82df178a7a 100644 --- a/tests/unit_tests/test_cli.py +++ b/tests/unit_tests/test_cli.py @@ -32,6 +32,8 @@ _GRACEFUL_SHUTDOWN_TIMEOUT_SEC, RunConfig, RunHelper, + TestConfig, + _resolve_server_dir, _select_shard, exit_cleanly_on_config_error, init_resources_server, @@ -192,6 +194,28 @@ def test_run_helper_falls_back_to_install_when_not_in_cwd(self, tmp_path: Path) assert dir_path == PARENT_DIR / "resources_servers" / "arc_agi" +class TestResolveServerDir: + """`_resolve_server_dir` resolves a relative server dir against cwd first, then the install root.""" + + def test_prefers_local_server_in_cwd(self, tmp_path: Path, monkeypatch) -> None: + local = tmp_path / "resources_servers" / "my_server" + local.mkdir(parents=True) + (local / "requirements.txt").write_text("nemo-gym\n") + monkeypatch.chdir(tmp_path) + assert _resolve_server_dir(Path("resources_servers/my_server")) == local + + def test_falls_back_to_install_root(self, tmp_path: Path, monkeypatch) -> None: + # Empty cwd (no local server) -> the built-in resolves under the install root. + monkeypatch.chdir(tmp_path) + rel = Path("resources_servers/arc_agi") + assert _resolve_server_dir(rel) == PARENT_DIR / rel + + def test_test_config_resolved_dir_path_uses_install_root(self, tmp_path: Path, monkeypatch) -> None: + monkeypatch.chdir(tmp_path) + cfg = TestConfig(entrypoint="resources_servers/arc_agi") + assert cfg.resolved_dir_path == PARENT_DIR / "resources_servers" / "arc_agi" + + class TestRunHelperShutdownReap: """RunHelper.shutdown must reap every server subprocess on every exit path.""" diff --git a/tests/unit_tests/test_cli_setup_command.py b/tests/unit_tests/test_cli_setup_command.py index 4f531eedc1..1394db4d8f 100644 --- a/tests/unit_tests/test_cli_setup_command.py +++ b/tests/unit_tests/test_cli_setup_command.py @@ -260,14 +260,16 @@ def test_sanity(self, monkeypatch: MonkeyPatch) -> None: run_command( command="my command", - working_dir_path=Path("/my path"), + working_dir_path=Path("/root/resources_servers/my_server"), ) expected_args = call( "my command", executable="/bin/bash", shell=True, - env={"PYTHONPATH": "/my path", "UV_CACHE_DIR": "default uv cache dir"}, + # The server dir plus the project root (its grandparent, the dir holding the server-type + # dirs) so `resources_servers.`-style imports resolve. + env={"PYTHONPATH": "/root/resources_servers/my_server:/root", "UV_CACHE_DIR": "default uv cache dir"}, stdout="stdout", stderr="stderr", ) @@ -280,14 +282,17 @@ def test_custom_pythonpath(self, monkeypatch: MonkeyPatch) -> None: run_command( command="my command", - working_dir_path=Path("/my path"), + working_dir_path=Path("/root/resources_servers/my_server"), ) expected_args = call( "my command", executable="/bin/bash", shell=True, - env={"PYTHONPATH": "/my path:existing pythonpath", "UV_CACHE_DIR": "default uv cache dir"}, + env={ + "PYTHONPATH": "/root/resources_servers/my_server:/root:existing pythonpath", + "UV_CACHE_DIR": "default uv cache dir", + }, stdout="stdout", stderr="stderr", ) @@ -301,14 +306,14 @@ def test_custom_uv_cache_dir(self, monkeypatch: MonkeyPatch) -> None: run_command( command="my command", - working_dir_path=Path("/my path"), + working_dir_path=Path("/root/resources_servers/my_server"), ) expected_args = call( "my command", executable="/bin/bash", shell=True, - env={"PYTHONPATH": "/my path", "UV_CACHE_DIR": "my uv cache dir"}, + env={"PYTHONPATH": "/root/resources_servers/my_server:/root", "UV_CACHE_DIR": "my uv cache dir"}, stdout="stdout", stderr="stderr", ) @@ -430,7 +435,7 @@ def test_tee_logs_with_server_name(self, monkeypatch: MonkeyPatch) -> None: run_command( command="my command", - working_dir_path=Path("/my path"), + working_dir_path=Path("/root/resources_servers/my_server"), server_name="my_resources/my_server", ) @@ -438,7 +443,7 @@ def test_tee_logs_with_server_name(self, monkeypatch: MonkeyPatch) -> None: "set -o pipefail; (my command) 2>&1 | tee -a /tmp/gym_logs/my_resources_my_server.log", executable="/bin/bash", shell=True, - env={"PYTHONPATH": "/my path", "UV_CACHE_DIR": "default uv cache dir"}, + env={"PYTHONPATH": "/root/resources_servers/my_server:/root", "UV_CACHE_DIR": "default uv cache dir"}, stdout="stdout", stderr="stderr", ) @@ -455,14 +460,14 @@ def test_tee_logs_falls_back_to_dir_name(self, monkeypatch: MonkeyPatch) -> None run_command( command="my command", - working_dir_path=Path("/my path"), + working_dir_path=Path("/root/resources_servers/my_server"), ) expected_args = call( - "set -o pipefail; (my command) 2>&1 | tee -a /tmp/gym_logs/my path.log", + "set -o pipefail; (my command) 2>&1 | tee -a /tmp/gym_logs/my_server.log", executable="/bin/bash", shell=True, - env={"PYTHONPATH": "/my path", "UV_CACHE_DIR": "default uv cache dir"}, + env={"PYTHONPATH": "/root/resources_servers/my_server:/root", "UV_CACHE_DIR": "default uv cache dir"}, stdout="stdout", stderr="stderr", ) diff --git a/tests/unit_tests/test_global_config.py b/tests/unit_tests/test_global_config.py index 49f913d1f4..51ad5e8403 100644 --- a/tests/unit_tests/test_global_config.py +++ b/tests/unit_tests/test_global_config.py @@ -24,6 +24,8 @@ import nemo_gym.server_utils from nemo_gym import CACHE_DIR, WORKING_DIR from nemo_gym.config_types import ( + AlmostServerError, + ConfigError, ConfigMissingValuesError, ConfigPathNotFoundError, MalformedConfigPathsError, @@ -895,8 +897,10 @@ def hydra_main_wrapper(fn): hydra_main_mock.return_value = hydra_main_wrapper monkeypatch.setattr(nemo_gym.global_config.hydra, "main", hydra_main_mock) - with raises(ValueError, match="almost-server.*validation errors"): + # AlmostServerError is a ConfigError, so the CLI reports it cleanly (no traceback). + with raises(AlmostServerError, match="almost-server.*validation errors") as exc_info: get_global_config_dict() + assert isinstance(exc_info.value, ConfigError) def test_almost_servers_error_flag_bypasses_value_error(self, monkeypatch: MonkeyPatch) -> None: """ From 5a3a09f5a5dfb23bb959208a463cd5457e5d1d29 Mon Sep 17 00:00:00 2001 From: Wojciech Prazuch Date: Mon, 29 Jun 2026 13:22:48 +0200 Subject: [PATCH 2/3] docs: also fix +dryrun -> +dry_run in the v0.3.0 release notes The latest release notes were corrected, but the same invalid `+dryrun=true` (the key is `dry_run`) remained in the frozen v0.3.0 copy, which is still served. This is a factual key typo (never valid), not CLI-version history, so fix it too. Signed-off-by: Wojciech Prazuch --- fern/versions/v0.3.0/pages/about/release-notes.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fern/versions/v0.3.0/pages/about/release-notes.mdx b/fern/versions/v0.3.0/pages/about/release-notes.mdx index f960c4762f..8d9df17b45 100644 --- a/fern/versions/v0.3.0/pages/about/release-notes.mdx +++ b/fern/versions/v0.3.0/pages/about/release-notes.mdx @@ -131,7 +131,7 @@ Added 5 new agent servers: Aviary agent, proof refinement agent, SWE agents, too ### Infrastructure & Developer Experience - PyPI compatibility: install via `pip install nemo-gym` -- Dry run mode: `ng_run +dryrun=true` to validate configs and install environments without starting servers +- Dry run mode: `ng_run +dry_run=true` to validate configs and install environments without starting servers - `ng_status` command to list running servers and their health - FastAPI worker support for higher throughput across multiple workers - Server stdout/stderr redirection with server name prefixes From e9eea33da02e2b6e3c5f35d875f420a6fdd64b8e Mon Sep 17 00:00:00 2001 From: Wojciech Prazuch Date: Mon, 29 Jun 2026 14:02:21 +0200 Subject: [PATCH 3/3] refactor: scope run_command project-root PYTHONPATH; reuse _resolve_server_dir Pre-review cleanups: - run_command no longer unconditionally injects work_dir.parent.parent into PYTHONPATH (a silent contract change on a publicly re-exported helper with a baked-in layout assumption). Add an opt-in `project_root` param; only `gym env test` (whose generated tests import `resources_servers....`) passes it. `gym env start`/`gym env packages` keep the original work_dir-only PYTHONPATH. - RunHelper.start now reuses _resolve_server_dir instead of an inline duplicate of the cwd-then-install-root heuristic. Signed-off-by: Wojciech Prazuch --- nemo_gym/cli/env.py | 14 ++++---- nemo_gym/cli/setup_command.py | 16 +++++---- tests/unit_tests/test_cli_setup_command.py | 42 +++++++++++++++------- 3 files changed, 47 insertions(+), 25 deletions(-) diff --git a/nemo_gym/cli/env.py b/nemo_gym/cli/env.py index 3492225f7b..2b4bcd910d 100644 --- a/nemo_gym/cli/env.py +++ b/nemo_gym/cli/env.py @@ -213,11 +213,8 @@ def start(self, global_config_dict_parser_config: GlobalConfigDictParserConfig) entrypoint_fpath = Path(server_config_dict.entrypoint) assert not entrypoint_fpath.is_absolute() - # Check cwd first for a local server, fall back to the install location for built-ins. - _server_rel_path = Path(first_key, second_key) - _cwd_path = Path.cwd() / _server_rel_path - _cwd_is_server = (_cwd_path / "requirements.txt").exists() or (_cwd_path / "pyproject.toml").exists() - dir_path = _cwd_path if _cwd_is_server else PARENT_DIR / _server_rel_path + # Resolve cwd-first (a local server), else the install location for built-ins. + dir_path = _resolve_server_dir(Path(first_key, second_key)) command = f"""{setup_env_command(dir_path, global_config_dict, top_level_path)} \\ && {NEMO_GYM_CONFIG_DICT_ENV_VAR_NAME}={escaped_config_dict_yaml_str} \\ @@ -556,8 +553,11 @@ def _validate_data_single(test_config: TestConfig) -> None: # pragma: no cover def _test_single(test_config: TestConfig, global_config_dict: DictConfig) -> Popen: # pragma: no cover # Eventually we may want more sophisticated testing here, but this is sufficient for now. prefix = test_config.entrypoint.replace("/", "\\/") - command = f"""{setup_env_command(test_config.resolved_dir_path, global_config_dict, prefix)} && pytest""" - return run_command(command, test_config.resolved_dir_path) + resolved_dir = test_config.resolved_dir_path + command = f"""{setup_env_command(resolved_dir, global_config_dict, prefix)} && pytest""" + # Generated server tests import `resources_servers....`, so the project root (the dir + # holding the server-type dirs) must be on PYTHONPATH when running from outside a repo checkout. + return run_command(command, resolved_dir, project_root=resolved_dir.parent.parent) def test(): # pragma: no cover diff --git a/nemo_gym/cli/setup_command.py b/nemo_gym/cli/setup_command.py index 75d47d95b6..4741f25457 100644 --- a/nemo_gym/cli/setup_command.py +++ b/nemo_gym/cli/setup_command.py @@ -172,16 +172,20 @@ def setup_env_command(dir_path: Path, global_config_dict: DictConfig, prefix: st return f"cd {dir_path} && {env_setup_cmd}" -def run_command(command: str, working_dir_path: Path, server_name: str = "") -> Popen: +def run_command( + command: str, working_dir_path: Path, server_name: str = "", project_root: Path | None = None +) -> Popen: global_config_dict = get_global_config_dict() work_dir = f"{working_dir_path.absolute()}" - # The server dir on PYTHONPATH lets `import app` work; the project root (the dir containing - # resources_servers/, responses_api_agents/, ...) lets generated `resources_servers..app` - # style imports resolve, which otherwise fail when running from outside a repo checkout. - project_root = f"{working_dir_path.absolute().parent.parent}" custom_env = environ.copy() - py_path_entries = [work_dir, project_root] + # The server dir on PYTHONPATH lets `import app` work. When a caller passes `project_root` (the + # dir containing resources_servers/, responses_api_agents/, ...), it's added so generated + # `resources_servers..app`-style imports resolve from outside a repo checkout — opt-in, so + # this generic helper doesn't bake a layout assumption in for its other callers. + py_path_entries = [work_dir] + if project_root is not None: + py_path_entries.append(f"{project_root.absolute()}") existing_py_path = custom_env.get("PYTHONPATH") if existing_py_path: py_path_entries.append(existing_py_path) diff --git a/tests/unit_tests/test_cli_setup_command.py b/tests/unit_tests/test_cli_setup_command.py index 1394db4d8f..26d3a83075 100644 --- a/tests/unit_tests/test_cli_setup_command.py +++ b/tests/unit_tests/test_cli_setup_command.py @@ -260,16 +260,15 @@ def test_sanity(self, monkeypatch: MonkeyPatch) -> None: run_command( command="my command", - working_dir_path=Path("/root/resources_servers/my_server"), + working_dir_path=Path("/my path"), ) expected_args = call( "my command", executable="/bin/bash", shell=True, - # The server dir plus the project root (its grandparent, the dir holding the server-type - # dirs) so `resources_servers.`-style imports resolve. - env={"PYTHONPATH": "/root/resources_servers/my_server:/root", "UV_CACHE_DIR": "default uv cache dir"}, + # Default (no project_root): only the server dir is on PYTHONPATH. + env={"PYTHONPATH": "/my path", "UV_CACHE_DIR": "default uv cache dir"}, stdout="stdout", stderr="stderr", ) @@ -280,19 +279,38 @@ def test_custom_pythonpath(self, monkeypatch: MonkeyPatch) -> None: Popen_mock, get_global_config_dict_mock = self._setup(monkeypatch) monkeypatch.setattr(nemo_gym.cli.setup_command, "environ", {"PYTHONPATH": "existing pythonpath"}) + run_command( + command="my command", + working_dir_path=Path("/my path"), + ) + + expected_args = call( + "my command", + executable="/bin/bash", + shell=True, + env={"PYTHONPATH": "/my path:existing pythonpath", "UV_CACHE_DIR": "default uv cache dir"}, + stdout="stdout", + stderr="stderr", + ) + actual_args = Popen_mock.call_args + assert expected_args == actual_args + + def test_project_root_added_to_pythonpath(self, monkeypatch: MonkeyPatch) -> None: + # Opt-in: callers that need `resources_servers.`-style imports (e.g. gym env test) pass + # the project root, which is appended after the server dir. + Popen_mock, get_global_config_dict_mock = self._setup(monkeypatch) + run_command( command="my command", working_dir_path=Path("/root/resources_servers/my_server"), + project_root=Path("/root"), ) expected_args = call( "my command", executable="/bin/bash", shell=True, - env={ - "PYTHONPATH": "/root/resources_servers/my_server:/root:existing pythonpath", - "UV_CACHE_DIR": "default uv cache dir", - }, + env={"PYTHONPATH": "/root/resources_servers/my_server:/root", "UV_CACHE_DIR": "default uv cache dir"}, stdout="stdout", stderr="stderr", ) @@ -306,14 +324,14 @@ def test_custom_uv_cache_dir(self, monkeypatch: MonkeyPatch) -> None: run_command( command="my command", - working_dir_path=Path("/root/resources_servers/my_server"), + working_dir_path=Path("/my path"), ) expected_args = call( "my command", executable="/bin/bash", shell=True, - env={"PYTHONPATH": "/root/resources_servers/my_server:/root", "UV_CACHE_DIR": "my uv cache dir"}, + env={"PYTHONPATH": "/my path", "UV_CACHE_DIR": "my uv cache dir"}, stdout="stdout", stderr="stderr", ) @@ -443,7 +461,7 @@ def test_tee_logs_with_server_name(self, monkeypatch: MonkeyPatch) -> None: "set -o pipefail; (my command) 2>&1 | tee -a /tmp/gym_logs/my_resources_my_server.log", executable="/bin/bash", shell=True, - env={"PYTHONPATH": "/root/resources_servers/my_server:/root", "UV_CACHE_DIR": "default uv cache dir"}, + env={"PYTHONPATH": "/root/resources_servers/my_server", "UV_CACHE_DIR": "default uv cache dir"}, stdout="stdout", stderr="stderr", ) @@ -467,7 +485,7 @@ def test_tee_logs_falls_back_to_dir_name(self, monkeypatch: MonkeyPatch) -> None "set -o pipefail; (my command) 2>&1 | tee -a /tmp/gym_logs/my_server.log", executable="/bin/bash", shell=True, - env={"PYTHONPATH": "/root/resources_servers/my_server:/root", "UV_CACHE_DIR": "default uv cache dir"}, + env={"PYTHONPATH": "/root/resources_servers/my_server", "UV_CACHE_DIR": "default uv cache dir"}, stdout="stdout", stderr="stderr", )