feat(training): support multiple validation sets and eval at start - #5076
feat(training): support multiple validation sets and eval at start#5076MoPl90 wants to merge 14 commits into
Conversation
…mputation Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…tion Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for adding these two features. I found four correctness issues around validation iterator shape, checkpoint resume, and step-zero evaluation bookkeeping; details are inline.
| global_batch_size=eval_gbs, | ||
| seed=sampler_seed, | ||
| valid_dataloader = _build_eval_test_dataloaders( | ||
| valid_ds, train_state.consumed_valid_samples, val_dataloader_type |
There was a problem hiding this comment.
This applies the single aggregate consumed_valid_samples offset to every validation set. evaluate() increments that scalar for every set, so after N sets each individual loader has consumed only 1/N of the recorded total. After checkpoint resume, every loader skips the full aggregate offset (for example, 5 sets x 2 eval iterations makes each set resume at 10 iterations rather than 2), silently changing the evaluated samples or repeating cyclic data. MCore currently refuses multiple validation sets without full validation for this exact reason. Please either persist per-set offsets, or require and fully implement full_validation, and add save/resume coverage.
| if isinstance(valid_dataloader, list): | ||
| valid_data_iterator = [_get_iterator(val_dataloader_type, dl) for dl in valid_dataloader] | ||
| else: | ||
| valid_data_iterator = _get_iterator(val_dataloader_type, valid_dataloader) |
There was a problem hiding this comment.
When multiple_validation_sets=True but the provider returns a single validation dataset, this branch produces one RerunDataIterator. evaluate_and_print_results() then assumes the flag means a sequence of sets and calls len() on it, but RerunDataIterator has no __len__ or __iter__, so the first validation fails. The built-in blended builder does return a single dataset when the validation blend has one prefix. Please normalize it to a one-element list whenever the flag is enabled, or reject that configuration early, and cover the singleton case.
| energy_monitor.resume() | ||
|
|
||
| if val_config.eval_at_start and global_state.train_state.do_valid: | ||
| _run_validation( |
There was a problem hiding this comment.
This extra validation pass is not included in get_train_valid_test_num_samples(): the existing formula reserves samples for interval evaluations plus the final evaluation only. For train_iters=10, eval_interval=5, and eval_iters=2, it builds 6 validation iterations but this change executes 8. A cyclic loader hides the shortage by reusing early samples, while a single-pass loader can exhaust early. Please add one eval_iters * eval_global_batch_size allocation when the pre-train pass can run.
|
|
||
| if val_config.eval_at_start and global_state.train_state.do_valid: | ||
| _run_validation( | ||
| f"iteration {global_state.train_state.step} (pre-train validation)", |
There was a problem hiding this comment.
This condition also runs at a nonzero resumed step and on every NVRx in-process retry, because recovery re-enters train() with the restored step. That duplicates validation/logging and advances validation state during recovery. Issue #3996 proposed guarding this with train_state.step == 0. If ordinary checkpoint resume should intentionally evaluate, please distinguish it from an automatic in-process retry; otherwise add the step-zero guard and a nonzero-step test.
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…validation_sets Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…ide per-set counters Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
What does this PR do ?
Implements #3996 (eval at start; approved by @yaoyu-33) and #618 (multiple validation sets + separate loss reporting), bringing Bridge's training loop to parity with Megatron-LM's
multiple_validation_sets/validation_set_namesconvention. Bridge inherits both fields inValidationConfig, andBlendedMegatronDatasetBuilderalready returns a list of validation datasets when the flag is set, but Bridge's loaders and eval cannot consume it today (likely related to the crash reported in #2101).Changelog
training/config.py: add BridgeValidationConfigsubclass (same pattern asTrainingConfig) witheval_at_start: bool = Falsetraining/train.py: extract the in-loop eval into_run_validation()(all existing logic preserved one-to-one); called once before the loop wheneval_at_startanddo_validare settraining/eval.py: per-set evaluation and suffixed logging inevaluate_and_print_results()for the supported loggers (TensorBoard / W&B / MLflow / Comet); cross-rank dataset-count guard; raisesValueErroronvalidation_set_nameslength mismatchdata/loaders.py: folds the three duplicated valid/test dataloader build sites into_build_eval_test_dataloaders(), mapping over dataset lists; errors when a provider returns multiple validation datasets without the flagDesign notes
evaluate()calls per eval round, so the per-set loop cannot desync pipeline collectivesevaluate_and_print_resultsand the eval plumbing intrain()/loaders.py, so most of the diff (the_run_validation()extraction, the per-set eval restructuring, the test scaffolding) would be needed for either feature alone. They are also used together in practice: the motivating use case is a step-0 baseline per validation set, and the end-to-end evidence below exercises exactly that combinationTesting
Unit tests
test_eval.py: indexed and named suffixes; exact logger calls (metric names, ppl, step) on TensorBoard/W&B;validation_set_nameslength mismatch raises before any evaluation; timelimit mid-loop aborts remaining sets; a list without the flag is evaluated once (VPP); rank set-count agreement passes / mismatch raises;is_testnever splits a list.test_loaders.py: one dataloader per set with correct per-set contents; iterator layer yields oneRerunDataIteratorper set in blend order; guard error without the flag.test_config.py:eval_at_startdefault/settable;multiple_validation_setsvalidates under PP 1 and 2.End-to-end training
Ran 6 short training jobs (train_iters=4, eval_interval=2, eval_iters=2) with eval_at_start=true and multiple_validation_sets=true on a blend of 5 validation datasets, using dense Qwen3.5-VL models (2B and 9B sizes). All 6 finished and logged one loss series per validation set, starting at iteration 0. The matrix covers:
Validation on start. With eval_at_start=true, all validation sets are evaluated before the first training step and logged at iteration 0. From the 2B run with named sets:
Multi-dataset validation. Each set is evaluated independently at every eval_interval and at the end of training, printed as its own block with the set suffix. From the 9B TP2/PP2 run (default index naming, final validation):
GitHub Actions CI
See the CI section in the Contributing doc for how to trigger the CI. A Nvidia developer will need to approve and trigger the CI for external contributors.
Before your PR is "Ready for review"
Pre checks:
Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)Additional Information
multiple_validation_setsinGPTDatasetConfigresults in aZeroDivisionErrorwhen entering in a validation cycle #2101feature,area:training,needs-review