Skip to content

Commit 533ea92

Browse files
committed
Merge branch 'main' into config-composer
Resolve PR #1673 conflicts with main (now includes #1804/#1805/#1806). The gym env compose work coexists with #1805's install-root _asset_config_path and the agent asset selector in nemo_gym/cli/main.py; both `env compose` and `env validate` remain registered. Auto-merge verified: no conflict markers, 167 unit tests pass (only the pre-existing TestDidYouMean argparse failures), and `gym env compose --benchmark gsm8k --agent simple_agent` + `gym env validate` both work. Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
2 parents 94d92c4 + 00fe01d commit 533ea92

8 files changed

Lines changed: 180 additions & 35 deletions

File tree

nemo_gym/__init__.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,31 @@
3737

3838
sys.path.append(str(PARENT_DIR))
3939

40+
41+
def _resolve_under_cwd_or_install(path) -> Path:
42+
"""Resolve a possibly-relative path for *reading* a built-in or user-supplied file.
43+
44+
Absolute paths are returned unchanged. A relative path is tried first under the current working
45+
directory (the user's project), then under the Gym install root (``PARENT_DIR``) where built-in
46+
assets live in both editable and wheel installs. This mirrors ``config_paths`` resolution, so a
47+
repo-relative path like ``resources_servers/<env>/data/example.jsonl`` resolves by name from any
48+
cwd. If neither exists the cwd candidate is returned so error messages point at the user's cwd.
49+
50+
Use this for read paths only — never for write targets (e.g. metrics written next to a dataset),
51+
which must stay relative to the user's writable cwd rather than the install root.
52+
"""
53+
p = Path(path)
54+
if p.is_absolute():
55+
return p
56+
cwd_candidate = Path.cwd() / p
57+
if cwd_candidate.exists():
58+
return cwd_candidate
59+
install_candidate = PARENT_DIR / p
60+
if install_candidate.exists():
61+
return install_candidate
62+
return cwd_candidate
63+
64+
4065
# TODO: Maybe eventually we want an override for OMP_NUM_THREADS ?
4166

4267
# Turn off HF tokenizers paralellism

nemo_gym/cli/main.py

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
from dataclasses import dataclass, field
2222
from pathlib import Path
2323

24-
from nemo_gym import WORKING_DIR
24+
from nemo_gym import PARENT_DIR, WORKING_DIR
2525

2626

2727
VERSION_TARGET = "nemo_gym.cli.general:version"
@@ -173,22 +173,34 @@ def _bool_flag(name: str, hydra_key: str, flag_help: str) -> Flag:
173173
def _asset_config_path(flag: str, value: str, search_dirs: tuple[str, ...] = ()) -> str:
174174
"""Map a named asset (`name` or `name/flavor`) to its config path.
175175
176-
Searches WORKING_DIR (built-ins) first, then any user-registered --search-dir roots.
176+
Searches the Gym install root (`PARENT_DIR` — where the built-in asset trees live in both
177+
editable and wheel installs), then the current working directory (the user's project), then
178+
any user-registered --search-dir roots. Searching `PARENT_DIR` is what lets built-ins resolve
179+
by name from an arbitrary cwd (e.g. a wheel install), not just from inside the repo checkout.
177180
"""
178181
parent, subdir, default_flavor = _ASSETS[flag]
179182
server_name, _, config_flavor = value.partition("/")
180183
config_flavor = config_flavor or default_flavor or server_name
181184
config_dir = f"{parent}/{server_name}/{subdir}".rstrip("/")
182185
path = f"{config_dir}/{config_flavor}.yaml"
183186

184-
# Match in WORKING_DIR (built-ins) and every --search-dir root; dedupe roots that resolve to the same file.
185-
roots = [WORKING_DIR, *(Path(d) for d in search_dirs)]
187+
# Search the install root (built-ins) and the user's cwd / --search-dir roots; dedupe roots that
188+
# resolve to the same directory so an editable install run from the repo root isn't searched twice.
189+
seen_roots: set[Path] = set()
190+
roots: list[Path] = []
191+
for root in (PARENT_DIR, WORKING_DIR, Path.cwd(), *(Path(d) for d in search_dirs)):
192+
resolved_root = root.resolve()
193+
if resolved_root not in seen_roots:
194+
seen_roots.add(resolved_root)
195+
roots.append(root)
186196
matches: list[Path] = []
187197

