Skip to content

[recipe, test] feat: add unit tests for recipes/common.py base helpers#3666

Open
lonexreb wants to merge 2 commits intoNVIDIA-NeMo:mainfrom
lonexreb:training/test-recipes-common
Open

[recipe, test] feat: add unit tests for recipes/common.py base helpers#3666
lonexreb wants to merge 2 commits intoNVIDIA-NeMo:mainfrom
lonexreb:training/test-recipes-common

Conversation

@lonexreb
Copy link
Copy Markdown
Contributor

@lonexreb lonexreb commented May 4, 2026

Summary

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 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

Helper Tested
_pretrain_common model=None placeholder, training cadence, validation cadence, DDP overlap+dist optimizer, HuggingFaceTokenizer placeholder, mock-blend dataset, RNG seed 1234, no PEFT, bf16_mixed
_sft_common model=None, shorter training (1k/GBS128/MBS1), low LR (5e-6), adam_beta2=0.98, minimal DDP (no overlap), RNG seed 5678, no PEFT, pretrained_checkpoint slot
_peft_common higher LR (1e-4) than SFT, default LoRA attached with documented 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 (300k iters, GBS 32, MBS 2), NullTokenizer, 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

Test plan

  • python3 -m ast parse clean
  • ruff check + ruff format --check clean
  • CI: cicd-unit-tests-core picks up the new module under tests/unit_tests/recipes/

Risk

Zero — tests only.

`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]>
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 4, 2026

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.

@cuichenx
Copy link
Copy Markdown
Contributor

cuichenx commented May 6, 2026

/ok to test c588b8e

Copy link
Copy Markdown
Contributor

@cuichenx cuichenx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lonexreb thanks for your contribution. I'm not sure if these tests add value, since they're already tested indirectly in the model specific recipe tests. These default values don't really need to be tested.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-customer Waiting on the original author to respond label May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-request waiting-on-customer Waiting on the original author to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants