Repository navigation
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.
There was a problem hiding this comment.
As I outline below, I opted for the full bookkeeping approach instead of full_validation. See my comment below for details.
| 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>
|
Thank you for the review and spotting those correctness issues. I will address your inline comments directly. Regarding the
Moreover, I added unit tests and
Happy to address any remaining concerns! |
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for the thorough revision — all four items from my previous round are genuinely addressed, and the approach you picked for the per-set offsets (real bookkeeping in TrainState rather than requiring full_validation) is the right call. The save/resume functional test with two sets and dataloader_type="single" is exactly the coverage that was missing.
I also checked two things that are easy to get wrong here, and you got both right, so no action needed:
- PP collective balance holds.
evaluate()returns{}rather thanNoneoff the last pipeline stage, and the onlyreturn Nonepath is the timelimit one, which is gated on anall_reduce(MAX). So the earlyreturn Noneinside the per-set loop cannot leave ranks with unequalevaluate()counts. _run_validation(toggle_pre_hook=False)is correct for the pre-train pass. The forward pre-hook is already disabled attrain.py:338before your insertion point, andinterval-timeis started attrain.py:189, so thetimers("interval-time").stop()inside the helper will not trip MCore'sassert self._started.
One blocking issue and a few smaller ones inline. The PR is also currently conflicting with main and will need a rebase.
| "The dataset provider returned multiple validation datasets but " | ||
| "ValidationConfig.multiple_validation_sets is not set." | ||
| ) | ||
| if cfg.validation.multiple_validation_sets and valid_ds is not None and not isinstance(valid_ds, list): |
There was a problem hiding this comment.
Blocking: the MegatronMIMO path bypasses all of this.
build_train_valid_test_data_loaders() returns early for MegatronMIMOProvider at line 259, which is above this normalization block and above the consumed_valid_samples_per_set sizing at line 400. If a user sets multiple_validation_sets=True on a MIMO model, three things go wrong:
train_state.consumed_valid_samples_per_setis never sized, so it stays[].evaluate()then doesstate.train_state.consumed_valid_samples_per_set[0] += eval_batch_size→IndexError.train_megatron_mimo.py:390-393gates evaluation on a per-rank condition (valid_data_iterator is not None), because heterogeneous MIMO legitimately leaves some ranks without a validation loader. The new set-countall_reduceinevaluate_and_print_results()is unconditional, so only a subset of ranks enters it → collective hang. That is the exact failure mode the all_reduce was added to prevent.len(data_iterators)is called on a singleRerunDataIterator, which has no__len__→TypeError.
Given design note 3 in the PR description ("fail loud instead of hanging"), the consistent fix is to reject the combination up front — e.g. in ConfigContainer.validate(), raise when validation.multiple_validation_sets is set and cfg.model is a MegatronMIMOProvider. Supporting it properly is fine too, but out of scope for this PR; an explicit error is enough.
Please add a unit test for whichever guard you choose.
There was a problem hiding this comment.
Agree that this is out of scope. I added a guard in 1a80d21, which also loudly fails for one more potential silent failure modes: cfg.validation.full_validation can be set, but has no effect. Now raises a ValueError at startup.
| energy_monitor.resume() | ||
|
|
||
| # Skipped on in-process restart retries so recovery does not re-run validation. | ||
| if val_config.eval_at_start and global_state.train_state.do_valid and not is_inprocess_restart_retry: |
There was a problem hiding this comment.
eval_at_start re-runs on every ordinary checkpoint resume, but only one extra pass is budgeted.
is_inprocess_restart_retry only suppresses NVRx in-process retries. An ordinary resume from a checkpoint is a fresh process with restart_attempt == 0, so this branch runs again.
Meanwhile get_train_valid_test_num_samples() (loaders.py:133-135) reserves exactly one extra eval_iters worth of samples. A job that is preempted and resumed K times executes K+1 pre-train validation passes and consumes K * eval_iters * eval_global_batch_size more validation samples than the dataset was sized for.
Concrete failure: train_iters=10000, eval_interval=1000, eval_iters=2, dataloader_type="single" (non-cyclic — the same type your new functional test uses). Five preemptions ⇒ six pre-train passes ⇒ ten eval iterations of validation data beyond the reservation. The single-pass sampler drifts forward and can exhaust the validation set / raise StopIteration.
Issue #3996 proposed guarding on train_state.step == 0 specifically, and I think that matches the actual use cases: the value of this feature is a baseline for the loaded checkpoint at the start of training (SFT from a converted HF checkpoint, sanity-checking a HF↔MCore conversion before burning GPU-hours, anchoring an A/B of data mixes). On resume you already have that number from the last in-loop eval, so re-running it adds cost and puts a duplicate point at an already-logged step in TensorBoard/W&B.
Either add the step == 0 guard, or — if you deliberately want resume to re-evaluate — say so in the field docstring and widen the sizing formula accordingly. Right now it does the widened behaviour with the narrow budget. Please also add a nonzero-step test for whichever semantics you land on.
There was a problem hiding this comment.
Following up on my own comment — I agree the step == 0 semantics is the one that actually carries meaning, and I checked that it covers the motivating use cases cleanly:
load_checkpoint() resets state.train_state.step = 0 when cfg.checkpoint.finetune or release is set (checkpointing.py:2322-2323). So "SFT / continued pretraining from a converted HF checkpoint" — the strongest reason to want this feature — already lands at step 0 and gets its baseline. A plain mid-run resume does not, and that is exactly the case where the extra pass buys nothing (the previous in-loop eval already produced that number) while costing unbudgeted validation samples and putting a duplicate point at an already-logged step. So step == 0 is not a compromise here; it is the semantics that matches the use cases.
Given that, I think the field should be renamed while it is still unreleased and free to change. eval_at_start invites exactly the ambiguity this thread is about — "start" of the run, or of training? Two candidates, in my order of preference:
eval_at_step_zero— states the guard itself, so the behaviour on resume is unambiguous from the name alone and the docstring does not have to carry the disambiguation.baseline_eval— intent-revealing: a baseline is by definition taken once, at the beginning, which makes "why doesn't this re-run on resume?" a non-question.
There is an additional argument for renaming: ValidationConfig already inherits MCore's start_eval_at_iter (megatron/training/config/training_config.py:125), which delays the first evaluation. Having eval_at_start and start_eval_at_iter sit next to each other in the same config — near-identical wording, essentially opposite meanings — is a real readability trap for anyone scanning the config.
Happy to defer to your preference on the exact name; the substantive ask is the step == 0 guard plus a name that does not require a docstring to disambiguate.
There was a problem hiding this comment.
I think I can agree with this argument. The checkpoint-resume functionality was valuable for development, but practically adds little value.
The suggested re-naming is also reasonable given that Megatron-core already has a near-identical config field.
Fixed in 8f8fa1a
| f"(min {min_sets}, max {max_sets}; this rank has {num_sets}). The dataset " | ||
| "provider must return the same number of validation sets on every rank." | ||
| ) | ||
| if val_config.validation_set_names is not None and len(val_config.validation_set_names) != len(data_iterators): |
There was a problem hiding this comment.
validation_set_names set without multiple_validation_sets is silently ignored — this length check only runs inside the flag branch, and the suffix logic at line 486 is likewise gated on the flag. The user gets no names and no warning.
MLM asserts on this (megatron/training/training.py:3633: assert args.multiple_validation_sets, "--validation-set-names requires --multiple-validation-sets"). Worth matching.
Ideally both this check and the names/count check move into ConfigContainer.validate() — a static config error should fail at startup, not at the first eval_interval after the user has already paid for model init and dataset build.
| counts = torch.tensor( | ||
| [num_sets, -num_sets], dtype=torch.long, device="cuda" if torch.cuda.is_available() else "cpu" | ||
| ) | ||
| torch.distributed.all_reduce(counts, op=torch.distributed.ReduceOp.MAX) |
There was a problem hiding this comment.
Two concerns about this all_reduce:
- Process group. It uses the default world group, while the rest of this function routes collectives through
pg_collection(andevaluate()takes an explicitp2p_communicator/pg_collection). For multi-module / VLM setups that is inconsistent. - Placement. The number of validation sets is a static property of the config and the dataset provider — it cannot change between eval rounds. Validating it costs a full-world collective on every eval round, on the hot path.
build_train_valid_test_data_loaders()already knows the count (line 400) and is where the per-set counters get sized; doing the agreement check once there and trusting it afterwards is both cheaper and closer to the source of truth.
The check itself is a good idea — I just think it belongs at dataloader-build time.
There was a problem hiding this comment.
Excellent point! Moved to dataloader build in 666aefc
| print_rank_last("-" * length) | ||
|
|
||
| return total_loss_dict | ||
| return set_loss_dict |
There was a problem hiding this comment.
Non-blocking, but worth pinning down: the return value is now the last set's loss dict when multiple_validation_sets is on. No current caller uses the return value (pretrain.py:165/178, train.py:612, train_megatron_mimo.py:396 all discard it), so nothing breaks today — but this is a silent trap for any future early-stopping / save-best-checkpoint logic, which would end up keying off whichever set happens to be last in the blend.
Consider returning a dict keyed by the set suffix (or None in multi-set mode) so the ambiguity is impossible to consume accidentally. The docstring update is good; making it unrepresentable would be better.
Separately: when the timelimit fires mid-loop you return None after sets 0..k have already advanced their per-set counters while k+1..N-1 have not, so those sets resume from different offsets. That is defensible since the sets are independent, but it deserves a sentence in the docstring.
…ll_validation, multi test sets) Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…l_at_step_zero Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…names is set Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…ld time Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…ults Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…fixtures Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…eval_interval is unset Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…st but multi-val is disabled Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…idation_sets and full_validation Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…art-multi-val Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
|
Thank you for reviewing again! It seems like I missed some crucial changes when merging and resolving conflicts. To make sure this is not the case now, I ran the following tests and made sure they are all green: Unit suite (311 tests) and the L0/L1 pretrain functional tests (2 GPUs) pass on the branch tip. Addressing your inline comments above. Thanks! |
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…disabled Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…dation_sets Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…art-multi-val Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
|
A few additional hardening commits on top of the requested changes:
One behavior noted but intentionally unchanged: weights in the validation blend are ignored under The branch is also merged with the latest main. At the current tip, the unit suites for |
|
Happy to address any further concerns if there are any @yaoyu-33 |
|
@yaoyu-33 should we proceed with this feature? |
…art-multi-val Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
…ep_zero for MIMO Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
Signed-off-by: Moritz Platscher <moritz.platscher@kaiko.ai>
3b21583 to
3596952
Compare
|
@yaoyu-33 I merged the latest main into the branch and fixed what the merge broke. Changes since the last head:
Unit tests for test_eval, test_config, test_loaders, test_train, test_pg_collection_wiring and test_state pass Could you take another look if it looks good? |
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