fix(cli): fail fast when gym eval run --split has no matching dataset - #2911
Conversation
|
/ok to test 8f7f1d9 |
|
🌿 Preview your docs: https://nvidia-preview-fix-2685-eval-split-early-exit.docs.buildwithfern.com/nemo/gym Here are the markdown pages you've updated: |
gym eval run --split train against a config that declares no train-typed dataset (e.g. example_single_tool_call, which ships only an example dataset) previously walked the entire data-preparation sequence, printed its success banners, and only then died on a bare `assert input_jsonl_fpath.exists()` whose message was an internal path (and which disappears entirely under python -O). - Validate up front that some config declares a dataset of the requested split's type; error with the declared datasets and, for example datasets, the copy-pasteable --no-serve --input recipe from the docs. - Replace the bare assert with an explicit ConfigError backstop that lists which split files data preparation actually produced. - Reject --split example before the pydantic Literal check with the documented gym env start + gym eval run --no-serve --input flow, instead of a bare "Input should be 'train'". - Fix a latent bug in the serving-path --input rejection: the CLI passes an OmegaConf DictConfig, which is a Mapping but not a dict, so the isinstance(data, dict) guard never fired on the real path and input_jsonl_fpath was silently dropped — exactly the failure the validator was written to prevent. Match on Mapping instead. Fixes #2685 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Adil Asif <adasif@nvidia.com>
8f7f1d9 to
0ebf761
Compare
|
/ok to test 0ebf761 |
There was a problem hiding this comment.
Thanks for improving this failure mode. Failing before data preparation and replacing the user-facing assert with ConfigError are clear improvements.
I think the reported behavior also exposes a deeper semantic mismatch that is worth acknowledging, even if it is outside the scope of this fix. A command such as:
gym eval run --resources-server example_single_tool_call --split trainreads as “run the train split of this benchmark,” but a resources server is not itself a benchmark. It provides environment-side behavior such as tools, state, and verification, and it does not inherently need to own a train, validation, or benchmark dataset. In practice, the CLI composes several loaded configs and treats --split as a global request, with datasets potentially declared on an agent or resources server.
The validator in this PR correctly checks the complete loaded configuration rather than requiring the selected resources server itself to have the split. However, that also highlights that the CLI currently blurs three concepts:
- the resources server (tools/state/verifier),
- the dataset or benchmark tasks,
- the complete environment configuration that composes resources, agent, model, and datasets.
I think this PR is a worthwhile backward-compatible guard for the current interface, but we should avoid wording that implies every resources server is a benchmark or must provide the requested split. The contract is instead: some dataset in the complete loaded configuration must match --split.
As a follow-up, it may be worth tracking a clearer environment/dataset-oriented CLI interface, where users select an environment/config and a named dataset directly rather than inferring dataset ownership through --resources-server plus --split.
| f"Even though the `reuse_existing_data_preparation=true` flag was set, we will still do data preparation since the final input jsonl fpath `{input_jsonl_fpath}` does not exist yet" | ||
| ) | ||
|
|
||
| server_instance_configs = GlobalConfigDictParser().filter_for_server_instance_configs(global_config_dict) |
There was a problem hiding this comment.
Could we run _validate_split_datasets_declared() before the should_skip_data_processing branch? As written, when reuse_existing_data_preparation=true and the expected split file already exists, the declaration check is skipped. That appears to allow a stale prepared file to run even when the current loaded configuration no longer declares that split. Is that intentional? If reuse is expected to bypass this validation, should the new documentation qualify its statement that a matching dataset “must be declared”?
There was a problem hiding this comment.
Thanks! I'll fix that
ffrujeri
left a comment
There was a problem hiding this comment.
A few comments worth taking a look. but otherwise I think we can proceed
…g prepared data Hoist the declaration check above the reuse_existing_data_preparation branch. Reuse skips the preparation work, not the request validation, so a stale prepared file can no longer run for a split the loaded config no longer declares. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Adil Asif <adasif@nvidia.com>
|
/ok to test a5309d7 |
@adil-a, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test ee4fd8f |
…dataset (2911)` into `r0.6.0` (#2953) beep boop [🤖]: Hi @adil-a 👋, we've cherry picked #2911 into for you! 🚀 Please review and approve this cherry pick by your convenience! Signed-off-by: Adil Asif <adasif@nvidia.com> Signed-off-by: NeMo Bot <nemo-bot@nvidia.com> Co-authored-by: Adil <47084919+adil-a@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
Fixes #2685 (
[VDR][v0.6.0] ERR-91baf7d3).gym eval run --split trainagainst a config with notrain-typed dataset (e.g.example_single_tool_call, which ships only anexampledataset) walked the entire data-preparation banner sequence, printed "View your final data!" and "# Finished!", and only then exited 1 on a bareassert input_jsonl_fpath.exists()whose message was an internal path (and which disappears underpython -O).Changes
gym eval run --no-serve --input <server>/data/example.jsonl).ConfigErrorbackstop naming the split and the split files preparation actually produced (covers other missing-file causes: failed downloads, stalereuse_existing_data_preparation).--split examplerejection: caught before the pydanticLiteralcheck with the documentedgym env start+--no-serve --inputflow, instead of a bare "Input should be 'train'". Per the discussion on [VDR][v0.6.0] ERR-91baf7d3 ·gym eval run --splitwith no matching dataset prints "Finished!" then dies on a bare assert #2685, example datasets stay non-runnable via--split(they are the PR data gate's committed smoke-test samples); the suggested "let--inputsatisfy the serving path" change is deliberately not included — it reverses an intentional guard and deserves its own issue.DictConfig(aMapping, not adict), so the existingisinstance(data, dict)check in_reject_input_jsonl_fpathnever fired on the real path and-i/--inputwas silently dropped when serving — exactly the failure it was written to prevent. Now matches onMapping, with aDictConfigregression test.New behavior
Testing
pytest tests/unit_tests/: 1264 passed (new:test_cli_eval.py, extendedTestE2EInputJsonlFpathRejected, newTestE2EExampleSplitRejected).example_single_tool_call(output above; exit 1, no banners, no traceback).pre-commit runon changed files: clean.🤖 Generated with Claude Code