Repository navigation
feat(eval): add the Exporter base class and registry, with atif as the first exporter - #4911
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
svcnvidia-nemo-ci
added
area:evaluation
|
/ok to test 4a51f87 |
|
🌿 Preview your docs: https://nvidia-preview-pr-4911-24c71ffb44ab.docs.buildwithfern.com/nemo/gym |
4a51f87 to
4ca8fdc
Compare
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — three RISK findings affect CLI override compatibility and the exporter's failure-result contract.
Reviewed all 11 changed files and their directly affected parser, configuration, and ATIF export paths. No BLOCKER findings. This was a static review; tests were not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
| exporter = args.exporter | ||
| # argparse can capture the first bare Hydra override (`rollouts_jsonl_fpath=...`) in the optional EXPORTER slot. | ||
| if exporter and _is_positional_hydra_override(exporter): | ||
| overrides = [exporter, *overrides] |
There was a problem hiding this comment.
RISK — Restore captured overrides before combining them with translated flags
Failure: With a valid rollout and fresh output, gym eval export ++session_id=override --session-id original --rollouts r.jsonl --output-dir out --agent-version 1.2.3 should export using session ID override. Instead, the captured ++session_id=override is prepended ahead of the translated +session_id='original'; Hydra creates the key with ++, then rejects the later + because it already exists. This regresses the previous router's flag-before-raw-override ordering. Supplying explicit atif avoids the failure, so default-exporter users receive inconsistent behavior.
Correction: Move this restoration into main(), beside the existing TARGET restoration, before translated + overrides, and remove the restoration block here:
if command.target is _eval_export and args.exporter and _is_positional_hydra_override(args.exporter):
overrides = [args.exporter, *overrides]
args.exporter = NoneThe invariant is that translated flags precede every raw override, including the token captured by the optional positional.
Preservation / acceptance (proposed, not run): Run the example above and its explicit-atif equivalent with separate fresh outputs; both must succeed and write session_id == 'override'. Retain the exact default, explicit-plus, and dotted-key dispatch assertions in the exporter argument tests. Leave _eval_run restoration and its folder/deletion disambiguation unchanged.
There was a problem hiding this comment.
Reproduced and fixed in 2c69447. The restoration now lives in main() beside the TARGET handling, so translated flags precede every raw override. gym eval export ++session_id=override --session-id original ... and the explicit-atif form both succeed and write session_id == 'override'. Added a dispatch-order test.
| if exporter not in RUN_EXPORTERS: | ||
| args._parser.error(f"unknown exporter {exporter!r}; choose from: {', '.join(RUN_EXPORTERS)}") | ||
| # Exporter settings are new config keys, which Hydra only accepts with a leading `+`; let users omit it. | ||
| overrides = [f"+{token}" if re.match(r"[A-Za-z_][\w.]*=", token) else token for token in overrides] |
There was a problem hiding this comment.
RISK — Do not add + to existing Hydra configuration overrides
Failure: A normal Hydra control such as gym eval export atif hydra.run.dir=/tmp/export-log --rollouts r.jsonl --output-dir out --session-id s --agent-version 1.2.3 is rewritten to +hydra.run.dir=/tmp/export-log. Hydra already defines this string-valued key and rejects an add-only override, so the command fails before export. Previously, this control passed through unchanged. The same problem affects other existing hydra.* settings.
Correction: Limit implicit prefixing to exporter/application settings; preserve Hydra's reserved namespace and explicitly supplied operators. Replace the comprehension at this normalization point with:
overrides = [
f"+{token}"
if re.match(r"[A-Za-z_][\w.]*=", token)
and not token.startswith(("hydra.", "hydra="))
else token
for token in overrides
]Preservation / acceptance (proposed, not run): With valid input and a fresh destination, the example must compose successfully and export normally. Assert its dispatched token remains hydra.run.dir=/tmp/export-log, without +. Bare rollouts_jsonl_fpath=r.jsonl and meta.k=v must still acquire +; explicit +a=1, ++a=1, and deletion tokens must remain unchanged. Retain the exact explicit-plus/dotted-key assertion in the existing test.
There was a problem hiding this comment.
Reproduced and fixed in 2c69447. Implicit + prefixing now skips hydra.* (your suggested condition), so hydra.run.dir=/tmp/log is dispatched unchanged; bare rollouts_jsonl_fpath=... and meta.k=v still get +, and ++a=1 and ~b pass through. Added a test asserting the exact dispatched tokens and ran the hydra.run.dir example through the CLI.
| config = self.config.model_copy(update={"rollouts_jsonl_fpath": artifacts.rollouts_jsonl_fpath}) | ||
| try: | ||
| result = export_rollouts_to_atif(config) | ||
| except AtifExportError as exc: |
There was a problem hiding this comment.
RISK — Return failed results for ordinary filesystem failures
Failure: Create an existing rollout file and a regular file named blocked, then export to blocked/atif. The converter's output-parent mkdir raises FileExistsError, which escapes AtifExporter.export() rather than returning FAILED. Unreadable input and unwritable output similarly escape as OSError. These are expected operational failures, not programming bugs, contrary to the new Exporter contract. Library callers lose structured failure handling, and the CLI produces a traceback instead of its failed-export report.
Correction: At this adapter boundary, replace the handler with:
except (AtifExportError, OSError) as exc:
return ExportResult.failed(str(exc))Keep unexpected programming exceptions propagating; do not catch every Exception or change the converter's publication/cleanup logic.
Preservation / acceptance (proposed, not run): For the regular-file parent setup, assert FAILED, exit code 1, a nonempty filesystem diagnostic, and no published destination. Also check a converter-raised PermissionError becomes FAILED, while a synthetic RuntimeError still propagates. Retain the existing malformed-input assertions: exit 1, invalid JSON, no output, and the artifact-supplied missing filename rather than the config's ignored path. A valid two-rollout export must still succeed with count 2 and the hash-verified manifest behavior asserted by the converter regression test.
There was a problem hiding this comment.
Reproduced and fixed in 2c69447. AtifExporter.export() now catches (AtifExportError, OSError); the regular-file-parent case returns FAILED with exit code 1 and the filesystem message, and nothing is published. Tests cover that case and a PermissionError becoming FAILED, while a RuntimeError still propagates. The converter and its cleanup are unchanged.
|
/ok to test 2c69447 |
svcnvidia-nemo-ci
added
the
needs-review
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — all three prior RISK findings are resolved; no eligible new findings.
Verified against the reviewed baseline and current source:
- Captured override restoration now occurs before combining translated flags with raw overrides, preserving flag-before-override precedence.
- Override normalization preserves reserved Hydra settings and explicit operators while prefixing bare application settings.
- ATIF failure handling returns failed results for filesystem errors without catching programming exceptions or changing converter cleanup.
Inspected all 11 changed files. Follow-up discovery covered changes since the reviewed baseline and their directly affected paths, retaining the historical moderate-impact threshold. Reviewed the CLI regression assertions and exporter failure assertions. This was a static review; tests were not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a findings-only review. Request a full review with /review reset=true.
2c69447 to
c4c599e
Compare
0d0899b to
c32e7c0
Compare
c32e7c0 to
d8241be
Compare
d8241be to
93e5ebf
Compare
|
/review |
…e first exporter gym eval export now takes an optional exporter name (default atif) and key=value settings. The ATIF conversion runs as an Exporter that returns an ExportResult; --format is deprecated. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…stem errors in gym eval export A captured override now follows the translated flags, hydra.* settings are not given a + prefix, and the ATIF exporter returns a failed result for OSError instead of raising. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
… flag Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
…other commands gym eval export no longer adds + to bare key=value overrides, so it follows the Hydra convention the CLI reference describes (+key=value). The --format deprecation goes through the logger, and the atif settings docstring names the exporter instead of the deprecated flag. Signed-off-by: Tomasz Grzegorzek <tgrzegorzek@nvidia.com>
93e5ebf to
24c71ff
Compare
|
/ok to test 24c71ff |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/40684638314 |
Summary
Exporterbase class,ExportStateandExportResult(nemo_gym/exporters/base.py); the liveBaseExporteris untouched.run_exporter_registry.py, names only sogym --helpimports no backend) andload_run_exporter_class.AtifExporterwraps the ATIF conversion; conversion and filesystem errors come back asfailedresults.RunExportConfig(sharedrollouts_jsonl_fpath), extended byExportAtifConfig.gym eval export [EXPORTER] +key=value ....atifis the default; settings are plain Hydra overrides like in the other commands (a barekey=valuegets Hydra's own "use+" hint);--formatis deprecated, with its warning going through the logger; the old--rollouts,--output-dir,--session-idand--agent-versionflags still work. CLI reference and the trajectory capability page updated.Behavior change: success now prints
Export atif: succeededplus detail lines instead of "Exported N ATIF trajectory file(s) ...".Part of #2977 (export-aware evaluation runs).
PR plan
Separate PRs; steps 1-8 are stacked (stack #4910). Goal: make
gym eval exportpluggable so a finished run can go to tracking systems without live exporters, andgym eval submitcan export after each benchmark.gym eval run(feat(eval): save the masked config beside rollouts from gym eval run #4908)RunArtifactsand path derivation helpers (feat(eval): add RunArtifacts to derive a run's files from its rollouts path #4909)Exporterbase class and registry, starting withatif, plus the positional exporter CLI argument (--formatdeprecated)mlflowandwandbexport: shared metrics flattening,BaseExporter.export()and the already-exported guard (feat(eval): export a finished run to MLflow or W&B with gym eval export #4914)gym eval submitintegration: anexportmap with job-level defaults and per-benchmark overrides, validation, install extras, and a job-script tail (Slurm only at first) (feat(orchestration): run exports after each benchmark of gym eval submit #4974)Validation
ExportResult, the registry, the ATIF exporter's failure results (includingOSError) and CLI parsing (default and positional exporter,--format, unknown exporter, overrides passed through unchanged,++override order).pre-commit: passed.failed, exit 1;--format atifand++session_id=...work. Hydra typesagent_version=1as an int, so use a string such as1.0.0.Rollout evidence
N/A: no agent, verifier or model behavior is touched. The conversion code is unchanged.