[recipe, test] feat: add unit tests for recipes/common.py base helpers#3666
Open
lonexreb wants to merge 2 commits intoNVIDIA-NeMo:mainfrom
Open
[recipe, test] feat: add unit tests for recipes/common.py base helpers#3666lonexreb wants to merge 2 commits intoNVIDIA-NeMo:mainfrom
lonexreb wants to merge 2 commits intoNVIDIA-NeMo:mainfrom
Conversation
`src/megatron/bridge/recipes/common.py` exports five base config
builders that every recipe in the project inherits from:
- `_pretrain_common`
- `_sft_common`
- `_peft_common`
- `_sft_common_vlm`
- `_peft_common_vlm`
These were the foundation of every recipe but had **zero direct unit
test coverage**. Recipe-level tests catch model-specific overrides;
they don't catch regressions in the shared base.
Add a focused test file with one class per helper (5 classes,
27 tests total). All 5 helpers are pure config builders with no HF
I/O, so the tests are fast and require no mocking.
Coverage highlights:
- `_pretrain_common`: documented model=None placeholder, training
cadence (300k iters / GBS 32 / MBS 2), validation cadence, DDP
overlap + dist optimizer, HuggingFaceTokenizer placeholder,
mock-blend dataset, RNG seed 1234, no PEFT, bf16_mixed precision.
- `_sft_common`: shorter training (1k iters / GBS 128 / MBS 1),
low LR (5e-6), adam_beta2=0.98, minimal DDP (no overlap by
default), RNG seed 5678, no PEFT, pretrained_checkpoint slot.
- `_peft_common`: higher LR (1e-4) than SFT, default LoRA attached
with the documented standard targets (linear_qkv / linear_proj /
linear_fc1 / linear_fc2) and dim/alpha 32, training cadence
matches SFT, RNG seed 5678.
- `_sft_common_vlm`: overrides LLM SFT — longer training (300k iters)
with smaller GBS=32 / MBS=2, NullTokenizer (processor-driven),
HFDatasetConversationProvider with `make_cord_v2_dataset`, DDP
without overlap, RNG seed 1234, lr=3e-4.
- `_peft_common_vlm`: inherits LoRA from `_peft_common`,
NullTokenizer, HF conversation provider, RNG seed 1234.
Tests-only — no production code changes. Locks in the documented
contract so future refactors of the inheritance chain (e.g. adding a
field, changing a default) fail loudly rather than silently changing
the defaults every recipe inherits.
Signed-off-by: lonexreb <[email protected]>
Contributor
|
/ok to test c588b8e |
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.
Summary
src/megatron/bridge/recipes/common.pyexports five base config builders that every recipe in the project inherits from:_pretrain_common,_sft_common,_peft_common,_sft_common_vlm,_peft_common_vlmThese had zero direct unit-test coverage. Recipe-level tests catch model-specific overrides; they don't catch regressions in the shared base. A field default change in one of these helpers silently shifts every recipe in the project.
This PR adds 27 focused tests (5 classes, +312 LoC). All 5 helpers are pure config builders with no HF I/O, so the tests are fast and require no mocking. Tests-only — no production changes.
Coverage highlights
_pretrain_common_sft_common_peft_common_sft_common_vlmmake_cord_v2_dataset, DDP without overlap, RNG seed 1234, lr=3e-4_peft_common_vlm_peft_common, NullTokenizer, HF conversation provider, RNG seed 1234Test plan
python3 -m astparse cleanruff check+ruff format --checkcleancicd-unit-tests-corepicks up the new module undertests/unit_tests/recipes/Risk
Zero — tests only.