Add support for packed thd in BERT language module. - #5540
Conversation
Signed-off-by: Björn Buschkämper <bjoern.buschkaemper@gmail.com>
Signed-off-by: Björn Buschkämper <bjoern.buschkaemper@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds packed thd (sequence packing) input support to the Megatron Core BertModel, enabling packed-sequence execution paths (no explicit attention mask, packed-aware position ids, and packed-aware RoPE length/params). It also adds a unit test to validate that packed inputs bypass attention-mask expansion and use packed position ids / RoPE invocation.
Changes:
- Add
packed_seq_paramsplumbing toBertModel.forward()and route packed runs to skip extended attention mask creation. - Generate packed position ids from
cu_seqlens_qwhenpacked_seq_paramsis provided. - Add a unit test that asserts packed forward uses packed position ids and does not build an extended attention mask.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
megatron/core/models/bert/bert_model.py |
Adds packed_seq_params support in forward path, packed position-id generation, and packed-aware RoPE setup. |
tests/unit_tests/models/test_bert_model.py |
Adds a unit test validating packed-THD forward behavior (position ids, no extended mask, packed params propagation). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Björn Buschkämper <bjoern.buschkaemper@gmail.com>
|
/ok to test ab4aefc |
|
/claude review |
There was a problem hiding this comment.
Nice change — the packed thd position-id logic and validation are correct, and the new tests cover the validation paths well.
One non-blocking edge case: in BertModel.forward, the return_embeddings post-process branch computes:
masks = torch.sum(attention_mask, dim=1)When packed_seq_params is provided, attention_mask is None, so this would raise a TypeError if a caller ever combined packed input with return_embeddings=True and post_process=True. It's an unusual flag combination (and the added tests use post_process=False), so it may be out of scope — but worth a guard or an explicit 'unsupported' error to fail clearly rather than deep in the sum.
|
@NVIDIA/mcore-oncall |
|
/claude fix |
|
❌ Claude fix stopped because a workflow step failed. Inspect the run. |
Signed-off-by: Björn Buschkämper <bjoern.buschkaemper@gmail.com>
|
@ericharper can you retry? Fixed the merge conflict. |
|
@NVIDIA/mcore-oncall |
guihong-nv
left a comment
There was a problem hiding this comment.
The packed path still needs fixes for padded or context-parallel metadata and for packed-aware post-processing. The inline comments describe the affected cases and expected behavior.
- Build position ids from physical padded boundaries - One logit for whole pack in binary head pool - return_embeddings crashes on packed input - .item() call forces sync, unsafe Signed-off-by: Björn Buschkämper <bjoern.buschkaemper@gmail.com>
|
@guihong-nv Should be fixed now, can you check?
|
|
@guihong-nv ptal, thanks |
What does this PR do ?
Adds packed thd input support for BERT language module to support sequence packing.
Issue tracking
Linked issue: #5539
Contribution process
Pre-checks
Code review
Feel free to message or comment @NVIDIA/mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
All PRs start as draft. If you open a non-draft PR, it will be automatically converted to draft.
Step 1: Mark PR as "Ready for Review"
.github/CODEOWNERS.Final Review might get declined if these requirements are not fulfilled.
Step 2: Final Review
For PRs that change
megatron/core, once all expert reviewers have approved, theFinal Reviewlabel is applied automatically and final reviewers are assigned.For PRs outside
megatron/core, this step is skipped.Step 3: Approved
Once all required reviewers have approved, the
Approvedlabel is applied automatically.Merge
Any member of mcore-engineers will be able to merge your PR.