Skip to content

feat(data): expose shuffle in DataloaderConfig - #5142

Open
filaretov wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
filaretov:expose-shuffle-dataset-attribute
Open

feat(data): expose shuffle in DataloaderConfig#5142
filaretov wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
filaretov:expose-shuffle-dataset-attribute

Conversation

@filaretov

Copy link
Copy Markdown

What does this PR do ?

This PR exposes shuffle as an argument in the top-level dataset configuration API so it can be set in MegatronPretrainingBatchSampler. It was added in #4601 and it changed the convergence behavior of our Llama 2 70b LORA benchmark.

Changelog

  • Add shuffle to necessary config classes to expose it through DataloaderConfig.

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?
  • Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
    • Reviewer: Does the PR have correct import guards for all optional libraries?

If you haven't finished some of the above items you can still open "Draft" PR.

@copy-pr-bot

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

@yaoyu-33 yaoyu-33 added area:data Dataset builders, preprocessing, and samplers community-request feature New capabilities, enhancements, or enablement work needs-review PR is ready for code review and waiting on a reviewer labels Jul 28, 2026
@filaretov
filaretov marked this pull request as draft July 28, 2026 15:27
@yaoyu-33

Copy link
Copy Markdown
Contributor

@filaretov this is still in draft, kindly remind to "ready for review" if need merge

Signed-off-by: Hristo Filaretov <hfilaretov@nvidia.com>
@filaretov
filaretov force-pushed the expose-shuffle-dataset-attribute branch from fecae65 to 90319dc Compare July 30, 2026 14:18
@filaretov
filaretov marked this pull request as ready for review July 30, 2026 14:18
@filaretov

Copy link
Copy Markdown
Author

@yaoyu-33 Thank you for the reminder. Yeah, it'd be nice to merge it if possible, it's ready for review.

@yaoyu-33

Copy link
Copy Markdown
Contributor

I think this is addressing the right user need at the wrong configuration layer.

After #4601, MegatronPretrainingBatchSampler already performs deterministic per-epoch shuffling by default (shuffle=True, seeded by epoch), while dataloader_type="cyclic" selects MegatronPretrainingRandomSampler. For GPTSFTDatasetConfig, however, batch is intentional: its collate_fn must see the full global batch to determine variable-length padding before the training loop splits it into microbatches. Switching SFT to cyclic is therefore not an equivalent way to control shuffling.

If the goal is simply the normal policy, I do not think we need a public DataloaderConfig.shuffle: keep the batch sampler, enable shuffle for the train loader, and explicitly disable it for validation/test. If users genuinely need to opt out of the new #4601 training order (for example, to reproduce the older LoRA benchmark), the option should be train-only or sampler-specific rather than a shared dataset flag. The current patch passes the same value to train, validation, and test, so the default True also explicitly shuffles evaluation, while setting it to False changes all three streams together.

Could we revise this to one of these shapes?

  1. No public option: shuffle=True for the batch train loader and shuffle=False for batch validation/test; or
  2. A clearly train-scoped option such as train_shuffle, used only for the train loader, with validation/test always false.

The focused tests should cover the resulting contract: shuffled train order remains deterministic and resume-equivalent, the train-only opt-out (if retained) is sequential, and validation/test order is deterministic and unshuffled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:data Dataset builders, preprocessing, and samplers community-request feature New capabilities, enhancements, or enablement work needs-review PR is ready for code review and waiting on a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants