Fix text-only VLM CPT packing truncation - #7211
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c7785eae7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6817905ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d8503eae7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd9a54ee87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb06e4c8f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3af93545ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ence contamination
|
Thanks for this. Flagging one regression we hit in testing, and I pushed a fix to the branch. Removing the blanket I checked whether it was just a different but valid loss or real contamination. Using LR=0 (frozen weights, identical batches) on Qwen3.5-2B:
The only change between those two cases is a second sequence sharing the Fix in the pushed commit: add Longer term it is probably cleaner to gate the auto-enable on attention type rather than a per-model blocklist, but this keeps the PR correct for now. |
… for packing guard
|
Follow-up: generalized the guard so it is not tied to two model names. Replaced the Verified in an isolated env (transformers 5.2.0, FLA, causal_conv1d): Qwen3.5-2B and its PEFT-wrapped form are detected and have Making packing actually correct for these hybrids (threading |
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47cc8d4eeb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and ( | ||
| _is_vision_dataset(train_dataset, unknown_is_vision = is_vlm) | ||
| or _is_vision_eval_dataset(eval_dataset, unknown_is_vision = is_vlm) |
There was a problem hiding this comment.
Block packing for images embedded in message columns
When a VLM is passed its raw tokenizer, a dataset using the supported chat format messages=[{"content": [{"type": "image", ...}]}] has no top-level key in _VISION_DATASET_KEYS, so this branch enables packing and padding-free despite containing images. check_dataset_for_missing_videos already recognizes image/video-style message columns in unsloth/models/vision.py; similarly detect nested multimodal content here (or conservatively block messages/conversations) so these runs do not flatten multimodal examples and silently train as text-only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Looked at this closely and it is not reachable as a packing bug. The scenario requires a VLM with an explicitly supplied raw tokenizer (not a processor) plus a messages/conversations dataset carrying nested image content. A PreTrainedTokenizer cannot produce pixel_values, so with a raw tokenizer the images are dropped whether or not packing is on. Blocking packing would not preserve them; the text-only outcome comes from the tokenizer choice, not from flattening. The auto-processor path (processing_class is None) is already blocked via is_auto_processor_vlm, and a processor-supplied VLM is blocked via is_processor. Conservatively blocking messages/conversations would instead disable packing for the text-only chat-format CPT this PR exists to enable (chat-format text is the primary use case), so that would regress the feature. If a defensive warning for nested multimodal content in a raw-tokenizer run is wanted, that is a separate hardening enhancement rather than a correctness fix, and it would need to target nested image/video content specifically, not all chat columns.
| function = function.replace( | ||
| " # All Unsloth Zoo code licensed under LGPLv3\n", | ||
| """ # All Unsloth Zoo code licensed under LGPLv3 |
There was a problem hiding this comment.
Avoid depending on an exact Zoo source header
The matched guard validates only the fast helper's signature, but this injection additionally requires its source to contain this exact license-comment line. With a compatible newer unsloth_zoo that changes or moves that header (the dependency is only lower-bounded), the replacement is a no-op while the later tokenization replacement still introduces _unsloth_wrapped_packing; every affected SFT dataset preparation then fails with NameError. Insert the setup based on a structural location or verify the replacement succeeded before emitting references to these helper variables.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 92f5efe. The _unsloth_wrapped_packing / _inspect setup is now inserted at the sft_prepare_dataset signature (a structural anchor that always exists) via re.subn, instead of matching the exact 'All Unsloth Zoo code licensed under LGPLv3' comment line, and it raises if the signature cannot be located. So the helper variables are always defined before the truncation / pack_dataset rewrites reference them, across unsloth_zoo versions that move or drop that header. Regression test test_wrapped_packing_setup_survives_missing_zoo_header patches in a Zoo source without the header and asserts the setup still lands before the reference.
…omment The _unsloth_wrapped_packing / _inspect setup block was injected by matching the exact 'All Unsloth Zoo code licensed under LGPLv3' comment line in the sourced sft_prepare_dataset. The unsloth_zoo dependency is only lower-bounded, so a newer Zoo that moves or drops that header made the setup a silent no-op while the truncation and pack_dataset rewrites still emitted references to those names, raising NameError on every SFT dataset preparation. Anchor the setup on the function signature instead (a structural location that always exists) and fail loudly if it cannot be found, so the helper variables are always defined before they are referenced across Zoo versions. Adds a regression test that patches in a Zoo source without the license header.
|
@codex review |
for more information, see https://pre-commit.ci
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Resolve conflicts after #7211 landed in main. #7249 stacks on #7211 and carries the more general versions of the shared code, so keep them: - trainer.py: the varlen shim gating (hybrid_varlen_active), the encoder-decoder block, and the string-model config resolution supersede the base hybrid guard. - rl_replacements.py: the _require_replace helper and _WRAPPED_PACKING_SETUP constant supersede the inline function.replace injection. - test_packing.py: keep the encoder-decoder / decoder-only split and the _require_replace / drift-resistant tests; drop the now-duplicated base fixtures.
* Fix text-only VLM CPT packing truncation * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Handle streaming vision datasets in packing * Harden multimodal packing detection * Preserve safe packing boundaries * Scope stream packing checks to VLMs * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Narrow VLM packing detection * Align packing mode and eval safety * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add qwen3_5/qwen3_next to PADDING_FREE_BLOCKLIST to avoid packed-sequence contamination * Detect hybrid linear-attention models structurally instead of by name for packing guard * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Install wrapped-packing setup at the signature, not the Zoo license comment The _unsloth_wrapped_packing / _inspect setup block was injected by matching the exact 'All Unsloth Zoo code licensed under LGPLv3' comment line in the sourced sft_prepare_dataset. The unsloth_zoo dependency is only lower-bounded, so a newer Zoo that moves or drops that header made the setup a silent no-op while the truncation and pack_dataset rewrites still emitted references to those names, raising NameError on every SFT dataset preparation. Anchor the setup on the function signature instead (a structural location that always exists) and fail loudly if it cannot be found, so the helper variables are always defined before they are referenced across Zoo versions. Adds a regression test that patches in a Zoo source without the license header. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Etherl <61019402+Etherll@users.noreply.github.com> Co-authored-by: danielhanchen <danielhanchen@gmail.com>
Summary
pack_datasetAPIsTests
python -m pytest tests/utils/test_packing.py -qpython -m pytest studio/backend/tests/test_training_preflight.py -qruff check unsloth/trainer.py unsloth/models/rl_replacements.py studio/backend/core/training/trainer.py tests/utils/test_packing.pyFixes #7206