Fix VLM captioning label alignment and add an EOS stop target - #196
Merged
Conversation
Adds an independently derived, index-by-index statement of the label contract and parametrized cases that pin the prompt-mask boundary in both directions, EOS survival under truncation, and cross-rank label identity.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
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
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.
Summary
labels[i]is input tokeni+1, last real position-100.cross_entropy_lossdoes no internal shift and the VLM arches return logits over the text positions, so the previouslabels[i] == input_ids[i]scored each text logit against the already-visible token.labels[:prompt_len - 1] = -100.labels[prompt_len - 1]predicts the first caption token and stays supervised.max_text_len, so the last caption token is trained to stop. Tokenizers without an EOS keep the full budget.vlm.max_text_lenmust now be>= 2— one caption token plus its target.data/video_dataset.pyimports_tokenize_and_mask, so this covers the image and video captioning paths and all four arches.Testing
uv run ruff check kempnerforge/ tests/ scripts/passes — All checks passeduv run ruff format --check kempnerforge/ tests/ scripts/passes — 166 files already formatteduv run pyright kempnerforge/passes — 0 errors, 0 warningsuv run pytest tests/unit/ -v --timeout=120— 1765 passed, 3 skippeduv run pytest examples/vlm/tests -q— 33 passed;uv run pytest examples/vlm/eval/tests/unit -q— 104 passedBefore/after loss trajectory, 1 GPU on
kempner_eng.examples/vlm/configs/vlm_debug.toml, seed 42, batch 16, 600 steps, warmup 100, 30k-sample COCO captions (9.6k seen, under one epoch). Same worktree and venv; onlydata/vlm_dataset.pyandconfig/vlm.pydiffer between sides, verified by recording each side's file hashes in its log.Both start at
ln(50257) = 10.82.maintail mean1.1711 ± 0.2100, still descending (mid-third1.7086) with grad_norm decaying toward 0 — the copy objective has no entropy floor.fixtail mean4.3875 ± 0.2225, levelled off against its mid-third4.9687. Separation3.2164= 10.5σ from the two tails' own scatter. An identical curve would have meant the fix did not take.Same A/B repeated with
--model.tie_embeddings=true(as in the Qwen3-0.6B config the issue reproduced on) for 2000 steps,log_interval100.mainstarts at 8.3125, i.e. 2.51 nats below chance, before any training: with a tied output head the copy target is already partly satisfied at initialization.fixstarts at10.8750, on chance. Tails:main 0.6791 ± 0.1641(grad_norm 0.41) vsfix 4.0268 ± 0.1889(grad_norm 2.34) = 13.4σ separation,mainstill descending.Rewritten expectations were re-derived from
labels[i] = ids[i+1]on paper before running the code, not adjusted until green.TestNextTokenLabelContractencodes that derivation as an index-by-index loop — not tensor slices, so it cannot share an off-by-one with the implementation — and cross-checks it over 28 parametrized cases (14 input shapes x tokenizer with and without an EOS): caption exactlymax_text_len - 1, exactlymax_text_len, longer than the budget, empty, single-token, empty prompt,prompt_len == 1,max_text_len == 2, and prompts fillingmax_text_len - 1,max_text_len, and beyond.Five deliberate regressions injected; the suite caught all five, no survivors: unshifted labels (the original bug, 35 failures), prompt mask one too wide (10), one too narrow (15), EOS not appended (22), shift reversed (37).
Prompt-mask boundary in both directions:
test_prompt_mask_boundary_both_directionspinslabels[prompt_len - 1]supervised (equal to the first caption id) andlabels[prompt_len - 2]masked.EOS in targets:
test_eos_is_a_supervised_target,test_eos_survives_an_overlong_caption,test_single_token_caption_supervises_eos; no-EOS-tokenizer branch intest_no_eos_tokenizer_appends_nothing.Cross-rank label identity:
test_labels_are_identical_across_ranksbuilds labels for one index under two rank environments and RNG seeds with separate tokenizer instances, then byte-comparesinput_idsandlabels.uv run torchrun --nproc_per_node=4 -m pytest tests/distributed/test_checkpoint.py tests/distributed/test_resilience.py -v— 13 passed, 0 skipped, all 4 ranks. Collateral-damage guard only: no file undertests/distributed/importsvlm_datasetor_tokenize_and_maskand the VLM distributed tests synthesize their own labels, so this is evidence of no breakage, not evidence about label construction.Re-verified against
.external_docs/kf-codebase-design-new.htmlafter implementation. Part 3 admits a correctness fix, and the plan's own live case study lists "VLM label alignment" among the general fixes to promote to core. The finished diff adds no registry category and no new config field (it tightens an existingmax_text_lenvalidator), carries tests, and helps runs beyond its origin via the shared helper. No core-to-examples/dependency introduced.2-node run via
scripts/slurm/multinode.sh— deliberately not run. Its detection power is collective ordering, rank divergence, and a wedged job. This change adds no collective, no checkpoint key and no rank-dependent code, and the output tensor shape is unchanged, so none of those apply. The residual risk is rank divergence in label construction, which the cross-rank label-identity test detects directly and more cheaply.Closes #154
Closes #156