Skip to content

Revert "[recipe] feat: enable THD packing by default for Qwen3.5-VL finetune" - #3894

Merged
cuichenx merged 1 commit into
mainfrom
revert-3481-chcui/qwen35-vl-thd-default
May 19, 2026
Merged

Revert "[recipe] feat: enable THD packing by default for Qwen3.5-VL finetune"#3894
cuichenx merged 1 commit into
mainfrom
revert-3481-chcui/qwen35-vl-thd-default

Conversation

@cuichenx

Copy link
Copy Markdown
Contributor

Reverts #3481

@cuichenx
cuichenx merged commit f8c6113 into main May 19, 2026
6 of 9 checks passed
@cuichenx
cuichenx deleted the revert-3481-chcui/qwen35-vl-thd-default branch May 19, 2026 23:16
@claude

claude Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Light Review

Clean revert of #3481 — flips pack_sequences_in_batch back to False and removes the associated comment in _qwen35_vl_apply_common. No logic issues.

One observation: The revert PR body says only Reverts #3481 with no explanation of why. The original PR documented a hard dependency on Megatron-LM #2645 (GDN packed-sequence support). If the revert is because that MCore change has not landed in the pinned submodule yet, a one-line note in the PR description would help future readers understand the timeline (and know when it is safe to re-enable).

Test coverage: The existing unit tests in tests/unit_tests/recipes/qwen_vl/test_qwen35_vl_recipes.py do not assert on pack_sequences_in_batch. This is fine for a revert to the prior default, but means neither the original enablement nor this revert is guarded by a unit test. Consider adding a lightweight assertion (e.g., assert cfg.dataset.pack_sequences_in_batch is False for a representative SFT config) so the next flip is intentional and CI-visible.

Suggested test cases: No perf tests impacted.

vasunvidia pushed a commit to vasunvidia/Megatron-Bridge that referenced this pull request Jun 10, 2026
…inetune" (NVIDIA-NeMo#3894)

Signed-off-by: Vasudevan Rengasamy <vrengasamy@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant