Skip to content

Fix sampled-token matching after hidden demonstration text - #910

Merged
bradhilton merged 1 commit into
mainfrom
neumann/056-truncated-history
Sep 16, 2026
Merged

bradhilton merged 1 commit into
mainfrom
neumann/056-truncated-history

Conversation

@bradhilton

@bradhilton bradhilton commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

When a template omits demonstration text that also appears in a later sampled reply, approximate context matching can advance beyond that reply. Tokenization then drops its sampled tokens or raises even though its boundaries are already proven.

Reset the search cursor to the proven sampled-message boundary. This fixes a live Qwen truncated-reasoning trace encountered while smoke-testing Caladan experiment 056 after OpenPipe/caladan#419.

Validation:

  • 269 existing tokenization/model tests and the new regression pass; the regression fails on the previous revision.
  • The saved failing Qwen trace preserves all 8,839 sampled tokens and 7,452 canonical tokens, with exact token IDs and logprobs.
  • Ruff and ty 0.0.59 checks pass for both changed files.
  • CI passes: 783 Megatron lightweight tests and 1,505 unit tests (68 skipped).
  • All 64 saved Qwen training traces from the original attempts and reruns pass the canonical-token count audit with Caladan's native-output selection. Two live H200 reruns have each completed four groups with separate CISPO/SFT updates, adapter publication and durable step-4 saves; both full 32-row free-generation validations passed, and both runs exited successfully. Full results are in OpenPipe/caladan#441.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton
bradhilton merged commit 49e3392 into main Sep 16, 2026
7 checks passed
@bradhilton

Copy link
Copy Markdown
Collaborator Author

Consolidated review record (automated agent review traffic removed 2026-09-17)

Change. When a chat template omits demonstration text that recurs in a later sampled reply, approximate context matching could advance the search cursor past that reply, so tokenization dropped its sampled tokens or raised despite proven boundaries. The fix resets the cursor to the proven sampled-message boundary, resolving a live Qwen truncated-reasoning trace hit while smoke-testing Caladan experiment 056.

Review. Minsky and McCarthy independently reviewed the merged head 8f6acf0; both returned source-correctness CLEAR, judging it a small internal fix with no public API or art.megatron change. Minsky ran 308 tokenization/model tests (one unrelated environment failure) and confirmed the new regression fails on the base runtime; McCarthy passed 93 focused cases plus 16 adversarial provenance controls, with base-negative controls reproducing the defect. CI quality checks and trainer-rank GPU validation were green; the owner's saved-trace audit and H200 reruns (OpenPipe/caladan#441) were reported, not independently reproduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant