Skip to content

feat(config): unify dataset source via discriminated source: block (FEP-1025) - #1637

Merged
wprazuch merged 6 commits into
mainfrom
wprazuch/dataset-source
Jun 25, 2026
Merged

feat(config): unify dataset source via discriminated source: block (FEP-1025)#1637
wprazuch merged 6 commits into
mainfrom
wprazuch/dataset-source

Conversation

@wprazuch

@wprazuch wprazuch commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

What

Replaces the two parallel identifier fields on DatasetConfig
gitlab_identifier and huggingface_identifier — with a single, self-describing
discriminated source: block:

# GitLab
source:
  type: gitlab
  dataset_name: my_dataset
  version: 0.0.1
  artifact_fpath: train.jsonl

# HuggingFace
source:
  type: huggingface
  repo_id: org/dataset
  artifact_fpath: train.jsonl   # optional

The type discriminator makes it unambiguous which fields apply, instead of two
optional sibling blocks where only one may be set.

Backward compatibility

Fully non-breaking — no consumer changes required:

  • A legacy *_identifier is accepted, mirrored into source, and emits a
    DeprecationWarning.
  • A source: is back-filled into the matching legacy field, so existing code
    that reads gitlab_identifier / huggingface_identifier keeps working unchanged.
  • Specifying both source and a legacy identifier is rejected with a clear error.
  • Specifying neither (local jsonl_fpath only) remains valid.

Tests

  • tests/unit_tests/test_dataset_source.py — 7 cases covering both backends, the
    legacy↔source mirroring (incl. deprecation warning), the both-set rejection, the
    no-source path, and discriminator validation.
  • Updated test_train_data_utils.py serialization fixture for the new source field.

