Repository navigation
fix(data): scope GLM assistant boundary scan to the current turn - #5753
Conversation
Before resolving a non-terminal assistant span, the boundary guard scanned
every turn's payload, so a role marker quoted in a user, system or tool
message aborted the whole row. GLM-shaped templates expose no {% generation %}
mask to fall back to, so the row raised ValueError out of the data pipeline
rather than degrading. Only the current turn renders inside the region being
searched, so the scan is now restricted to it.
Adds a unit test covering the change (red-green verified).
Detected by: megatron-bridge QA
Signed-off-by: Pruthviraj Prakash <pruprakash@nvidia.com>
pruprakash
added
bug
|
LGTM The fix correctly narrows the boundary-token scan to the current turn in the rendered_turn_end_roles branch. Only that turn payload renders between content_start and the next role header, so a marker quoted in any other turn can no longer veto the loss boundary and drop the whole mask. The default whole-conversation scan is preserved for the unrelated guard at line 1772, so no behavior change there. Both failure modes are covered by tests (quoted-in-user-turn still masks; quoted-in-assistant-payload still fails closed), and the tokenizer stub now routes content through encode consistently so multi-token quoted content renders the same way in _assistant_tokens and apply_chat_template. Minor non-blocking note: the docstring says template kwargs are always scanned. Scanning chat_template_kwargs unconditionally even when turns is restricted matches prior behavior, so it looks deliberate. Suggested test cases (unit, added/exercised by this diff):
No perf tests impacted. |
yaoyu-33
added
the
needs-more-tests
|
/ok to test c18b8de |
|
Retrospective QA-fix tracking issue: #6395. It records the fix authored by @pruprakash and delivered by this merged PR. |
This branch was previously deployed
4 participants
What does this PR do ?
Fixes GLM-shaped SFT rows aborting the data pipeline with
ValueError: AssistantMaskBoundaryConfig did not match any loss-contributing spans in input_idswhenever any turn quotes a role marker such as<|observation|>.Changelog
src/megatron/bridge/data/conversation_processing.py:_conversation_contains_boundary_tokensgains a keyword-onlyturns; the non-terminal assistant guard passesturns=[turn].tests/unit_tests/models/test_glm52_kimi_k3_minimax_m3_chat.py: 2 tests covering both branches of the guard.GitHub Actions CI
See the CI section in the Contributing doc for how to trigger the CI. A Nvidia developer will need to approve and trigger the CI for external contributors.
Before your PR is "Ready for review"
Pre checks:
Additional Information
conversation_processing.py:1449scanned every turn's payload, but only the current turn renders betweencontent_startand the next role header. BothTrueand the unknownNoneabort, so a marker quoted anywhere vetoed the boundary.{% generation %}mask to fall back to anddata/collators/sft.py:230does not catch, so the row killed the training job instead of degrading.2d3733eb, 2026-08-19), which added the guard and widened the GLM marker map to<|system|>/<|user|>/<|observation|>. Before it the row was mis-masked, not unloadable. Fail-closed is preserved when the marker is inside the assistant's own payload.nvcr.io/nvidian/nemo:nightly— RED (test patch only) 1 failed; GREEN 19 passed in the file, plus 129 passed acrosstest_conversation_processing.pyandtest_sft_processing.py.turn_search_start >= turn_limitwindow from [data] fix: Preserve assistant mask provenance #5166 collapses for a second non-terminal assistant turn under a history-dependent template) is unfixed here — it needs a different window derivation for all templates, not just GLM.