Repository navigation
fix: warn when max_length cuts a supervised span part-way - #49
Merged
Neonkraft merged 2 commits intoAug 25, 2026
Merged
Conversation
`any(assistant_masks)` only catches rows whose assistant turn is truncated away entirely. A row that straddles max_length keeps a non-zero mask, so it stays in training and teaches an answer cut mid-sentence with no end-of-turn token. One untruncated render classifies all three cases instead — no assistant tokens, all of them past the cap, or straddling it — at the same cost. The keep/drop decision is unchanged.
Collaborator
|
Hi @KonstiNik Great catch! Assistant turns cut-off mid-sentence could indeed be a problem. I wonder if it would be better to drop these rows entirely, instead of simply warning the user? One challenge I can see with doing that is that perhaps one too many rows will be dropped. I'll take a look to see if this happens. |
Warning is right when the cap is wrong, since raising max_seq_length keeps the data. It is not enough when the sequence length is fixed by memory and dropping is the only lever left, so make it a switch. Default stays warn. Drop still warns. data.datasets[].weight multiplies the rows that survive filtering, so an uneven drop changes a dataset's share of the mixture while the config still claims the original weights — that has to be in the log rather than inferred from a row count.
Member
Author
|
Good point, and I think this should be the user's choice: I'd be against dropping straight away, because the user should first rethink the max_seq_length setting. If memory pins you down, then they should deliberately switch to a dropping mode where we discard the rows. I've pushed this as sft.truncated_span_action (warn by default, drop opt-in). |
Collaborator
|
Nice, LGTM! |
Neonkraft
added a commit
that referenced
this pull request
Aug 25, 2026
* feat: add original Qwen3-8B template from HuggingFace * feat: add generation markers * feat: add Qwen3-8B template to registry * fix: exclude think-stripped history from Qwen3 loss mask Qwen3 drops <think> from assistant turns at or before the last user query, so history rendered without a reasoning trace was being trained on. * test: assert which spans the qwen3 template puts in the loss * fix: exclude think-stripped history from Qwen3 loss mask Qwen3 drops <think> from assistant turns at or before the last user query, so history rendered without a reasoning trace was being trained on. * test: assert which spans the qwen3 template puts in the loss * test: assert qwen3 template renders byte-identically to upstream Vendors the pristine Qwen/Qwen3-8B chat_template as a fixture and diffs rendered output across the 20-combination matrix. * refactor: rename qwen3 jinja file * feat: filter out rows with all zero assistant loss mask * tests: add tests for row filters * fix: warn when max_length cuts a supervised span part-way (#49) * fix: warn when max_length cuts a supervised span part-way `any(assistant_masks)` only catches rows whose assistant turn is truncated away entirely. A row that straddles max_length keeps a non-zero mask, so it stays in training and teaches an answer cut mid-sentence with no end-of-turn token. One untruncated render classifies all three cases instead — no assistant tokens, all of them past the cap, or straddling it — at the same cost. The keep/drop decision is unchanged. * feat: add sft.truncated_span_action to drop rows a cut span damages Warning is right when the cap is wrong, since raising max_seq_length keeps the data. It is not enough when the sequence length is fixed by memory and dropping is the only lever left, so make it a switch. Default stays warn. Drop still warns. data.datasets[].weight multiplies the rows that survive filtering, so an uneven drop changes a dataset's share of the mixture while the config still claims the original weights — that has to be in the log rather than inferred from a row count. --------- Co-authored-by: Konstantin Nikolaou <knikolaou@icp.uni-stuttgart.de> --------- Co-authored-by: Konstantin Nikolaou <87869540+KonstiNik@users.noreply.github.com> Co-authored-by: Konstantin Nikolaou <knikolaou@icp.uni-stuttgart.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #47 (and merge into it).
The filter keeps a row if any assistant token survives the render truncated to
max_length— so it drops one whose assistant turn falls entirely past the cap, but not one whose span straddles it.My suggestion would be to have an untruncated render that classifies three cases instead: no assistant tokens at all, all of them past the cap, or straddling it. The last kept, with a warning. The render must be untruncated for that case to be visible, so the window is applied by slicing.
The keep/drop decision is unchanged (
any(mask[:max_length])is what the truncated render already computed), so the existing 21 row-filter tests pass untouched. 5 new tests, 108 total; ruff and black clean.