Part of the configuration-friction epic (#1205).

@copy-pr-bot

copy-pr-bot Bot commented Jun 17, 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.

@wprazuch
wprazuch force-pushed the wprazuch/ng-test-concurrency branch from 2fdee46 to eebfcf8 Compare June 22, 2026 13:45
@wprazuch
wprazuch requested a review from a team as a code owner June 22, 2026 13:45
@wprazuch
wprazuch force-pushed the wprazuch/dataset-source branch 2 times, most recently from 695207e to b024b01 Compare June 24, 2026 07:01
@wprazuch
wprazuch changed the base branch from wprazuch/ng-test-concurrency to wprazuch/cli-clean-base June 24, 2026 07:02
@wprazuch wprazuch closed this Jun 24, 2026
@wprazuch wprazuch reopened this Jun 24, 2026
@wprazuch
wprazuch force-pushed the wprazuch/dataset-source branch 2 times, most recently from cdcbd27 to 2905987 Compare June 24, 2026 07:18
@wprazuch
wprazuch changed the base branch from wprazuch/cli-clean-base to martas/1434 June 24, 2026 07:18
@github-actions

Copy link
Copy Markdown
Contributor

@wprazuch
wprazuch force-pushed the wprazuch/dataset-source branch from 2905987 to 6ca5c48 Compare June 24, 2026 07:21
wprazuch added a commit that referenced this pull request Jun 24, 2026
…a) (#1638)

## What

Generated resources-server configs (`ng_init_resources_server`) now
carry inline
comments explaining each non-obvious field — `domain`,
`resources_server`, the
`policy_model` magic name, and the `datasets`/`source:` block — so new
users
understand the scaffold without leaving the file. Addresses friction #7
(no inline
documentation in generated configs).

While here, the scaffold now emits the canonical `source:` dataset block
instead of
the deprecated `gitlab_identifier`, so a freshly created server starts
on the
recommended schema (depends on #1637).

## Notes

- Chose inline comments over an indirected `FIELD_DOCS` constant
(suggested in the
RFC): there is a single generation site, so a one-line-per-field
constant would add
indirection without reuse. Easy to extract later if a second site
appears.

## Tests

- Extended `test_init_resources_server_includes_domain` to assert the
generated config
(a) contains the inline docs, (b) uses `source: {type: gitlab}` rather
than
`gitlab_identifier`, and (c) validates cleanly with no
`DeprecationWarning`.

Part of the configuration-friction epic (#1205), milestone M6a. Targets
`wprazuch/dataset-source` since it builds on the `source:` schema; will
retarget to
the shared base once that merges.

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
@anwithk
anwithk requested a review from ananthsub June 24, 2026 16:30
ananthsub
ananthsub previously approved these changes Jun 24, 2026
Comment thread resources_servers/mcqa/configs/mcqa.yaml Outdated
type: train
jsonl_fpath: resources_servers/example_multi_step/data/train.jsonl
gitlab_identifier:
# Unified dataset source. `type` selects the backend (gitlab | huggingface); the remaining

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.

in case more backends are added later, it'll be tedious to update all of these comments

Suggested change
# Unified dataset source. `type` selects the backend (gitlab | huggingface); the remaining
# Unified dataset source. `type` selects the backend; the remaining

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@anwithk anwithk linked an issue Jun 25, 2026 that may be closed by this pull request
5 tasks
@wprazuch
wprazuch force-pushed the wprazuch/dataset-source branch from c64574e to 35864ee Compare June 25, 2026 08:20
Base automatically changed from martas/1434 to main June 25, 2026 10:50
@marta-sd
marta-sd dismissed ananthsub’s stale review June 25, 2026 10:50

The base branch was changed.

wprazuch and others added 6 commits June 25, 2026 13:27
Replace the parallel gitlab_identifier / huggingface_identifier fields on
DatasetConfig with a single self-describing source: block (type selects the
backend). Legacy fields keep working: a legacy identifier is mirrored into
source (with a DeprecationWarning), and a source: is back-filled into the
matching legacy field so existing consumers that read the *_identifier fields
are unaffected. Specifying both is rejected.

Addresses FEP-1025 (reduce configuration friction, epic #1205).

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
…a) (#1638)

## What

Generated resources-server configs (`ng_init_resources_server`) now
carry inline
comments explaining each non-obvious field — `domain`,
`resources_server`, the
`policy_model` magic name, and the `datasets`/`source:` block — so new
users
understand the scaffold without leaving the file. Addresses friction #7
(no inline
documentation in generated configs).

While here, the scaffold now emits the canonical `source:` dataset block
instead of
the deprecated `gitlab_identifier`, so a freshly created server starts
on the
recommended schema (depends on #1637).

## Notes

- Chose inline comments over an indirected `FIELD_DOCS` constant
(suggested in the
RFC): there is a single generation site, so a one-line-per-field
constant would add
indirection without reuse. Easy to extract later if a second site
appears.

## Tests

- Extended `test_init_resources_server_includes_domain` to assert the
generated config
(a) contains the inline docs, (b) uses `source: {type: gitlab}` rather
than
`gitlab_identifier`, and (c) validates cleanly with no
`DeprecationWarning`.

Part of the configuration-friction epic (#1205), milestone M6a. Targets
`wprazuch/dataset-source` since it builds on the `source:` schema; will
retarget to
the shared base once that merges.

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
…_step

Rewrite the example_multi_step datasets from the deprecated gitlab_identifier: blocks to the new
unified source: block (type: gitlab), with an inline comment showing the huggingface form. Serves
as a worked example of the new dataset-source syntax. Verified end to end: ng_dump_config merges
the config, and each dataset validates via DatasetConfig (source: parsed, legacy gitlab_identifier
back-filled so existing consumers keep working).

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
…ource: example

The unified source: validator wrongly rejected datasets that set both gitlab_identifier and
huggingface_identifier together. That combo is a supported gitlab-primary / huggingface-fallback
pair (backend chosen at download time via config.data_source, see train_data_utils.py), and
existing configs such as mcqa rely on it. Make source: mutually exclusive with the legacy
identifiers only; keep two legacy identifiers together valid (source: left unset since the single
discriminated block cannot represent both). Add a regression test.

Also rewrite the mcqa validation dataset to the new source: {type: huggingface} block as a worked
huggingface example. Verified end to end: ng_dump_config merges the config and every mcqa dataset
validates via DatasetConfig (train keeps both legacy fields, validation back-fills
huggingface_identifier from source:).

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
Co-authored-by: Ananth Subramaniam <ansubramania@nvidia.com>
Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
… review)

Apply ananthsub's review suggestion to example_multi_step.yaml — don't enumerate
'(gitlab | huggingface)' in the inline comment, since it goes stale as backends are added (mirrors
the same suggestion already applied to mcqa.yaml via the web UI).

Also remediate the missing DCO sign-off on that web-applied suggestion commit, which was committed
through GitHub without a Signed-off-by line:

I, Wojciech Prazuch <wprazuch@nvidia.com>, hereby add my Signed-off-by to this commit: b7b0d55

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
@wprazuch
wprazuch force-pushed the wprazuch/dataset-source branch from 35864ee to f96315d Compare June 25, 2026 11:31

@ananthsub ananthsub 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.

.

@wprazuch
wprazuch merged commit cd48916 into main Jun 25, 2026
16 checks passed
@wprazuch
wprazuch deleted the wprazuch/dataset-source branch June 25, 2026 11:40
@ritaneves ritaneves linked an issue Jun 25, 2026 that may be closed by this pull request
1 task
wprazuch added a commit that referenced this pull request Jun 25, 2026
…tion #12) (#1599)

## What

Adds **`gym env validate`** (+ `ng_validate` / `nemo_gym_validate`
deprecated shims) — runs the full config parse with **no Ray and no
server subprocesses**, then exits **0 (valid) / 1 (invalid)** with a
clean, rich-escaped message (**no traceback**). Returns in well under a
second instead of after a ~30–60s Ray bootstrap.

```bash
gym env validate --config resources_servers/<env>/configs/<env>.yaml --config responses_api_models/<model>/configs/<model>.yaml
gym env validate --benchmark gsm8k --model-type openai_model
```

## How

`validate()` lives in `cli/env.py` and is registered as `env validate`
in the `gym` router (`cli/main.py` COMMANDS) with the same
config-selection flags as `env start` (`--config`, `--benchmark`,
`--environment`, `--resources-server`, `--model-type`, `--search-dir`,
`--model*`). It reuses the same `get_global_config_dict()` parse path
the other commands use, so the validation checks stay in sync:

- **config_paths** resolution — missing/typo'd
([#1488](#1488)) and malformed
([#1490](#1490))
- **server cross-references** — unknown `name:` refs
([#1561](#1561))
- **mandatory `???`** values
([#1575](#1575))
- **schema** (`BaseNeMoGymCLIConfig`)

Wrapped in `exit_cleanly_on_config_error` (from #1609) so any
`ConfigError` becomes a clean message + `exit 1`. A dummy `policy_model`
is injected (the `NO_MODEL` parser config, as in `gym list` / `env
compose`) so model interpolations like `${policy_base_url}` resolve
without real creds — validation is about config **well-formedness**; the
real model is supplied by the `--model*` flags at run time.

## Targets `main`

Originally drafted on the unified-CLI epic branch; rebuilt directly on
`main` now that [#1630](#1630)
(and #1637/#1609/#1635/#1671) have merged. The old branch contents (a
snapshot of the CLI refactor + unrelated CI commits) were superseded and
replaced.

## Scope note

The zero-server check
([#1489](#1489), "nothing
configured to run") is intentionally **not** part of `validate`:
`NO_MODEL` injects a dummy model server (which would defeat the check),
and "is anything configured to run" is a *start*-time concern already
enforced by `gym env start` before Ray init. `validate` focuses on
config well-formedness.

## Why

Epic [#1205](#1205) friction
#12 (no config validation tooling) — the M1 "fast failure triage"
deliverable. Config errors otherwise only surface after Ray starts
(~30–60s).

## Tests

- `test_cli_main.py`: `gym env validate --config X` routes to
`nemo_gym.cli.env:validate` with `+config_paths=[X]` (added to the
parametrized config-command matrix).
- `test_cli.py`: `validate()` prints OK on a valid config; a raised
`ConfigError` becomes `exit 1` (no traceback).
- All `test_cli` + `test_cli_main` + `test_cli_legacy` pass (the only
failures are the pre-existing Python-3.12 `TestDidYouMean` argparse
issue on `main`); ruff + pre-commit clean. Smoke-tested end-to-end: `✓
Config is valid.` on a real benchmark, clean error + `exit 1` on a bad
path, and the `ng_validate` deprecation shim.

---------

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
ritaneves pushed a commit that referenced this pull request Jun 25, 2026
…(FEP-1025) (#1637)

## What

Replaces the two parallel identifier fields on `DatasetConfig` —
`gitlab_identifier` and `huggingface_identifier` — with a single,
self-describing
discriminated `source:` block:

```yaml
# GitLab
source:
  type: gitlab
  dataset_name: my_dataset
  version: 0.0.1
  artifact_fpath: train.jsonl

# HuggingFace
source:
  type: huggingface
  repo_id: org/dataset
  artifact_fpath: train.jsonl   # optional
```

The `type` discriminator makes it unambiguous which fields apply,
instead of two
optional sibling blocks where only one may be set.

## Backward compatibility

Fully non-breaking — no consumer changes required:

- A **legacy `*_identifier`** is accepted, mirrored into `source`, and
emits a
  `DeprecationWarning`.
- A **`source:`** is back-filled into the matching legacy field, so
existing code
that reads `gitlab_identifier` / `huggingface_identifier` keeps working
unchanged.
- Specifying **both** `source` and a legacy identifier is rejected with
a clear error.
- Specifying **neither** (local `jsonl_fpath` only) remains valid.

## Tests

- `tests/unit_tests/test_dataset_source.py` — 7 cases covering both
backends, the
legacy↔source mirroring (incl. deprecation warning), the both-set
rejection, the
  no-source path, and discriminator validation.
- Updated `test_train_data_utils.py` serialization fixture for the new
`source` field.

Part of the configuration-friction epic (#1205).

---------

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
Co-authored-by: Ananth Subramaniam <ansubramania@nvidia.com>
Signed-off-by: Rita Fernandes Neves <rfernandesne@nvidia.com>
wprazuch added a commit that referenced this pull request Jun 29, 2026
…CLI commands (#1754)

Documents the config/CLI features merged for epic #1205 that were not
yet reflected in the docs. Requested follow-up: a single doc PR covering
the recently merged config/CLI work.


## What's documented

| Area | Change | Merged in |
|---|---|---|
| `reference/cli-commands.mdx` | New `gym list environments`, `gym list
agents`, and `gym env validate` sections (+ Quick-Reference entries) |
#1635, #1671, #1599 |
| `troubleshooting/configuration.mdx` | Three `config_paths` startup
errors (Config Path Not Found, Malformed `config_paths`, No Server
Instances) + a `gym env validate` pre-flight tip; corrected the stale
`ServerRefNotFound` message | #1609, #1599 |
| `data/index.mdx` | Unified dataset `source:` block + dedicated
"Dataset `source`" subsection + legacy-deprecation note | #1637 |
| `data/download-huggingface.mdx` | Auto-download example migrated to
`source: { type: huggingface }` | #1637 |
| `reference/faq.mdx` | Dataset example and field bullets rewritten
around `source:` | #1637 |
| `environment-tutorials/single-step-environment.mdx` | Scaffold comment
updated to reference `source:` | #1637 |

---------

Signed-off-by: Wojciech Prazuch <wprazuch@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 - Configuration Friction epic: reduce configuration friction for environment authors and users

2 participants