Skip to content

[Dev] ci: guard examples/multimodal_dev with a unit-test bucket - #6083

Open
BestJuly wants to merge 6 commits into
NVIDIA:devfrom
BestJuly:lit/multimodal_dev_ut_ci
Open

[Dev] ci: guard examples/multimodal_dev with a unit-test bucket#6083
BestJuly wants to merge 6 commits into
NVIDIA:devfrom
BestJuly:lit/multimodal_dev_ut_ci

Conversation

@BestJuly

Copy link
Copy Markdown
Contributor
  • I, the PR author, have personally reviewed every line of this PR.

What does this PR do?

Runs the examples/multimodal_dev/tests/ suite as its own H100 unit-test bucket, and repairs the two things that stopped that suite from being a usable guard.

examples/multimodal_dev tracks megatron.core APIs closely — rope_utils, GPTModel, TransformerConfig, the GPT layer specs — but it lives outside tests/unit_tests, so nothing in CI exercises it. Core changes have therefore landed without anyone noticing they broke the example.

That is not hypothetical: it is broken on dev today. The fused-mRoPE change (#5962) made apply_rotary_pos_emb read config.mrope_section and config.apply_rope_fusion directly, and test_mrope_parity.py::test_vision_rope_wrapper_forwards_max_seqlen_to_thd drives that code with a SimpleNamespace stub written before those fields existed:

AttributeError: 'types.SimpleNamespace' object has no attribute 'mrope_section'
megatron/core/models/common/embeddings/rope_utils.py:93

Measured on dev @ 9304a2564, 1×8×H100: 1 failed, 45 passed (identical at 1 rank and at 8).

Three commits:

  1. Repair the stale config stub. The stub now carries mrope_section=None, apply_rope_fusion=False, pinning the test to the unfused legacy THD path it was written for. No change to megatron/core.

  2. Make the standalone harnesses real tests. Four of the eight files (test_cp_correctness.py, test_cp_thd_correctness.py, test_thd_correctness.py, test_vision_patch_merger_parity.py) were argparse + main() scripts with no test_* functions — pytest imported them and collected nothing, so the BSHD-vs-THD, CP-vs-CP=1 and patch-merger parity checks they implement only ever ran by hand. Each comparison body is factored into a reusable function that both a pytest entry point and the original main() call, so the CLI and the tests cannot drift; a per-module DEFAULTS dict backs both the argparse defaults and the test parameters. CP sizes that do not divide the world size skip rather than fail, and every module leaves the model-parallel groups torn down, so the files work at any --nproc-per-node and share one pytest process. The __main__ entry points are unchanged in behaviour and were re-verified after the refactor. Net: +7 executing tests.

  3. Wire up the bucket. New test_case: [examples/multimodal_dev/tests/**/*.py] in tests/test_utils/recipes/h100/unit-tests.yaml, plus a local conftest.py. The bucket is collected outside tests/unit_tests/, so tests/unit_tests/conftest.py does not apply to it; the local conftest supplies exactly what tests/unit_tests/run_ci_test.sh depends on — the --experimental option its second pytest pass passes, and the pytest_sessionfinish hook that maps pytest's "no tests collected" (exit 5) to success.

Scope of the bucket

environment: [dev], tag: [latest], deliberately:

  • tag: latest only — the legacy ref pins an older mcore and an older copy of this example, so the pairing this bucket guards does not exist there.
  • environment: dev only — validated against the dev container; lts is unverified and an unverified entry risks a red bucket on older TE. Widening is a one-line change once it has been observed green.

gb200 is not wired up: selection there is marker-driven (launch_on_gb200) and no file in this suite carries the marker, so the bucket would launch an empty job.

Validation (1×8 H100)

Result
dev @ 9304a2564, baseline 1 failed, 45 passed
This branch, 8 ranks 53 passed

The exact CI entry point was then run with the tree bind-mounted at /opt/megatron-lm so the script's hardcoded paths resolve as they do in CI:

bash tests/unit_tests/run_ci_test.sh --tag latest --environment dev \
  --bucket "examples/multimodal_dev/tests/**/*.py" --log-dir /tmp/ci-logs
53 passed, 64 warnings in 40.11s                  # pass 1: -m 'not experimental and not flaky_in_dev'
collected 53 items / 53 deselected / 0 selected   # pass 2: --experimental
RUN_CI_TEST_EXIT=0

Pass 2 is what the new conftest exists for: --experimental is accepted and the resulting exit code 5 is mapped to 0. Recipe resolution was checked through recipe_parser.load_workloads(scope='unit-tests', model='unit-tests', test_case='examples/multimodal_dev/tests/**/*.py', environment='dev', tag='latest'), which returns the workload plus its mcore-pyt-dev build dependency like every other bucket, and find_test_cases.py returns no --ignore args (no child buckets). Bucket wall clock ~40 s.

Issue tracking

Linked issue: none. This is a test/CI change with no runtime impact.

Contribution process

Pre-checks

  • I have added relevant unit tests
  • I have added relevant functional tests — n/a, this PR is test/CI wiring
  • I have added proper typing to my code — matches the surrounding suite's style
  • I have added relevant documentation — the recipe entry carries a comment explaining the dev/latest scoping, and each converted module documents its pytest invocation
  • I have run the autoformatter.sh on my PR — a no-op here: it only considers .py files under megatron/core and tests/, and this PR's only tests/ change is a YAML

🤖 Generated with Claude Code

BestJuly added 3 commits July 27, 2026 20:54
The fused-mRoPE change (NVIDIA#5962) made apply_rotary_pos_emb read
config.mrope_section and config.apply_rope_fusion directly, but
test_vision_rope_wrapper_forwards_max_seqlen_to_thd drives it with a
SimpleNamespace stub written before those fields existed, so the test
fails with AttributeError on current dev.

Give the stub both attributes, pinning the test to the unfused legacy
THD path it was written for.

Signed-off-by: Li Tao <lit@nvidia.com>
…ests

Four files in examples/multimodal_dev/tests were argparse + main()
scripts with no test_* functions: pytest imported them and ran nothing,
so the BSHD-vs-THD, CP-vs-CP=1 and patch-merger parity checks they
implement were only ever exercised by hand.

Factor each comparison into a reusable function and add pytest entry
points on top of it; main() calls the same code, so the CLI and the
tests cannot drift. Per-module DEFAULTS dicts back both the argparse
defaults and the test parameters. CP sizes that do not divide the world
size skip instead of failing, and every module leaves the model-parallel
groups torn down, so the files run at any --nproc-per-node and share one
pytest process.

Adds 7 executing tests; the __main__ entry points are unchanged in
behaviour.

Signed-off-by: Li Tao <lit@nvidia.com>
examples/multimodal_dev tracks megatron.core APIs closely (rope_utils,
GPTModel, TransformerConfig, the GPT layer specs) but lives outside
tests/unit_tests, so core changes have landed without anyone noticing
they broke the example — the stale mRoPE config stub fixed in this
branch is one such case.

Add the directory as its own H100 unit-test bucket (environment dev,
tag latest) so it runs in the same pipeline as the change that breaks
it. tag latest only: the legacy ref pins an older mcore and an older
copy of the example, so the pairing this guards does not exist there.

The bucket is collected outside tests/unit_tests, so
tests/unit_tests/conftest.py does not apply to it. A local conftest
supplies what run_ci_test.sh depends on: the --experimental option its
second pytest pass passes, and the sessionfinish hook that maps pytest's
"no tests collected" exit code 5 to success.

Signed-off-by: Li Tao <lit@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@BestJuly
BestJuly requested a review from FDecaYed July 30, 2026 08:37
BestJuly and others added 3 commits August 6, 2026 02:46
The suite runs as its own CI bucket, so tests/unit_tests/conftest.py is not
an ancestor of the collected files and none of its hooks apply here. The
session-scoped `cleanup` fixture was not reproduced, so the NCCL process
group was still alive at interpreter exit and torch reported a resource
leak on every run.

Reproduce that fixture, and document the two hooks that are deliberately
not reproduced: `set_env` (this suite needs the fused and flash attention
backends enabled, as the THD paths under test are only reachable through
them, whereas tests/unit_tests pins them off) and `reset_env_vars`. Also
record that `--experimental` is registered by both conftests, so the two
directories cannot be passed to a single pytest invocation.

In the recipe, note that GitHub CI always runs dev+latest and ignores the
environment/tag matrix; that restriction is only expanded by GitLab.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Li Tao <lit@nvidia.com>
…teardown

test_cp_correctness.py and test_vision_patch_merger_parity.py initialised
and destroyed the model-parallel groups through parallel_state directly,
while the rest of the suite goes through Utils. That leaves Utils.inited
stale, which silently turns a later Utils.destroy_model_parallel() into a
no-op. Route both modules through Utils.

Collapse the CP=1 baseline and the CP>1 trial into a single run_cp(). They
differed only in who built the batch -- a pure function of the seed, so it
needs no threading between calls -- and in whether reference weights were
supplied, neither of which is a property of CP=1. One code path keeps the
baseline and the trial differing only in cp_size, and lets a module-scoped
fixture reuse a single CP=1 run across every CP size under test.

Drop the cp_size key that the run_cp_comparison parameter shadowed in
test_cp_thd_correctness.py, and collect the remaining hard-coded sizes and
tolerances into DEFAULTS as the sibling modules already do.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Li Tao <lit@nvidia.com>
@BestJuly
BestJuly marked this pull request as ready for review August 6, 2026 10:02
@BestJuly
BestJuly requested review from a team as code owners August 6, 2026 10:02
@BestJuly BestJuly changed the title ci: guard examples/multimodal_dev with a unit-test bucket [Dev] ci: guard examples/multimodal_dev with a unit-test bucket Aug 6, 2026
@BestJuly

Copy link
Copy Markdown
Contributor Author

/claude review

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Light review. The core of this looks right — I verified the claims that matter:

  • The mrope_section=None / apply_rope_fusion=False stub fix is correct: _is_raw_mrope_freqs_thd short-circuits on config.mrope_section is None before the t.dim() != 3 / freqs.shape[1] != 1 raise branches (rope_utils.py:90-97), so the [3, 1, 1, 8] freqs tensor in that test — which is raw-looking — correctly stays off the raw-mRoPE path and lands on the monkeypatched _apply_rotary_pos_emb_thd.
  • The recipe comment about GitHub CI ignoring the matrix is accurate: cicd-main.yml:680 parses only .products[].test_case[], and .github/actions/action.yml:137-141 hardcodes --environment dev with tag: latest from the caller.
  • find_test_cases.py returns no --ignore args either direction — is_child_of_bucket compares base paths, and neither examples/multimodal_dev/tests nor tests/unit_tests is a prefix of the other. No overlap with the catch-all tests/unit_tests/**/*.py bucket.
  • Routing every init/teardown through Utils keeps Utils.inited honest, and Utils.initialize_model_parallel/destroy_model_parallel pop NVTE_FLASH_ATTN/NVTE_FUSED_ATTN themselves — consistent with the conftest docstring's claim about deliberately not reproducing set_env.
  • Grad hygiene in the shared thd_model module fixture is fine: run_equal_length_test zeroes between BSHD and THD and again at exit, so the smoke test starts clean and neither test is order-dependent.

Two non-blocking notes:

1. aligned_seq_len uses max where the docstring promises all (test_cp_correctness.py:232-240)

The docstring says it rounds up "to a multiple of 2*cp for every CP size given," but the implementation aligns only to 2 * max(cp_sizes). That is a multiple of every 2*cp only when max(cp_sizes) is divisible by each of the others. It holds for the current CP_SIZES = [2, 4], so nothing is broken today — but adding a non-dividing size (e.g. [2, 3, 4]align = 8, which is not a multiple of 6) would silently produce a seq_len that fails zigzag splitting for one of the trials. LCM would make the code match the contract:

import math  # at top of module

def aligned_seq_len(seq_len, cp_sizes):
    align = 2 * math.lcm(*cp_sizes)
    return ((seq_len + align - 1) // align) * align

Alternatively just narrow the docstring to say it aligns to the largest CP size.

2. The repaired stub reproduces the drift it exists to catch (test_mrope_parity.py:667-676)

This is exactly the failure mode the PR is built to prevent: a hand-rolled SimpleNamespace that goes stale the next time rope_utils reads a new config attribute. The bucket will now catch the next occurrence rather than let it land silently, which is the main win — but constructing a real TransformerConfig here (no distributed init needed) would make the test immune instead of merely loud. Worth considering as a follow-up rather than a change to this PR.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant