diff --git a/docs/design/dsh5-10-replay-preference-rows.md b/docs/design/dsh5-10-replay-preference-rows.md index 84592ca2a..d175a3af0 100644 --- a/docs/design/dsh5-10-replay-preference-rows.md +++ b/docs/design/dsh5-10-replay-preference-rows.md @@ -1,6 +1,6 @@ # DSH5-10: replay-grounded preference rows from undo/redo history (SLM-418) -**Status:** partial slice, in progress (ninth increment). +**Status:** partial slice, in progress (tenth increment). **Claim class:** `wiring`. **Honest verdict:** not yet dispositioned -- this PR extends a scoped subset, not the full issue. @@ -35,7 +35,13 @@ pairs corpus (no captured conversation-trace data exists anywhere in this repo -- see below), a scope decision for the other six patterns' history- control/merge actions (none has a row in the typed policy's action space), the four-baseline comparison, held-out benefit measurement, and turn-depth/ -context-view ablations. +context-view ablations. This slice does not attempt that scope decision; +instead it closes a smaller, independent gap on the generic TwoTower demo +corpus side: two of the seven named patterns (`partial_rollback`, +`fork_then_choose_one_branch`) were still absent from +`scripts/build_replay_preference_pairs.py`'s demo corpus even though +extraction has covered them since the fifth slice -- see "Tenth slice" +below. ## What this PR delivers @@ -547,6 +553,79 @@ there. * `harness.experiments.argument_preference` registered fresh (`v1`, initial registration) in `src/slm_training/resources/versions.json`. +## Tenth slice + +This slice does not attempt the ninth slice's flagged scope decision +(what an "action row" means for a history-control or merge action in the +typed policy's action space) -- that remains genuinely open. Instead it +closes a smaller, independent gap on the generic TwoTower demo-corpus side +that the eighth and ninth slices both left explicitly unaddressed: two of +the seven named patterns were still absent from +`scripts/build_replay_preference_pairs.py`'s demo corpus even though +`extract_replay_preference_rows` has covered them since the fifth slice. + +* Two new standalone scratch traces, `build_demo_partial_rollback_scenario` + and `build_demo_fork_choose_scenario`, mirror the exact shapes + `tests/test_dsl/test_replay_preference.py`'s own + `test_partial_rollback_yields_a_row_for_the_second_consecutive_undo` and + `test_fork_then_return_to_original_branch_yields_a_distinct_relation` + already verify -- root->edit1->edit2->undo->undo for `partial_rollback` + (a second consecutive undo, no intervening edit), and + root->edit->fork->checkout(pre-fork branch) for + `fork_then_choose_one_branch` -- using the script's existing toy 4-state + cycling operator rather than inventing a new one. +* The three trace-building scenarios' near-duplicated pack/library/root + setup was factored into two shared helpers, `_new_toy_trace` and + `_apply_cycle`, rather than copied a third time; `build_demo_trace` + itself is behavior-unchanged (same rows, same pairs) after the refactor, + which the unmodified `test_build_demo_trace_exercises_three_named_patterns` + test confirms. +* `main()` now combines four sources instead of two: **7 rows total** (2 + `edit_then_undo` + 1 each of `undo_then_redo` / `checkout_another_state` / + `partial_rollback` / `fork_then_choose_one_branch` / `merge_success`), **6 + render**. `undo_then_redo` is still the only drop, for the same + structural reason the seventh slice documented (never a bug on this + fixture family). The `partial_rollback` scenario's own extra + `edit_then_undo` row is kept and counted honestly rather than + cherry-picked out, matching every prior slice's convention of reporting + everything a scenario extracts. +* **6 of the 7 named patterns are now exercised in this demo TwoTower + corpus.** Only `pronoun_focus_followup` remains absent here -- and + deliberately so: the ninth slice already covers it on the separate + `typed_operator_policy` argument-preference path (a different pair + format from this script's generic TwoTower one), and wiring it into this + corpus too would just duplicate that slice's already-documented + structural (feature-identical-siblings) null-training finding rather + than add new information. +* Reran the full training chain against the richer 6-pair corpus: + ```bash + python -m scripts.train_model --train-dir src/slm_training/resources/data/train/wf_smoke_v2 \ + --model twotower --context-backend scratch --steps 8 \ + --run-id replay_pref_sft_ckpt3 --no-sync-checkpoints --device cpu --seed 0 + python -m scripts.build_replay_preference_pairs \ + --out outputs/data/preference/replay_demo_pairs_v4.jsonl + python -m scripts.train_preference train \ + --checkpoint outputs/runs/replay_pref_sft_ckpt3/checkpoints/last.pt \ + --pairs outputs/data/preference/replay_demo_pairs_v4.jsonl \ + --out-dir outputs/runs/replay_pref_dpo3 --steps 12 --device cpu + ``` + SFT step: `last_loss=32.610084533691406` again -- the same deterministic + artifact every prior `wf_smoke_v2`/seed-0/8-step row in this ledger + reproduces. Preference step, now over 6 pairs instead of 3: + `{"steps": 12, "last_loss": 1.1219162940979004, "mean_loss": + 1.0559024934967358, "n_pairs": 6, "reference_free": true}` + (`outputs/runs/replay_pref_dpo3/preference_summary.json`, not committed). + Both commands again well under `MAX_RUN_MINUTES=3`. +* **Still not a training or held-out-benefit claim.** `n_pairs=6` on a + scratch fixture with no held-out split is a broader, more structurally + diverse smoke corpus (now covering 6 of 7 named patterns instead of 4), + not evidence the signal helps the model or generalizes. The ninth + slice's flagged scope decision for wiring history-control/merge actions + into the typed policy's own action space remains the actual open item + for the issue's real training target. +* `harness.preference.replay_pairs` bumped `v3` -> `v4` in + `src/slm_training/resources/versions.json`. + ## Reproducibility ```bash @@ -562,3 +641,5 @@ Result (seventh-slice PR #1125, real run in a fresh `.venv-dsh510`, same environ Result (eighth-slice PR #1126, real run in a fresh `.venv-dsh510`, same environment recipe as above, stacked on top of PR #1125 which was still unmerged when this slice started): `72 passed` (71 from the seventh slice + 1 new). Also verified: `ruff check` clean; `python -m scripts.verify_version_stamps --check --base origin/main` -- `ok (1 component(s) touched)`; `python -m scripts.repo_policy` -- `ok`; `python -m scripts.verify_decode_invariants` -- clean. Plus the reran training chain described above; `outputs/runs/replay_pref_sft_ckpt2/` and `outputs/runs/replay_pref_dpo2/` are not committed (`outputs/` is gitignored). Result (this PR, ninth slice, real run in the same `.venv-dsh510`, stacked on top of PR #1126 which was still unmerged when this slice started): `92 passed` (72 from the prior slices + 6 new in `test_argument_preference.py`, plus `test_typed_operator_policy.py`'s own 14 pre-existing tests now included in this suite's reproduction command for the first time since this slice touches that module's consumer surface). Also verified: `ruff check` clean on both new files; `python -m scripts.verify_version_stamps --check --base origin/main` -- `ok (2 component(s) touched)`; `python -m scripts.repo_policy` -- `ok`; `python -m scripts.verify_decode_invariants` -- clean. No end-to-end training run in this slice (the `train_typed_operator_argument_preference` calls are inside the test suite itself, proving the mechanism works on a synthetic distinguishable pair and correctly plateaus on the real fixture's feature-identical pair -- not a separate `outputs/`-writing run). + +Result (this PR, tenth slice, real run in a fresh `.venv-autotrain` -- Python 3.12.3, `torch==2.5.1+cu124` (CPU-only invocation via `--device cpu`), `pip install -e ".[dev,grammar]"`, plus `env -u NODE_OPTIONS npm ci` in `src/apps/openui_bridge` for the G2/G8 schema-oracle gate, same recipe as every prior slice's note above -- stacked on top of PR #1127 which was still unmerged when this slice started, base commit `9be1464`): `94 passed` (92 from the ninth slice + 2 new in `tests/test_scripts/test_build_replay_preference_pairs.py`). Also verified: `ruff check` clean on both changed files; `python -m scripts.verify_version_stamps --check --base origin/claude/great-dirac-ni43oh-pronoun-focus-policy` -- `ok (1 component(s) touched)`; `python -m scripts.repo_policy` -- `ok`; `python -m scripts.verify_decode_invariants` -- clean. Plus the reran training chain described above (SFT checkpoint + 6-pair demo corpus + preference-training pass, both commands well under `MAX_RUN_MINUTES=3`); `outputs/runs/replay_pref_sft_ckpt3/` and `outputs/runs/replay_pref_dpo3/` are not committed (`outputs/` is gitignored). diff --git a/scripts/build_replay_preference_pairs.py b/scripts/build_replay_preference_pairs.py index 557c8d1c8..b4448523a 100644 --- a/scripts/build_replay_preference_pairs.py +++ b/scripts/build_replay_preference_pairs.py @@ -37,6 +37,22 @@ so it needs its own two-branch fixture with disjoint-target edits, mirroring the shape ``tests/test_dsl/test_operator_merge.py`` already verifies is valid, replayable, and order-invariant. + +Two more standalone scratch traces -- ``build_demo_partial_rollback_scenario`` +and ``build_demo_fork_choose_scenario`` (tenth slice) -- cover +``partial_rollback`` (a second consecutive undo, not preceded by an +intervening edit) and ``fork_then_choose_one_branch`` (a checkout back +across a fork boundary), mirroring the exact shapes +``tests/test_dsl/test_replay_preference.py``'s own +``test_partial_rollback_yields_a_row_for_the_second_consecutive_undo`` and +``test_fork_then_return_to_original_branch_yields_a_distinct_relation`` +already verify, using this script's own toy cycling operator rather than +inventing a new one. That leaves ``pronoun_focus_followup`` as the only +named pattern absent from this demo TwoTower corpus -- the ninth slice +covers it on the separate `typed_operator_policy` argument-preference path +instead (see the disposition doc's "Ninth slice"); wiring it here too would +just duplicate that slice's already-documented structural finding on the +generic pair format this script produces. """ from __future__ import annotations @@ -71,6 +87,7 @@ create_conversation_trace, extract_merge_preference_row, extract_replay_preference_rows, + fork_conversation, merge_conversation_branches, redo_conversation, undo_conversation, @@ -144,14 +161,14 @@ def _execute(state: OperatorStateV1, _arguments) -> OperatorMutationV1: raise ValueError("demo.no_transition") -def build_demo_trace() -> tuple[DslPack, OperatorLibraryV1, ConversationTraceV1]: - """A single scratch trace exercising three of the seven named patterns. - - root -> edit -> undo (edit_then_undo) -> redo (undo_then_redo) -> edit - -> checkout(root) (checkout_another_state). Deterministic (fixed seeds, - no randomness); see the module docstring for why this is not real - captured conversation data. - """ +def _new_toy_trace( + *, branch_label: str +) -> tuple[DslPack, OperatorLibraryV1, ConversationTraceV1]: + """One fresh, standalone scratch conversation using this script's own + toy 4-state cycling operator (see ``_execute``). Every demo scenario + below starts from its own call to this helper so scenarios never share + trace/branch state -- each is independently replayable and its rows + are independently attributable to that one scenario.""" base_pack = get_pack("openui") root_state = OperatorStateV1.from_source(base_pack, _SOURCE) declaration = AstOperatorV1( @@ -167,7 +184,7 @@ def build_demo_trace() -> tuple[DslPack, OperatorLibraryV1, ConversationTraceV1] ) library = OperatorLibraryV1((RegisteredOperatorV1(declaration, _execute),)) pack = replace(base_pack, operator_library=library) - branch = branch_fingerprint(root_state.state_digest, _sha("demo-branch")) + branch = branch_fingerprint(root_state.state_digest, _sha(f"demo-branch-{branch_label}")) root_table = _table(root_state, branch, seed=1) trace = create_conversation_trace( pack=pack, @@ -175,27 +192,41 @@ def build_demo_trace() -> tuple[DslPack, OperatorLibraryV1, ConversationTraceV1] root_reference_table=root_table, provenance=_provenance(root_state), ) + return pack, library, trace - def apply_once(current: ConversationTraceV1, *, seed: int) -> ConversationTraceV1: - result = library.apply( - pack, current.current.state, _OPERATOR_ID, (), _provenance(current.current.state) - ) - assert result.succeeded and result.state is not None - return append_operator_turn( - current, - pack=pack, - library=library, - application=result.application, - output_reference_table=_table(result.state, current.current.branch_digest, seed=seed), - ) - edited = apply_once(trace, seed=2) +def _apply_cycle( + pack: DslPack, library: OperatorLibraryV1, trace: ConversationTraceV1, *, seed: int +) -> ConversationTraceV1: + result = library.apply( + pack, trace.current.state, _OPERATOR_ID, (), _provenance(trace.current.state) + ) + assert result.succeeded and result.state is not None + return append_operator_turn( + trace, + pack=pack, + library=library, + application=result.application, + output_reference_table=_table(result.state, trace.current.branch_digest, seed=seed), + ) + + +def build_demo_trace() -> tuple[DslPack, OperatorLibraryV1, ConversationTraceV1]: + """A single scratch trace exercising three of the seven named patterns. + + root -> edit -> undo (edit_then_undo) -> redo (undo_then_redo) -> edit + -> checkout(root) (checkout_another_state). Deterministic (fixed seeds, + no randomness); see the module docstring for why this is not real + captured conversation data. + """ + pack, library, trace = _new_toy_trace(branch_label="main") + edited = _apply_cycle(pack, library, trace, seed=2) original_child_id = edited.current_state_id undone = undo_conversation(edited, provenance=_provenance(edited.current.state)) redone = redo_conversation( undone, target_state_id=original_child_id, provenance=_provenance(undone.current.state) ) - edited_again = apply_once(redone, seed=3) + edited_again = _apply_cycle(pack, library, redone, seed=3) checked_out = checkout_conversation_state( edited_again, target_state_id=edited_again.root_state_id, @@ -204,6 +235,55 @@ def apply_once(current: ConversationTraceV1, *, seed: int) -> ConversationTraceV return pack, library, checked_out +def build_demo_partial_rollback_scenario() -> ( + tuple[DslPack, OperatorLibraryV1, ConversationTraceV1] +): + """A second, standalone scratch trace reaching ``partial_rollback``. + + root -> edit1 -> edit2 -> undo (edit_then_undo, edit2's state) -> undo + (partial_rollback, edit1's state) -- mirrors + ``test_partial_rollback_yields_a_row_for_the_second_consecutive_undo``. + Both rows this trace yields are kept and reported honestly (this + scenario is not cherry-picked down to only the new relation). + """ + pack, library, trace = _new_toy_trace(branch_label="rollback") + edited_once = _apply_cycle(pack, library, trace, seed=2) + edited_twice = _apply_cycle(pack, library, edited_once, seed=3) + undone_once = undo_conversation( + edited_twice, provenance=_provenance(edited_twice.current.state) + ) + undone_twice = undo_conversation( + undone_once, provenance=_provenance(undone_once.current.state) + ) + return pack, library, undone_twice + + +def build_demo_fork_choose_scenario() -> ( + tuple[DslPack, OperatorLibraryV1, ConversationTraceV1] +): + """A third, standalone scratch trace reaching ``fork_then_choose_one_branch``. + + root -> edit (main branch) -> fork (new branch) -> checkout(main) -- + mirrors + ``test_fork_then_return_to_original_branch_yields_a_distinct_relation``. + """ + pack, library, trace = _new_toy_trace(branch_label="fork") + edited = _apply_cycle(pack, library, trace, seed=2) + main_branch_state_id = edited.current_state_id + forked = fork_conversation( + edited, + branch_nonce_digest=_sha("demo-fork-target"), + reference_seed=8, + provenance=_provenance(edited.current.state), + ) + checked_out = checkout_conversation_state( + forked, + target_state_id=main_branch_state_id, + provenance=_provenance(forked.current.state), + ) + return pack, library, checked_out + + def _merge_branch_edit( *, base_pack: DslPack, @@ -383,31 +463,38 @@ def resolve(node: ConversationStateNodeV1): return [row], ([pair] if pair is not None else []) -def main(argv: list[str] | None = None) -> int: - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument( - "--out", type=Path, default=Path("outputs/data/preference/replay_demo_pairs.jsonl") - ) - args = parser.parse_args(argv) - - pack, library, trace = build_demo_trace() - trace_report = extract_replay_preference_rows( +def _extract_and_render( + pack: DslPack, library: OperatorLibraryV1, trace: ConversationTraceV1 +) -> tuple[list[OperatorReplayPreferenceRowV1], list[PreferencePair]]: + report = extract_replay_preference_rows( trace, pack=pack, library=library, provenance_for=_provenance ) - trace_pairs = render_replay_preference_pairs( - trace_report.rows, + pairs = render_replay_preference_pairs( + report.rows, resolve_node=trace.node, pack=pack, library=library, provenance_for=_provenance, ) + return list(report.rows), pairs + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--out", type=Path, default=Path("outputs/data/preference/replay_demo_pairs.jsonl") + ) + args = parser.parse_args(argv) + trace_rows, trace_pairs = _extract_and_render(*build_demo_trace()) + rollback_rows, rollback_pairs = _extract_and_render(*build_demo_partial_rollback_scenario()) + fork_rows, fork_pairs = _extract_and_render(*build_demo_fork_choose_scenario()) merge_rows, merge_pairs = build_demo_merge_scenario() - all_rows = list(trace_report.rows) + merge_rows - all_pairs = trace_pairs + merge_pairs - counts_by_relation = dict(trace_report.counts_by_relation) - for row in merge_rows: + all_rows = trace_rows + rollback_rows + fork_rows + merge_rows + all_pairs = trace_pairs + rollback_pairs + fork_pairs + merge_pairs + counts_by_relation: dict[str, int] = {} + for row in all_rows: key = row.semantic_relation.value counts_by_relation[key] = counts_by_relation.get(key, 0) + 1 diff --git a/src/slm_training/resources/versions.json b/src/slm_training/resources/versions.json index 0589ae006..c8226fae6 100644 --- a/src/slm_training/resources/versions.json +++ b/src/slm_training/resources/versions.json @@ -7957,7 +7957,7 @@ ] }, "harness.preference.replay_pairs": { - "version": "v3", + "version": "v4", "kind": "harness", "paths": [ "src/slm_training/harnesses/preference/replay_pairs.py", @@ -7966,6 +7966,11 @@ "tests/test_scripts/test_build_replay_preference_pairs.py" ], "history": [ + { + "version": "v4", + "date": "2026-07-27", + "note": "SLM-418 (DSH5-10) tenth slice: adds build_demo_partial_rollback_scenario and build_demo_fork_choose_scenario, two more standalone scratch traces mirroring test_replay_preference.py's own partial_rollback and fork_then_choose_one_branch fixtures, using the script's existing toy cycling operator (refactored the trace-building preamble into shared _new_toy_trace/_apply_cycle helpers rather than duplicating it a third time). main() now combines four sources: 7 rows (2 edit_then_undo + 1 each of undo_then_redo/checkout_another_state/partial_rollback/fork_then_choose_one_branch/merge_success), 6 render (still only undo_then_redo drops, same structural reason as before). 6 of 7 named patterns are now exercised in this demo TwoTower corpus; only pronoun_focus_followup remains absent here (covered instead on the separate typed_operator_policy argument-preference path added in the ninth slice). No change to the sixth slice's renderer itself" + }, { "version": "v3", "date": "2026-07-27", diff --git a/tests/test_scripts/test_build_replay_preference_pairs.py b/tests/test_scripts/test_build_replay_preference_pairs.py index e139b9b65..5557bc942 100644 --- a/tests/test_scripts/test_build_replay_preference_pairs.py +++ b/tests/test_scripts/test_build_replay_preference_pairs.py @@ -7,7 +7,9 @@ from scripts.build_replay_preference_pairs import ( _provenance, + build_demo_fork_choose_scenario, build_demo_merge_scenario, + build_demo_partial_rollback_scenario, build_demo_trace, main, ) @@ -28,6 +30,32 @@ def test_build_demo_trace_exercises_three_named_patterns() -> None: } +def test_build_demo_partial_rollback_scenario_yields_the_second_consecutive_undo() -> None: + pack, library, trace = build_demo_partial_rollback_scenario() + + report = extract_replay_preference_rows( + trace, pack=pack, library=library, provenance_for=_provenance + ) + # Both rows this trace produces are kept, honestly -- the first undo is + # its own edit_then_undo row, the second (no intervening edit) is the + # new partial_rollback relation this scenario targets. + assert report.counts_by_relation == { + ReplayPreferenceRelation.EDIT_THEN_UNDO.value: 1, + ReplayPreferenceRelation.PARTIAL_ROLLBACK.value: 1, + } + + +def test_build_demo_fork_choose_scenario_yields_one_fork_then_choose_row() -> None: + pack, library, trace = build_demo_fork_choose_scenario() + + report = extract_replay_preference_rows( + trace, pack=pack, library=library, provenance_for=_provenance + ) + assert report.counts_by_relation == { + ReplayPreferenceRelation.FORK_THEN_CHOOSE_ONE_BRANCH.value: 1, + } + + def test_build_demo_merge_scenario_yields_one_real_merge_success_pair() -> None: rows, pairs = build_demo_merge_scenario() @@ -50,23 +78,31 @@ def test_main_writes_a_real_pairs_file_and_reports_honest_counts( assert exit_code == 0 report = json.loads(capsys.readouterr().out) assert report["corpus_kind"] == "fixture_or_scratch" - assert report["rows"] == 4 + assert report["rows"] == 7 assert report["counts_by_relation"] == { - "edit_then_undo": 1, + "edit_then_undo": 2, "undo_then_redo": 1, "checkout_another_state": 1, + "partial_rollback": 1, + "fork_then_choose_one_branch": 1, "merge_success": 1, } # undo_then_redo never renders here: redo and "reapply the same # deterministic zero-arg operator" are the identical text by # construction, so the renderer's dedup guard correctly declines it. - assert report["pairs_rendered"] == 3 + assert report["pairs_rendered"] == 6 assert report["pairs_dropped"] == 1 pairs = load_pairs(out_path) assert len(pairs) == report["pairs_rendered"] relations = {pair.meta["semantic_relation"] for pair in pairs} - assert relations == {"edit_then_undo", "checkout_another_state", "merge_success"} + assert relations == { + "edit_then_undo", + "checkout_another_state", + "partial_rollback", + "fork_then_choose_one_branch", + "merge_success", + } for pair in pairs: assert pair.meta["pair_corpus"] == "replay_preference" assert pair.chosen != pair.rejected