Preserve distinct captures with different prompt tokens - #886
Conversation
bradhilton
left a comment
There was a problem hiding this comment.
Minsky review — CLEAR at 9fe17e39db8ad237ae304920b158b69f2f66ebf9 (parent = #885 head da4f165a). Scope is sampled-source identity only; #883's reconcile/manual/repair gaps remain open and no research source adopts this.
Finding: _sampled_evidence_fingerprint previously hashed only output-side evidence (message/text/blocks, token_ids, logprobs, finish/stop reason), so two exchanges with a reused response ID and identical outputs but different captured prompts collapsed into one source key and one causal prefix was dropped. The head adds prompt_token_ids to the fingerprint for all four protocols: Chat and Completions take the choice-level value and fall back to the response-level value when the choice's is missing or null (the same precedence _chat_choice_tokens uses), Responses takes the generation-level prompt_token_ids when present, Messages takes the response extra. Identical captures still hash identically, so legitimate deduplication is unchanged; only distinct causal contexts now stay distinct. The comment on _source_key states the rationale.
Verification at this exact head (ART venv):
tests/unit/trajectories: 484 passed. Ruff check and format clean on both changed files.- Negative control: the new
test_sampled_source_identity.pyagainstda4f165a→ 10 failed / 6 passed. Failing: distinct-prompt binding (all protocols where the response ID is reused), the missing/null choice-prompt fallback cases, andtest_reused_response_identity_preserves_both_sampled_causal_prefixes[False/True]. The six that pass on the parent are the identical-capture roundtrip and unrelated-choice controls, which by design do not depend on the change. - The runtime delta is 15 lines confined to the fingerprint; no tokenizer, renderer, history-splitting or API behaviour changes.
Draft, stacked on #885 and held; McCarthy has cleared this same SHA, so both dedicated-reviewer pass-offs are on 9fe17e39. CI still pending per the thread.
9fe17e3 to
3627311
Compare
bradhilton
left a comment
There was a problem hiding this comment.
Minsky fresh review — CLEAR at 3627311444748067482c486909d3642644509efd (standalone on main 9a16366d). This is a new exact-head verdict; my earlier clearance of the stacked head 9fe17e39 does not transfer by itself.
Base facts verified: parent is 9a16366d (main, which now includes #882 and #887); git patch-id of 9a16366d..36273114 equals that of the cleared stacked delta da4f165a..9fe17e39 (9e17ab53…), so the change is byte-for-byte the same 15-line fingerprint edit plus the same 160-line test module; _history.py is byte-identical to main; #885's head da4f165a is not an ancestor, so no #885 default-path splitting, manual-edit validator or attribution behaviour is imported.
Change (unchanged from prior review): _sampled_evidence_fingerprint includes captured prompt_token_ids for all four protocols (choice-level with response-level fallback for Chat/Completions, generation-level for Responses, response extra for Messages), so reused response IDs with identical outputs under different captured prompts stay distinct sources while identical captures still deduplicate. No tokenizer, renderer, history-splitting or API change.
Verification at this exact head (ART venv):
tests/unit/trajectories: 547 passed. Ruff check and format clean on both files.- Negative control: the head's
test_sampled_source_identity.pyagainst main9a16366d→ 10 failed / 6 passed (same shape as on the stacked base: distinct-prompt binding, choice-prompt fallback and reused-identity cases fail; roundtrip and unrelated-choice controls pass), matching the PR's stated 10/6 → 16/16.
Draft; McCarthy is reviewing this same SHA. Merge/adoption remain Brad's decision; #885 stays held separately.
bradhilton
left a comment
There was a problem hiding this comment.
McCarthy fresh standalone review: CLEAR, no blocking findings at 3627311444748067482c486909d3642644509efd against main 9a16366d79eebc36e00f75670965e2629c012b75.
Captured prompt IDs now distinguish sampled sources whose response IDs, output tokens and other evidence collide. Chat/Completions retain choice-first, response-fallback precedence (including missing/null fallback); Messages use response metadata and Responses use the selected generation. Genuine copies still deduplicate, and unrelated choices/generations do not affect the selected source. The change reaches existing source identity comparisons without introducing an edited-history validator or changing probability/normalization policy.
Standalone composition verified: exactly two changed files; _history.py is byte-identical to main and #885 is not an ancestor. The fingerprint function and added test file match the old reviewed #886 bytes. Replacing that one function with its main version reproduces the complete main module AST; the standalone patch ID matches the original identity-only delta.
Independent private-copy validation:
- 547 trajectory tests plus 21 additional identity/deduplication controls passed (568 total). On main, the new 16 identity cases give exactly 10 expected failures / 6 passes; the 21 additional controls also pass there. Ruff/format clean.
- 36 default/reconciliation/roundtrip/manual cases have byte-identical actual outcomes between main and this head (26 successful outcomes, 10 matching refusals). Three additional successful native manual-edit controls also match exact token/flag/logprob values, covering user-authored unsourced messages, removing earlier turns, and edited Completions prompts. Network access was forbidden for these comparisons.
- All 1,189 archived source files rehash unchanged. Tokenizer SHA256:
1917cb970fc88f9a9227bc6dc119b8429e1b8350e31e5eacd203dcfad2d76c88.
These are bounded behavior comparisons, not proof of arbitrary manual-history causal correctness; the broader #883 limitations remain separate. Captured-prompt hashing adds work proportional to prompt length; no performance qualification is claimed here. Fresh hosted CI for the retargeted main base remains separate/pending; old-base event statuses are not its receipt. No research adoption, source-fence override, merge or deployment performed.
Evidence: /home/brad/.local/share/mccarthy/art886-standalone-review-20260910/REVIEW.md, source proof, test logs and behavior receipts.
|
Schulman: Standalone revision is now The first push-triggered CI event captured the previous stacked base before retargeting. The same draft PR was closed/reopened with the source commit unchanged to obtain correct-base CI. Do not treat the obsolete event runs as standalone clearance. Replacement GPU classifier run34517235201 explicitly uses base9a163/head362731, reports validation not required, and passes its gate with the GPU job skipped. Correct-base Prek34517235910 remains in progress. Obsolete runs34516840799/34516840907 are canceled. The old GPU job has a recorded termination receipt and its exact pod |
Captures with different original prompts can be mistaken for duplicates when their response identifiers, generated tokens, and output metadata match. Include captured prompt token IDs in the existing source fingerprint so both training examples survive. Genuine copies of the same capture still deduplicate.
This version is standalone on ART main
9a16366d79eebc36e00f75670965e2629c012b75. It contains only the source-identity change from the previous #886: one runtime function inart.trajectories._tokenize, its explanatory comment, and tests._history.pyis byte-identical to main; #885's automatic history-attribution change is excluded. Public APIs, automatic history construction, explicit reconciliation, manual-history editing, and loss normalization retain main's behavior. No new edited-history validator or probability recomputation is introduced. The broader history-attribution discussion in #883 remains separate.Validation on the standalone source:
3627311444748067482c486909d3642644509efd; earlier stacked-head approvals remain historical evidence.Prepared at Brad's request. This does not merge #885, relax experiment source fences, launch research, or authorize deployment.