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#1874peterdsharpe wants to merge 1 commit intoNVIDIA:mainNVIDIA/physicsnemo:mainfrom peterdsharpe:psharpe/manifest-multidataset-offsetspeterdsharpe/physicsnemo:psharpe/manifest-multidataset-offsetsCopy head branch name to clipboard
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
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.
Contributor
CODEOWNERS review mapCurrent for commit ⏳ @coreyjadams — 4 file(s)
⏳ @peterdsharpe — 4 file(s)
Comment |
peterdsharpe
marked this pull request as ready for review
July 25, 2026 13:51
Contributor
Greptile SummaryThis PR enables manifest- and directory-split datasets to be combined safely in a single
Important Files Changed
Reviews (1): Last reviewed commit: "fix(examples): offset manifest indices i..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #1859. Implements the offset-shifted manifest indices that
#1859 deliberately deferred, and removes the
NotImplementedErrorguardit added.
Why
Manifest split indices are local to a single reader, but the sampler
addresses the concatenated
MultiDataset. Before #1859, choosing morethan 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(4runs, val =
run_0/run_1) andb(2 runs):Training on a validation run,
bnever 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_datasetsunusable for any combination. #1859's docstring assumed the SHIFT SUV
datasets were directory-mode; they auto-discover a
manifest.jsonnextto
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 trackedseparately, because the two concatenations diverge whenever a dataset
contributes to one split but not the other (a directory dataset with no
val_datadir, or anaugment=Truerun that builds a secondun-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:
Also
resolve_manifest_specnow warns when a dataset falls back to directorymode while a
manifest.jsonsits next to itstrain_datadir. Clearingthe 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_offsetfortrain_offsetfails 6 tests,including all the new ones.