fix(agent): realign from divergence using previous response length - #2360
Open
YeonwooSung wants to merge 1 commit into
Open
fix(agent): realign from divergence using previous response length#2360YeonwooSung wants to merge 1 commit into
YeonwooSung wants to merge 1 commit into
Conversation
3 tasks
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.
Background
_SampleBuilderheals TITO / chat-template token drift by either REALIGN (overwrite the drifted tail in-place) or FORK (close the builder and start a new Sample). REALIGN is supposed to be a cheap repair of a short previous response.Fixes #2338.
Problem
Two bugs in
slime/agent/trajectory.py, both visible in the reporter's 3-turn example (defaultfork_threshold=1024): a 61-token transition turn REALIGN'd a 28k-token previous response, and 184–383 already-trained tokens before the BPE split were zeroed with it.1. REALIGN gated on this turn's
output_ids, not the previous responseclassify_token_driftusedlen(turn.output_ids)to decide REALIGN vs FORK. REALIGN rewrites the previous response (it zeros that span and splices the new prompt suffix). A short follow-up therefore realigned — and discarded — a long trained span.In the report:
new_resp_len=61 < 1024→ REALIGN,zeroed_resp_tokens=28404.2.
_align_to_promptoverwrote fromlast_response_start_idx, not the divergenceEven when REALIGN is correct, zeroing from the start of the previous response also wipes tokens that still match the new prompt (the "innocent prefix" before the BPE split). The reporter measured 184 / 237 / 332 / 383 trained tokens killed before
realign_atacross four REALIGNs, withkilled_masked_lm0 == 0(the whole zeroed range wasloss_mask=1model output, not tool/history).Solution
classify_token_drift: REALIGN only when the divergence sits inside the most-recent response and bothsacrificed_len = len(tokens) - last_response_start_idx(previous response) andlen(turn.output_ids)are< fork_threshold. Otherwise FORK._try_merge_assistant_rewriteis unchanged (different contract: merging a short abandoned assistant rewrite)._align_to_prompt(prompt_ids, realign_at): overwritetokens/loss_mask/logprobsfrom the divergence only. Tokens beforerealign_atkeep their existing mask and logprobs.append_turnpasses the same_common_prefix_lencut that classify already computed (recomputed at append time; tokens are unchanged between the two calls).Tests
tests/test_agent/test_trajectory_manager_branching.py:test_4_6_drift_B1_threshold_boundarynow pins the new gate: REALIGN iff both sides are< threshold; a long previous response forks even if this turn is short (the oldthreshold=3, r1=4, r2=2case, which used to REALIGN). After REALIGN, the innocent prefix keepsloss_mask=1and only the suffix fromrealign_atis zeroed.test_2_4_drift_case_B1_short_replacesupdated so the trained token before the last-token split staysloss_mask=1.