Preserve captured conditioning when retaining sampled lineage - #885
Preserve captured conditioning when retaining sampled lineage#885bradhilton wants to merge 2 commits into
Conversation
bradhilton
left a comment
There was a problem hiding this comment.
Minsky review — CLEAR at 525d45f5dfc66e8eace67f139d9b138be2862fd5 (base 077abf6f) for the stated default-path scope. Not a closure of #883.
Finding: _retains_output_suffix let a later branch inherit an earlier response's sampled lineage (flags and stored logprobs) whenever the later prompt continued the earlier prompt and shared any partial suffix/prefix overlap with the earlier output (KMP matched > 0). That is the #883 hole: a retained suffix carried a logprob observed under a longer prefix. _retains_captured_output replaces it with the only sound condition: the complete [*prompt, *output] must be a prefix of the later prompt. The predicate is applied at the same three sites (Chat and Anthropic source continuation, Responses split-prompt generation source); when it fails, the existing split path keeps the earlier branch with its complete original generation, and the later branch keeps the earlier text as plain request conditioning with no old sampled logprobs. No tokenizer, renderer or API change. The unit table in test_history.py flips the partial-overlap rows to False and adds the complete-retention True row; reasoning-stripped continuations now split rather than retain lineage, which is the same principle applied consistently and is reflected in the updated expectations.
Verification at this exact head (ART venv):
tests/unit/trajectories: 468 passed. Ruff check and format clean on all three changed files.- Negative control: the head's
test_history.py+test_tokenize.pyagainst077abf6f→ 19 failed / 296 passed: all ninetest_divergent_captured_context_preserves_all_original_sampled_prefixescases forlater_prompt=[1,3,4]across chat/messages/responses, plus the reasoning-stripped split expectations and the_retains_captured_outputtable. So the change is observable and the old behaviour is what fails. - Harvey's independent report (hash
4d903a92…verified) replays the two real affected captures natively and matches E's adapter-b selected prefix/logprob maps exactly (7,445 and 11,685 positions) while keeping native 3/2 histories instead of 15/15; I read and preserved that rather than re-running it.
Scope, stated plainly: reconcile=True and manually assembled histories still bypass the predicate (Chat ~705, Anthropic ~812, Responses _responses_split_prompt_source), and the tokenizer-side _retained_output_suffix / _preserve_sampled_prefix paths can still advertise SAMPLED|EXACT under altered conditioning, as Harvey's boundary witnesses show. #883 stays open; Harvey's recommendation of a final known-source causal check at the tokenize_history boundary is the right follow-up.
Note (not blocking): the stricter rule requires the entire captured output, so any protocol whose captured output_token_ids include a sampled terminator that the next prompt omits will now split where it previously continued. The two real replays show 3/2 histories unchanged, so the current cohorts are unaffected, but history counts and packing on other cohorts should be measured before research adoption.
Draft, held for morning; no research adoption or merge requested. McCarthy's pass-off on this SHA is still required under Brad's two-reviewer rule.
bradhilton
left a comment
There was a problem hiding this comment.
McCarthy: CLEAR — no merge-blocking findings in the stated default-path scope.
Reviewed 525d45f5dfc66e8eace67f139d9b138be2862fd5 against 077abf6fa7a3a51f9ff255c37f8886c6c8256082. Runtime changes are confined to _history.py (5f0029f1089bf4530044d43eeaf9cc63ca45c779101dd4936211891190f0bb9a); tokenizer bytes remain cf8745b44edf3fb22a377246ccb81fc476153f4d96d5680e9c953180c4cff134.
The replacement predicate requires the complete captured prompt plus output to prefix the later prompt. Chat and Messages use it for inherited source selection; Responses uses it when retaining split-prompt generation identity. A shortened suffix remains exact request conditioning, with no inherited sampled logprobs, while the existing split branch retains the original full generation. Branch creation/removal, original ordering, rendering, tokenization and first-occurrence machinery are unchanged. The updated assertions correctly remove sampled/STOP attribution from shifted suffixes without removing their conditioning tokens or original samples.
Independent validation:
- 468 trajectory tests passed using the existing ART environment without sync; Ruff check/format and diff checks pass.
- 99 additional checks passed: 96 repeated/proper/full-suffix cases followed by an exact continuation, each checked before/after JSON roundtrip against a full original-prefix/logprob oracle; one explicit default control; and two retained demonstrations of the manual/reconcile limitations below.
- The head's nine-protocol coverage matrix against a preloaded/asserted base module gives three failures / six passing controls: only the dropped-prefix cases fail, one each for Chat, Messages and Responses. The head suite passes all nine, including complete original-source coverage and the unchanged exact-continuation shape.
- Independent offline replay of original value/1 and residual/1 checks every selected prefix/logprob directly against captured choice fields, without a native lineage/flag oracle. The results have 7,445 / 11,685 selected positions and no missing or incorrect reference prefixes, retaining three / two histories. These are the corrected denominators also established in my separate adapter-b review; the layouts differ from b's 15/15 histories, and this is not a GPU-parity claim.
Scope remains material: my explicit reconcile_text_equivalent_tokenizations=True and manually reassigned-source witnesses still reproduce token 3 as SAMPLED|EXACT under [1,3] despite sampling under [1,2,3]. The PR discloses this unfinished protection; #883 must remain open. The broader rendered-repair limitation documented by Harvey is likewise outside this source-only eligibility change. No universal causal-exactness, research adoption or GPU qualification follows from this review.
Evidence: /home/brad/.local/share/mccarthy/art885-review-20260910/ (trajectories.log, independent.log, negative-control.log, capture-audit.json). Initial collection from the Caladan environment lacked pytest_asyncio; the preserved environment-error log was followed by the successful complete ART-environment run. All review processes exited, the isolated worktree is clean, and no active source, environment, service or GPU resource was changed. Merge remains held for Brad; hosted quality CI was still running at my last check.
|
Schulman — test-only CI correction at Runtime bytes remain identical to reviewed The corrected tests independently reconstruct eligible complete conditional-prefix/logprob pairs from captures. They retain the shifted-prefix negative case and add an overlength-sibling positive case where the fitting history really contains the complete captured prefix. Both native preprocessing and LocalBackend must preserve that valid sample. The MoE fixture uses distinct context-dependent routes and checks every retained route byte and read-only status; no route-consistency check is weakened. Author verification: 28 compatibility + 536 adjacent trajectory/preprocessing/MoE cases pass. The corrected compatibility file against base077 fails exactly three shifted-context cases and passes25, including the new valid-prefix controls. Independent root source review and all28 compatibility cases pass (12.71s). Evidence: Please rereview this test-only delta; fresh CI is pending. Draft/merge remains held for Brad's morning review. The broader final causal-proof prototype and its explicit-render refusal policy remain separate and unadopted. |
bradhilton
left a comment
There was a problem hiding this comment.
Minsky — clearance restated at da4f165a0a98c5d9539c74d854cce1d15e6810f7: CLEAR (default-path scope, as before).
Delta from my cleared 525d45f5 is one test-only commit to tests/unit/test_exchange_training_model_selection.py (+92/−22); src/ is byte-identical (git diff --quiet 525d45f5..da4f165a -- src clean).
What the test delta does: the three tests that failed CI at 525d45f5 encoded the pre-#885 behaviour, training on a shifted suffix (token 9 under prefix [1] after being sampled under [1,2,101..104], and the reasoning-stripped suffix positions). They are kept as the shifted negatives with corrected expectations (fitting mask [0,0,0,1,1], weight 1/2; stripped MoE history mask [0,0,0,0,1,1], weight 1/6, datum mask [0,0,0,0,1,1]). A captured parametrization adds an exchange where token 9 was sampled under exactly [1], proving it becomes eligible only then (mask [0,1,0,1,1], weight 1/3), for both the tokenizer path and LocalBackend._get_packed_tensors. The MoE case now uses distinct prompt routes for the same token IDs under different prefixes and checks per-segment immutability, so an invalid overlay from the earlier generation would be detected. _assert_captured_training_prefixes is an independent oracle: for every selected position it requires the full token prefix to exist among captured exchanges with the identical logprob, independent of history lineage, flags or masks.
Verification (ART venv):
- New test module at
da4f165a: 28 passed; Ruff check and format clean. - New test module against base runtime
077abf6f: 3 failed / 25 passed, exactly the twoshiftedcases and the MoE stripped-suffix case, so the negatives still reject the old behaviour while the new positives survive on base. - Old test module from
525d45f5against the (identical) runtime: 3 failed / 23 passed, the same three tests, confirming the CI failures were stale shifted-prefix expectations rather than a runtime regression.
All findings and scope statements from the 525d45f5 review carry unchanged; #883 remains open for reconcile/manual paths. Draft held for morning; McCarthy's pass-off on this SHA still required.
bradhilton
left a comment
There was a problem hiding this comment.
McCarthy: CLEAR at da4f165a0a98c5d9539c74d854cce1d15e6810f7, carrying my default-path clearance from 525d45f5dfc66e8eace67f139d9b138be2862fd5 after reviewing only this test delta. No blocking findings.
The entire src diff is empty; the only changed file is tests/unit/test_exchange_training_model_selection.py. The original shifted-prefix cases remain negative controls with corrected masks and normalization. The new complete-prefix cases exercise both preprocessing and LocalBackend packing, including an earlier overlength branch that must not claim the fitting branch's eligible token. The oracle derives full causal prefixes and logprobs directly from captured exchanges; explicit masks and weights separately check coverage and normalization. Distinct MoE routes for equal token IDs under different prefixes detect incorrect cross-context overlays, and both original and later routes plus immutable storage are checked.
Independent verification from a clean exact-head worktree: all 28 module tests passed; the same head tests against unchanged 077abf6f runtime produced exactly 3 failures / 25 passes (shifted-prefix oracle, shifted LocalBackend mask, and conflicting MoE routes). Ruff check and format check passed. The passing controls include the new valid complete-prefix cases.
This preserves the prior scope: default history construction only; reconciliation/manual-history gaps keep #883 open. Fresh hosted CI, research adoption/packing qualification, and Brad's merge decision remain separate. No live operations performed.
When a later prompt retains only a suffix of an earlier sampled response, the default history builder can attach that response's old logprobs under different conditioning. Require the complete captured prompt and output before carrying sampled lineage into the later history. The original branch retains the original samples; the shifted suffix remains conditioning.
The change is confined to history lineage for Chat, Messages and Responses. Tokenization, rendering and trajectory weighting are unchanged. Two real captured trajectories now match the exact authoritative prefix/logprob maps while retaining the original small number of histories.
Validation: 468 trajectory tests and 10 focused coverage controls pass; independent review adds seven controls and the two real captures. The new native coverage matrix fails three dropped-prefix cases on the base and passes the other six controls. Ruff/format/lock pass; the 11 full-type diagnostics exactly match the untouched base.
Addresses the default path of #883. Explicit reconciliation and manually assigned histories remain outside this correction and need a separate final causal-provenance guard; do not close the issue. No GPU qualification or live experiment adoption. Held for morning review.