Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Unified Aero Recipe: offset manifest indices into the combined MultiDataset#1874

Open
peterdsharpe wants to merge 1 commit into
NVIDIA:mainNVIDIA/physicsnemo:mainfrom
peterdsharpe:psharpe/manifest-multidataset-offsetspeterdsharpe/physicsnemo:psharpe/manifest-multidataset-offsetsCopy head branch name to clipboard
Open

Unified Aero Recipe: offset manifest indices into the combined MultiDataset#1874
peterdsharpe wants to merge 1 commit into
NVIDIA:mainNVIDIA/physicsnemo:mainfrom
peterdsharpe:psharpe/manifest-multidataset-offsetspeterdsharpe/physicsnemo:psharpe/manifest-multidataset-offsetsCopy head branch name to clipboard

Conversation

@peterdsharpe

Copy link
Copy Markdown
Collaborator

What

Follow-up to #1859. Implements the offset-shifted manifest indices that
#1859 deliberately deferred, and removes the NotImplementedError guard
it added.

Why

Manifest split indices are local to a single reader, but the sampler
addresses the concatenated MultiDataset. Before #1859, choosing more
than one manifest dataset meant the last one's indices overwrote the
others and were applied to the whole concatenation — so training ran on
the wrong samples, silently.

I reproduced this on the pre-#1859 tree. Two manifest datasets, a (4
runs, val = run_0/run_1) and b (2 runs):

train: ['a/run_0/domain_0.pdmsh']    <-- a's held-out val run
val:   ['a/run_1/domain_1.pdmsh']

Training on a validation run, b never sampled at all, and no error.

#1859 correctly converted that into a loud failure. But every dataset
YAML in this recipe is manifest-mode — none defines a val_datadir
so the guard also blocks the multi-dataset recipe the README documents
(Transolver across DrivAerML + SHIFT SUV), leaving extra_datasets
unusable for any combination. #1859's docstring assumed the SHIFT SUV
datasets were directory-mode; they auto-discover a manifest.json next
to train_datadir.

How

Accumulate offset-shifted indices per dataset. Each dataset's local
train/val indices are shifted by its cumulative offset in the
corresponding MultiDataset. Train and val offsets are tracked
separately, because the two concatenations diverge whenever a dataset
contributes to one split but not the other (a directory dataset with no
val_datadir, or an augment=True run that builds a second
un-augmented reader for validation).

Directory-mode datasets contribute their full local ranges, so the two
modes can be combined in one run. The missing-val-split fallback moves
into the per-dataset loop, ahead of offsetting, so it can no longer
capture a neighbouring dataset's indices.

Same scenario on this branch:

train: ['a/run_2', 'a/run_3', 'b/run_0']
val:   ['a/run_0', 'a/run_1', 'b/run_1']

Also

resolve_manifest_spec now warns when a dataset falls back to directory
mode while a manifest.json sits next to its train_datadir. Clearing
the top-level split selectors is how a manifest/directory mix is
configured, so this is easy to reach — and left silent it sweeps the
manifest's held-out val/test runs into training.

Testing

examples/.../unified_external_aero_recipe/tests/: 165 passed (161 + 4).
New coverage: multi-manifest offsets, mixed manifest/directory ordering
in both orders, the per-dataset val fallback, distributed sharding over
the combined index space, and — the property that actually matters —
that indices resolve back through MultiDataset's own global → (dataset,
local) mapping to the runs the manifests name.

Mutation-checked: swapping val_offset for train_offset fails 6 tests,
including all the new ones.

Manifest split indices are local to a single reader, but the sampler
addresses the concatenated MultiDataset. With more than one manifest
dataset chosen, the last dataset's indices overwrote the others and
were then applied to the whole concatenation, so training silently ran
on the wrong samples -- including runs the manifest holds out for
validation.

NVIDIA#1859 made that combination fail loudly instead. Every dataset YAML in
this recipe is manifest-mode, though (none defines val_datadir), so the
guard also blocked the documented multi-dataset recipe (Transolver
across DrivAerML + SHIFT SUV) and left extra_datasets unusable.

Accumulate offset-shifted indices instead. Each dataset's local train
and val indices are shifted by its cumulative offset in the
corresponding MultiDataset, with train and val offsets tracked
separately because the two concatenations diverge whenever a dataset
contributes to one split but not the other. Directory-mode datasets
contribute their full local ranges, so the two modes can be combined in
one run. The missing-val-split fallback moves into the per-dataset
loop, ahead of offsetting, so it can no longer capture a neighbouring
dataset's indices.

Also warn when a dataset falls back to directory mode while a
manifest.json sits next to its train_datadir. Clearing the top-level
split selectors is how a manifest/directory mix is configured, so this
is easy to reach, and left silent it sweeps the manifest's held-out
val/test runs into training.

Tests cover multi-manifest offsets, mixed manifest/directory ordering
in both orders, the per-dataset val fallback, distributed sharding over
the combined index space, and that the resulting indices resolve to the
runs the manifests actually name.
@copy-pr-bot

copy-pr-bot Bot commented Jul 25, 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.

@github-actions

Copy link
Copy Markdown
Contributor

CODEOWNERS review map

Current for commit c5667ec402c7. An approval covers every file listed for that owner; one owner is sufficient for shared files.

@coreyjadams — 4 file(s)
  • examples/cfd/external_aerodynamics/unified_external_aero_recipe/README.md
  • examples/cfd/external_aerodynamics/unified_external_aero_recipe/conf/train.yaml
  • examples/cfd/external_aerodynamics/unified_external_aero_recipe/src/datasets.py
  • examples/cfd/external_aerodynamics/unified_external_aero_recipe/tests/test_manifest.py
@peterdsharpe — 4 file(s)
  • examples/cfd/external_aerodynamics/unified_external_aero_recipe/README.md
  • examples/cfd/external_aerodynamics/unified_external_aero_recipe/conf/train.yaml
  • examples/cfd/external_aerodynamics/unified_external_aero_recipe/src/datasets.py
  • examples/cfd/external_aerodynamics/unified_external_aero_recipe/tests/test_manifest.py

Comment /codeowners-info to refresh.

@peterdsharpe
peterdsharpe marked this pull request as ready for review July 25, 2026 13:51
@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR enables manifest- and directory-split datasets to be combined safely in a single MultiDataset.

  • Accumulates split-specific global sampler indices using independent training and validation offsets.
  • Applies missing-validation fallback per manifest dataset before offsetting.
  • Adds warnings for directory mode when a sibling manifest would be ignored.
  • Updates recipe documentation, configuration examples, and multi-dataset regression coverage.

Important Files Changed

Filename Overview
examples/cfd/external_aerodynamics/unified_external_aero_recipe/src/datasets.py Correctly maps each dataset's local split indices into the corresponding combined train and validation index spaces, including mixed split strategies.
examples/cfd/external_aerodynamics/unified_external_aero_recipe/tests/test_manifest.py Adds strong coverage for multiple manifests, mixed ordering, split-specific offsets, distributed sharding, fallback behavior, and resolved sample identity.
examples/cfd/external_aerodynamics/unified_external_aero_recipe/README.md Documents supported multi-dataset manifest behavior, mixed-mode configuration, and the consequences of unintentionally ignoring a manifest.
examples/cfd/external_aerodynamics/unified_external_aero_recipe/conf/train.yaml Updates configuration guidance and supplies a manifest-based multi-dataset training example consistent with the implementation.

Reviews (1): Last reviewed commit: "fix(examples): offset manifest indices i..." | Re-trigger Greptile

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.

1 participant

Morty Proxy This is a proxified and sanitized view of the page, visit original site.