188198
for root in roots:
189199
candidate = root / path
190200
if candidate.exists():
191-
matches.append(candidate.resolve())
201+
resolved = candidate.resolve()
202+
if resolved not in matches:
203+
matches.append(resolved)
192204

193205
if len(matches) > 1:
194206
matches_str = ", ".join(f"`{m}`" for m in matches)

nemo_gym/prompt.py

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
import yaml
2828
from pydantic import BaseModel, Field
2929

30-
from nemo_gym import PARENT_DIR
30+
from nemo_gym import _resolve_under_cwd_or_install
3131
from nemo_gym.config_types import BaseNeMoGymCLIConfig
3232

3333

@@ -38,26 +38,18 @@ class PromptConfig(BaseModel):
3838
system: Optional[str] = None
3939

4040

41-
def _resolve_path(path: str) -> Path:
42-
"""Resolve a path relative to the Gym root (PARENT_DIR), consistent with config_paths resolution."""
43-
p = Path(path)
44-
if not p.is_absolute():
45-
p = PARENT_DIR / p
46-
return p
47-
48-
4941
@lru_cache(maxsize=64)
5042
def load_prompt_config(path: str) -> PromptConfig:
5143
"""Load and validate a YAML prompt config file.
5244
53-
Relative paths are resolved against the Gym root directory (``PARENT_DIR``),
54-
consistent with how ``config_paths`` and other Gym paths are resolved.
45+
Relative paths are resolved against the current working directory first, then the Gym install
46+
root, consistent with how ``config_paths`` and other Gym paths are resolved.
5547
5648
Returns a ``PromptConfig`` with required ``user`` and optional ``system`` fields.
5749
Each value is a string template with ``{placeholder}`` syntax.
5850
Results are cached so the same file is only parsed once.
5951
"""
60-
resolved = _resolve_path(path)
52+
resolved = _resolve_under_cwd_or_install(path)
6153
with open(resolved) as f:
6254
data = yaml.safe_load(f)
6355
return PromptConfig.model_validate(data)
@@ -125,7 +117,7 @@ def materialize_prompts(input_jsonl: str, prompt_config: str, output_jsonl: str)
125117
output_jsonl: Path to write materialized JSONL (with responses_create_params.input).
126118
"""
127119
prompt_cfg = load_prompt_config(prompt_config)
128-
resolved_prompt_path = str(_resolve_path(prompt_config))
120+
resolved_prompt_path = str(_resolve_under_cwd_or_install(prompt_config))
129121
output_path = Path(output_jsonl)
130122
output_path.parent.mkdir(parents=True, exist_ok=True)
131123

nemo_gym/train_data_utils.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
from pydantic import BaseModel, ConfigDict, Field, ValidationError
2626
from tqdm.auto import tqdm
2727

28+
from nemo_gym import _resolve_under_cwd_or_install
2829
from nemo_gym.base_resources_server import BaseRunRequest
2930
from nemo_gym.config_types import (
3031
AGENT_REF_KEY,
@@ -424,7 +425,9 @@ def load_datasets(
424425
local_datasets_not_found: Dict[str, List[DatasetConfig]] = defaultdict(list)
425426
for c in server_instance_configs:
426427
for d in c.datasets:
427-
jsonl_fpath = Path(d.jsonl_fpath)
428+
# Read check: a built-in dataset path resolves under the install root too, so a
429+
# bundled example dataset counts as "found" (no download) from an external cwd.
430+
jsonl_fpath = _resolve_under_cwd_or_install(d.jsonl_fpath)
428431
if jsonl_fpath.exists():
429432
local_datasets_found[c.name].append(d)
430433
else:
@@ -523,7 +526,7 @@ def _iter_dataset_lines(self, dataset_config: DatasetConfig):
523526
)
524527

525528
# Don't load everything into memory at once. Throw things away immediately.
526-
with open(dataset_config.jsonl_fpath) as f:
529+
with open(_resolve_under_cwd_or_install(dataset_config.jsonl_fpath)) as f:
527530
for line in tqdm(f, desc=f"{dataset_config.jsonl_fpath}"):
528531
for _ in range(repeats):
529532
yield line
@@ -661,6 +664,9 @@ def validate_samples_and_aggregate_metrics(
661664
else:
662665
continue
663666

667+
# Ensure the artifact dir exists: the metrics file is written next to the dataset's
668+
# (cwd-relative) jsonl_fpath, which may not exist yet when collating from a fresh cwd.
669+
metrics_fpath.parent.mkdir(parents=True, exist_ok=True)
664670
with open(metrics_fpath, "w") as f:
665671
json.dump(aggregate_metrics_dict, f, indent=4)
666672

@@ -706,6 +712,9 @@ def _collate_samples_single_type(
706712

707713
data_path = Path(d.jsonl_fpath)
708714
prepare_path = data_path.with_name(f"{data_path.stem}_prepare.jsonl")
715+
# Create the artifact dir if needed (the prepared file is written next to the
716+
# cwd-relative jsonl_fpath, which may not exist when collating from a fresh cwd).
717+
prepare_path.parent.mkdir(parents=True, exist_ok=True)
709718
with open(prepare_path, "w") as target:
710719
for line in self._iter_dataset_lines(d):
711720
d = json.loads(line)

pyproject.toml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,12 @@ exclude-dependencies = [
329329
]
330330

331331
[tool.setuptools.package-data]
332+
# The built-in asset trees (resources_servers, responses_api_agents, responses_api_models,
333+
# benchmarks, environments — all under [tool.setuptools.packages.find]) ship their configs, READMEs,
334+
# and example data into the wheel via the setuptools-scm VCS file-finder, which includes git-tracked
335+
# files. (Adding `environments` to packages.find above is what makes that tree ship at all.) Bulk
336+
# train/validation JSONL is intentionally not committed to git, so it is excluded automatically.
337+
# Only nemo_gym's own non-package resource scripts need an explicit entry here.
332338
nemo_gym = ["resources/*.py"]
333339

334340
[tool.setuptools.dynamic]
@@ -445,6 +451,8 @@ include = [
445451
"responses_api_agents.*",
446452
"responses_api_models",
447453
"responses_api_models.*",
454+
"environments",
455+
"environments.*",
448456
"nemo_gym",
449457
"nemo_gym.*",
450458
]

tests/unit_tests/test_cli_main.py

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -958,6 +958,75 @@ def test_did_you_mean_spans_user_dir(self, monkeypatch: MonkeyPatch, tmp_path, c
958958
assert "Did you mean `mybench`?" in capsys.readouterr().err
959959

960960

961+
class TestInstallRootResolution:
962+
"""Built-in assets resolve from the Gym install root regardless of cwd (REQ C2/C5).
963+
964+
In a wheel install PARENT_DIR is site-packages (where the asset trees are installed) while the
965+
user runs from an unrelated project dir, so WORKING_DIR/cwd is not the install root. The selector
966+
must still find built-ins under PARENT_DIR. Editable-from-repo keeps working because PARENT_DIR,
967+
WORKING_DIR, and cwd all coincide and dedupe to a single root.
968+
"""
969+
970+
def _make_resources_server(self, root, name: str = "foo") -> None:
971+
config_dir = root / "resources_servers" / name / "configs"
972+
config_dir.mkdir(parents=True)
973+
(config_dir / f"{name}.yaml").write_text("{}\n")
974+
975+
def test_builtin_resolves_from_install_root_when_cwd_differs(self, monkeypatch: MonkeyPatch, tmp_path) -> None:
976+
install_root = tmp_path / "site-packages"
977+
user_cwd = tmp_path / "my-project"
978+
install_root.mkdir()
979+
user_cwd.mkdir()
980+
self._make_resources_server(install_root) # built-in only under the install root
981+
monkeypatch.setattr(cli_main, "PARENT_DIR", install_root)
982+
monkeypatch.setattr(cli_main, "WORKING_DIR", user_cwd)
983+
monkeypatch.chdir(user_cwd)
984+
985+
resolved = cli_main._asset_config_path("resources-server", "foo")
986+
assert resolved == str(install_root / "resources_servers" / "foo" / "configs" / "foo.yaml")
987+
988+
def test_user_cwd_asset_resolves_when_not_builtin(self, monkeypatch: MonkeyPatch, tmp_path) -> None:
989+
install_root = tmp_path / "site-packages"
990+
user_cwd = tmp_path / "my-project"
991+
install_root.mkdir()
992+
user_cwd.mkdir()
993+
self._make_resources_server(user_cwd, name="myenv") # exists only in the user's project
994+
monkeypatch.setattr(cli_main, "PARENT_DIR", install_root)
995+
monkeypatch.setattr(cli_main, "WORKING_DIR", user_cwd)
996+
monkeypatch.chdir(user_cwd)
997+
998+
resolved = cli_main._asset_config_path("resources-server", "myenv")
999+
assert resolved == str(user_cwd / "resources_servers" / "myenv" / "configs" / "myenv.yaml")
1000+
1001+
def test_same_name_in_install_root_and_cwd_is_ambiguous(self, monkeypatch: MonkeyPatch, tmp_path) -> None:
1002+
# A user asset shadowing a built-in of the same name is ambiguous; they must disambiguate with --config.
1003+
install_root = tmp_path / "site-packages"
1004+
user_cwd = tmp_path / "my-project"
1005+
install_root.mkdir()
1006+
user_cwd.mkdir()
1007+
self._make_resources_server(install_root)
1008+
self._make_resources_server(user_cwd)
1009+
monkeypatch.setattr(cli_main, "PARENT_DIR", install_root)
1010+
monkeypatch.setattr(cli_main, "WORKING_DIR", user_cwd)
1011+
monkeypatch.chdir(user_cwd)
1012+
1013+
with pytest.raises(ValueError, match="ambiguous"):
1014+
cli_main._asset_config_path("resources-server", "foo")
1015+
1016+
def test_editable_layout_single_root_not_self_ambiguous(self, monkeypatch: MonkeyPatch, tmp_path) -> None:
1017+
# Editable install: PARENT_DIR == WORKING_DIR == cwd. The same file found via all three roots
1018+
# must dedupe to one match, not raise a spurious ambiguity error.
1019+
repo_root = tmp_path / "Gym"
1020+
repo_root.mkdir()
1021+
self._make_resources_server(repo_root)
1022+
monkeypatch.setattr(cli_main, "PARENT_DIR", repo_root)
1023+
monkeypatch.setattr(cli_main, "WORKING_DIR", repo_root)
1024+
monkeypatch.chdir(repo_root)
1025+
1026+
resolved = cli_main._asset_config_path("resources-server", "foo")
1027+
assert resolved == str(repo_root / "resources_servers" / "foo" / "configs" / "foo.yaml")
1028+
1029+
9611030
class TestListEnvironmentsRouting:
9621031
def test_list_environments_dispatches(self, monkeypatch: MonkeyPatch) -> None:
9631032
target, overrides = _dispatch_for(monkeypatch, ["list", "environments"])

tests/unit_tests/test_prompt.py

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,9 @@
1818
import pytest
1919
import yaml
2020

21-
from nemo_gym import PARENT_DIR
21+
from nemo_gym import PARENT_DIR, _resolve_under_cwd_or_install
2222
from nemo_gym.prompt import (
2323
PromptConfig,
24-
_resolve_path,
2524
apply_prompt_to_row,
2625
fill_prompt,
2726
load_prompt_config,
@@ -73,21 +72,29 @@ def test_caching(self, tmp_path):
7372
assert result1 is result2
7473

7574

76-
class TestResolvePath:
77-
def test_absolute_path(self, tmp_path):
78-
p = tmp_path / "prompt.yaml"
79-
p.write_text("")
80-
assert _resolve_path(str(p)) == p
75+
class TestResolveUnderCwdOrInstall:
76+
def test_absolute_returned_unchanged(self, tmp_path):
77+
assert _resolve_under_cwd_or_install(str(tmp_path / "x.yaml")) == tmp_path / "x.yaml"
8178

82-
def test_relative_path_resolves_to_parent_dir(self, tmp_path):
83-
# Create a file relative to PARENT_DIR to test resolution
84-
test_file = PARENT_DIR / "test_resolve_path_temp.yaml"
85-
test_file.write_text("user: '{q}'")
79+
def test_cwd_preferred_over_install_root(self, tmp_path, monkeypatch):
80+
monkeypatch.chdir(tmp_path)
81+
(tmp_path / "rel.yaml").write_text("{}")
82+
assert _resolve_under_cwd_or_install("rel.yaml") == tmp_path / "rel.yaml"
83+
84+
def test_falls_back_to_install_root(self, tmp_path, monkeypatch):
85+
# cwd lacks the file; a file present under the install root resolves there.
86+
monkeypatch.chdir(tmp_path)
87+
rel = "test_resolve_install_fallback.yaml"
88+
install_file = PARENT_DIR / rel
89+
install_file.write_text("{}")
8690
try:
87-
resolved = _resolve_path("test_resolve_path_temp.yaml")
88-
assert resolved.exists()
91+
assert _resolve_under_cwd_or_install(rel) == install_file
8992
finally:
90-
test_file.unlink()
93+
install_file.unlink()
94+
95+
def test_missing_returns_cwd_candidate(self, tmp_path, monkeypatch):
96+
monkeypatch.chdir(tmp_path)
97+
assert _resolve_under_cwd_or_install("nope.yaml") == tmp_path / "nope.yaml"
9198

9299

93100
class TestFillPrompt:

tests/unit_tests/test_train_data_utils.py

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import nemo_gym.global_config
2323
import nemo_gym.train_data_utils
24+
from nemo_gym import _resolve_under_cwd_or_install
2425
from nemo_gym.config_types import DatasetConfig, ResponsesAPIAgentServerInstanceConfig
2526
from nemo_gym.global_config import DictConfig, GlobalConfigDictParser
2627
from nemo_gym.train_data_utils import (
@@ -576,7 +577,9 @@ def custom_open(filename, mode="r"):
576577
write_filenames.append(filename)
577578
return mock_write_file()
578579

579-
if filename == "resources_servers/example_multi_step/data/example.jsonl":
580+
if Path(filename) == _resolve_under_cwd_or_install(
581+
"resources_servers/example_multi_step/data/example.jsonl"
582+
):
580583
return original_open(filename, mode)
581584
elif filename == Path("resources_servers/example_multi_step/data/example_metrics.json"):
582585
with original_open(filename, mode) as f:
@@ -1117,6 +1120,26 @@ def custom_open(filename, mode="r"):
11171120
Path("example.jsonl"),
11181121
]
11191122

1123+
def test_collate_creates_missing_parent_dir(self, tmp_path: Path, monkeypatch: MonkeyPatch) -> None:
1124+
# The prepared-output file is written next to the dataset's jsonl_fpath. When that dir does not
1125+
# exist in the cwd (e.g. a built-in dataset resolved from the install root while collating from
1126+
# a fresh cwd), the write must create the parent instead of crashing with FileNotFoundError.
1127+
missing_dir = tmp_path / "does" / "not" / "exist"
1128+
assert not missing_dir.exists()
1129+
cfg = _make_agent_instance_config(
1130+
"ex", [{"name": "example", "type": "example", "jsonl_fpath": str(missing_dir / "data.jsonl")}]
1131+
)
1132+
processor = TrainDataProcessor()
1133+
# Bypass the dataset read so the source file isn't needed; we're exercising the write path.
1134+
monkeypatch.setattr(processor, "_iter_dataset_lines", lambda d: iter(['{"foo": "bar"}']))
1135+
1136+
paths = processor._collate_samples_single_type("example", [cfg])
1137+
1138+
prepare_path = missing_dir / "data_prepare.jsonl"
1139+
assert paths == [prepare_path]
1140+
assert prepare_path.exists() # parent dir auto-created; write did not crash
1141+
assert json.loads(prepare_path.read_text().strip())["foo"] == "bar"
1142+
11201143
def test_collate_samples_metrics_conflict_raises_ValueError(self, monkeypatch: MonkeyPatch) -> None:
11211144
write_filenames_to_mock = dict()
11221145

0 commit comments

Comments
 (0)