Skip to content

feat: resolve built-in data & prompt files from the install root - #1806

Merged
wprazuch merged 2 commits into
mainfrom
wprazuch/gym-install-paths
Jun 29, 2026
Merged

feat: resolve built-in data & prompt files from the install root#1806
wprazuch merged 2 commits into
mainfrom
wprazuch/gym-install-paths

Conversation

@wprazuch

@wprazuch wprazuch commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Problem

Follow-up to #1805. Relative dataset and prompt-config paths were resolved only against the current working directory (dataset reads) or only against the install root (prompt configs). So built-in files referenced by their repo-relative path didn't resolve from an arbitrary cwd, and a user's own relative --prompt-config was never found.

Part of epic #1205 (criterion C5, external-dependency/path-safety).

Changes

  • nemo_gym/__init__.py — add resolve_under_cwd_or_install(path): a shared read-path resolver. Absolute → unchanged; relative → tried under the cwd (the user's project) first, then the Gym install root (PARENT_DIR, where built-ins live in both editable and wheel installs). Mirrors config_paths resolution. Documented as read-only — never for write targets.
  • nemo_gym/prompt.py — _resolve_path now resolves cwd-first then install root (was install-root only). A user can pass a relative --prompt-config from their own project, and built-in prompt YAMLs still resolve by repo-relative path from any cwd.
  • nemo_gym/train_data_utils.py — the dataset existence-check and collation read use the resolver, so a bundled data/example.jsonl resolves from an external cwd. The metrics-write paths intentionally stay cwd-relative — writing _metrics.json into the install root (site-packages) would be wrong.

Tests

  • TestResolveUnderCwdOrInstall: absolute returned unchanged; cwd preferred over install root; falls back to install root; missing returns the cwd candidate.
  • test_relative_path_resolves_to_cwd_first for prompt configs.
  • Updated one train_data_utils mock that matched the old relative-string open() argument.

ruff clean; prompt + train_data_utils + cli_main + registry suites pass (only the 3 pre-existing TestDidYouMean argparse failures remain, unrelated).

Scope / follow-up

Complements #1805 (which packages built-in assets incl. example data into the wheel). The gym env test-from-an-external-cwd plumbing (test_all globs + setup_command PYTHONPATH) is intentionally deferred — that path is # pragma: no cover and requires reworking the test-execution resolution (TestConfig asserts a relative entrypoint; _test_single does cd {dir_path} relative to cwd), so it rides with the gym env test/validate hardening work.

This was referenced Jun 29, 2026
Merged
Closed
@wprazuch
wprazuch force-pushed the wprazuch/gym-install-paths branch from 10b8852 to cef1d0f Compare June 29, 2026 11:15
@copy-pr-bot

copy-pr-bot Bot commented Jun 29, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

prokotg
prokotg reviewed Jun 29, 2026
View reviewed changes
Comment thread nemo_gym/__init__.py Resolved
prokotg
prokotg reviewed Jun 29, 2026
View reviewed changes
Comment thread nemo_gym/prompt.py Resolved
@wprazuch
feat: resolve built-in data and prompt files from the install root
983251d 
Follow-up to the packaging/install-root work (#1805). Relative dataset and
prompt-config paths now resolve against the cwd first, then the Gym install
root, so built-in files referenced by their repo-relative path (e.g.
`resources_servers/<env>/data/example.jsonl`) work from any cwd, including a
wheel install (epic #1205, criterion C5; nvbug 6383277).

- add `nemo_gym.resolve_under_cwd_or_install(path)`: a shared read-path resolver
  (absolute as-is; else cwd, then install root), mirroring config_paths resolution
- prompt.py: `--prompt-config` now resolves cwd-first then install root (was
  install-root only, so a user's own relative prompt config was never found)
- train_data_utils: dataset existence-check and collation reads use the resolver
  so a bundled example dataset resolves from an external cwd; metrics-write paths
  intentionally stay cwd-relative (never write into the install root)

Tests: TestResolveUnderCwdOrInstall (absolute / cwd-preferred / install-fallback /
missing) plus a cwd-first prompt-config test.

Deferred to a follow-up (gym env test from an external cwd: test_all globs +
setup_command PYTHONPATH) — that path is # pragma: no cover and needs reworking
the test-execution resolution, so it rides with the gym env test/validate work.

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
@wprazuch
wprazuch force-pushed the wprazuch/gym-install-paths branch from cef1d0f to 983251d Compare June 29, 2026 11:31
prokotg
prokotg previously approved these changes Jun 29, 2026
View reviewed changes

@prokotg prokotg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some scoping comments, but approved otherwise

@wprazuch
refactor: mark the cwd/install path resolver private; drop the prompt…
5c4894e 
… wrapper

Address review (prokotg) on #1806:
- rename `resolve_under_cwd_or_install` -> `_resolve_under_cwd_or_install`: it's a
  Gym-internal helper, not end-user API, so it shouldn't be exposed at the package top level
- drop prompt.py's thin `_resolve_path` wrapper (it only delegated) and call
  `_resolve_under_cwd_or_install` directly; fold its test coverage into
  TestResolveUnderCwdOrInstall

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
@wprazuch
wprazuch merged commit 2628256 into main Jun 29, 2026
16 checks passed
@wprazuch
wprazuch deleted the wprazuch/gym-install-paths branch June 29, 2026 11:50
@wprazuch wprazuch mentioned this pull request Jun 29, 2026
Merged
wprazuch added a commit that referenced this pull request Jun 29, 2026
@wprazuch
fix: gym dataset collate creates the artifact dir when missing (run-f…
00fe01d 
…rom-wheel) (#1811)

## Summary

Final piece of the "use built-in components from a `pip install` of NeMo
Gym" work (epic #1205, criterion **C5**). After #1806 (merged) made a
dataset's **read** resolve to the install root, `gym dataset collate` on
a built-in dataset from an external cwd would then **crash on the
write**: the derived artifacts (`_metrics.json`, `_prepare.jsonl`) are
built from the raw, cwd-relative `jsonl_fpath` and `open(..., "w")` with
no `mkdir`, so they hit `FileNotFoundError` when that directory doesn't
exist in the cwd.

Fix: `mkdir(parents=True, exist_ok=True)` the artifact directory before
writing (the `output_dirpath` write was already guarded). For the common
case (collating your own dataset in your project) it's a no-op; it just
prevents the crash for the install-root-resolved case.

## Tests
- `test_collate_creates_missing_parent_dir` — collate auto-creates a
missing artifact parent instead of crashing.

ruff clean; `test_train_data_utils` + `test_cli_setup_command` green. 2
files, +26.

## Scope notes (verified, intentionally not in this PR)
- **`gym env start` of a built-in from a wheel already works** —
`setup_command`'s non-editable path installs `nemo-gym==<version>` from
the index; it just needs a published version (verified empirically). No
code change needed.
- **`gym env test` of a built-in from a wheel** — the per-server test
venv lacks `pytest` (it's only in the `[dev]` extra). Deliberately left
out: pulling all of `[dev]` (pre-commit/mypy/ruff + test deps) into
every test venv is too coarse, and running a built-in's tests from a
wheel is a contributor-in-repo edge case. Can be revisited with a narrow
`[test]` extra if needed.

---------

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
wprazuch added a commit that referenced this pull request Jun 29, 2026
@wprazuch
Merge branch 'main' into config-composer
533ea92 
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>
wprazuch added a commit that referenced this pull request Jun 29, 2026
@wprazuch
feat: clean config errors + run/test built-in servers from any cwd (#…
c83e4f2 
…1807)

## Summary

C4 validation hardening plus the `gym env test`-from-an-external-cwd
plumbing deferred from #1806. Part of epic #1205 (criteria **C4**
pre-start validation, **C5** external-cwd).

## Clean config errors (C4)

- Two raw `ValueError`s become `ConfigError` subclasses so they print
cleanly (no traceback) via `exit_cleanly_on_config_error` and `gym env
validate`, consistent with the #1609 error family:
- `InheritPathNotFoundError` — a `_inherit_from` / swap / copy directive
points at a missing config path (`global_config.py`).
- `AlmostServerError` — almost-servers detected with
`error_on_almost_servers` set.
- Docs: `release-notes.mdx` referenced `+dryrun=true`; the actual key is
`dry_run`.

## Run / test built-in servers from any cwd (C5)

- New `_resolve_server_dir` (cwd first, then the Gym install root) and
`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 (deduped, cwd shadows install).
- `setup_command` PYTHONPATH now includes the project root (the dir
holding the server-type dirs) so generated `resources_servers.<name>`
imports resolve from outside a checkout.

## Tests

- `ConfigError` assertion on the almost-server path.
- `_resolve_server_dir` (cwd-preferred / install-fallback) and
`TestConfig.resolved_dir_path`.
- Updated `run_command` PYTHONPATH expectations (now include the project
root).

ruff clean; `test_cli` / `test_cli_setup_command` / `test_global_config`
pass (115). The `gym env test` execution body is `# pragma: no cover`
(spawns per-server venvs); the resolution helpers it relies on are
unit-tested.

## Out of scope (documented in the tracking issue)

- **`extra="allow"` "typo field" detection** is **not** a simple
`forbid`: `BaseRunServerTypeConfig` intentionally allows arbitrary
fields because server authors add custom config their server reads, so
forbidding would break configs. A fuzzy "did you mean `host`?" *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 <wprazuch@nvidia.com>
wprazuch added a commit that referenced this pull request Jun 29, 2026
@wprazuch
feat(cli): config_composer + gym env compose for agent/dataset compos…
38e90c0 
…ition (RFC M3)

Compose a benchmark with a swapped agent harness and/or edited benchmark-dataset
params, without writing a bespoke per-combination config (epic #1205, criterion C3):
- nemo_gym/config_composer.py: compose(), substitute_agent() (wires the env's
  resources_server/model_server/datasets into the swapped agent, filling a generic
  agent's mandatory resources_server.name), find_agent_block_key,
  substitute_dataset_params, _validate_no_mandatory_placeholders (scans for remaining
  ??? WITHOUT resolving interpolations), and a composability guard (self-contained
  Pattern B agents rejected).
- gym env compose command + an agent asset selector in the unified CLI.
- docs: document gym env compose in the CLI reference.

Rebased onto current main (composes with #1805/#1806/#1807): #1807's env.py
install-root resolution + validate are preserved; gym env compose and gym env
validate coexist as separate commands. Verified: compose --benchmark gsm8k --agent
simple_agent wires resources_server; validate works; env-test resolution intact.

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
wprazuch added a commit that referenced this pull request Jun 29, 2026
@wprazuch
feat(cli): config_composer + gym env compose for agent/dataset compos…
72ef9e6 
…ition (RFC M3)

Compose a benchmark with a swapped agent harness and/or edited benchmark-dataset
params, without writing a bespoke per-combination config (epic #1205, criterion C3):
- nemo_gym/config_composer.py: compose(), substitute_agent() (wires the env's
  resources_server/model_server/datasets into the swapped agent, filling a generic
  agent's mandatory resources_server.name), find_agent_block_key,
  substitute_dataset_params, _validate_no_mandatory_placeholders (scans for remaining
  ??? WITHOUT resolving interpolations), and a composability guard (Pattern B rejected).
- gym env compose command + an agent asset selector in the unified CLI.
- docs: document gym env compose in the CLI reference.

Rebased onto current main (composes with #1805/#1806/#1807): #1807's env.py
install-root resolution + validate preserved; gym env compose and gym env validate
coexist. Verified: compose --benchmark gsm8k --agent simple_agent wires
resources_server; validate works; env-test resolution intact.

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
wprazuch added a commit that referenced this pull request Jun 29, 2026
@wprazuch
feat(cli): config_composer + gym env compose for agent/dataset compos…
1cb55e6 
…ition (RFC M3)

Compose a benchmark with a swapped agent harness and/or edited benchmark-dataset
params, without writing a bespoke per-combination config (epic #1205, criterion C3):
- nemo_gym/config_composer.py: compose(), substitute_agent() (wires the env's
  resources_server/model_server/datasets into the swapped agent, filling a generic
  agent's mandatory resources_server.name), find_agent_block_key,
  substitute_dataset_params, _validate_no_mandatory_placeholders (scans for remaining
  ??? WITHOUT resolving interpolations), and a composability guard (Pattern B rejected).
- gym env compose command + an agent asset selector in the unified CLI.
- docs: document gym env compose in the CLI reference.

Rebased onto current main (composes with #1805/#1806/#1807): #1807's env.py
install-root resolution + validate preserved; gym env compose and gym env validate
coexist. Verified: compose --benchmark gsm8k --agent simple_agent wires
resources_server; validate works; env-test resolution intact.

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
@linj-glitch linj-glitch mentioned this pull request Oct 15, 2026
Merged
cmunley1 pushed a commit to Anai-Guo/Gym that referenced this pull request Oct 15, 2026
@linj-glitch @claude
fix(core): preserve import precedence in non-editable installs (NVIDI…
0ef51ca 
…A-NeMo#4004)

`import nemo_gym` can let a dependency checkout in cwd shadow installed
packages, even under `python -I`.
This fix reuses `_is_editable_install` to preserve existing import order
for wheels and `--target` installs, including explicit server paths,
while retaining source-checkout and file-lookup precedence from NVIDIA-NeMo#2061
and NVIDIA-NeMo#1806.
Subprocess tests cover dependency shadowing, server sibling imports,
nonstandard install layouts, and explicit built-in overrides; the
configuration reference directs users to `--search-dir .` when
overriding both files and modules.
Validation passed 378 focused tests, 18 documentation link tests,
`pre-commit run --all-files`, and `make docs-check`; regression checks
fail on the prior implementations.
This reproduced core import bug needs no separate issue or model
rollout, and full mirrored CI plus Ananth's renewed review remain
required.

---------

Signed-off-by: Lin Jia <linj@nvidia.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@qiaochuz-nv qiaochuz-nv mentioned this pull request Oct 15, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@prokotg prokotg prokotg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wprazuch @prokotg