Realize the free-threaded parse win + start table-driving query SELECTs - #3427
Merged
Sinity merged 4 commits intoJul 31, 2026
Merged
Conversation
Problem: polylogue-dcz5's own audit noted the parse-stage-split benefit was never proven with numbers (AC item 3) and had no documented rollback (AC item 4). Investigation found the config flag this bead's task description assumed still gates the path (daemon_parse_stage_split) was already deleted on master (ef8a4c3, 2026-07-29) -- the off-writer-hold parse warm is now unconditional and self-degrading, so there was no flag left to flip. What changed: added a benchmark (tests/benchmarks/test_parse_stage_thread_scaling.py) that calls the real daemon dispatch function (_parse_unique_retained_raws) sequentially and thread-parallel against an identical synthetic Codex corpus on this host's live free-threaded python3.14t build, and documented the result plus rollback path in docs/daemon.md. Verification: pytest tests/benchmarks/test_parse_stage_thread_scaling.py --benchmark-enable -p no:xdist -v -s against a scratch archive root -> sequential=0.1901s, parallel(16 workers)=0.0310s, speedup=6.13x (free_threaded=True), consistent with polylogue-7mtf's 3.9x-9.6x control-run range. mypy --strict and ruff clean on the new file. Co-Authored-By: Claude <noreply@anthropic.com>
Problem: polylogue-a7xr.16's own audit found the INSERT side landed column_spec-driven table-driving (write.py, PR #2879) but the SELECT side (archive.py's query_* methods) was untouched -- 14 methods, hundreds of hand-written row[col] accessors, with the bead's own note admitting the SELECT pattern was only "demonstrated, ready for extension", not applied. What changed: archive.py's query_* methods are bespoke multi-table join projections, not full-table reads, so a literal TableColumnSpec.select_column_names swap (as used for write.py's INSERT) does not fit them directly -- forcing that would either change query semantics or require redesigning the query layer, out of scope for a behavior-preserving refactor. Found and fixed the genuine, mechanically-safe instance of the bead's stated drift hazard instead: two exact-duplicate query pairs where the SAME column list + hydration logic was hand-copied byte-for-byte between two methods -- query_messages/query_session_messages (block-fetch-for-messages, ~30 lines each) and query_files/query_session_files (the affected-file outer projection + hydration, ~19 lines each). Extracted each into one shared module-level helper (_fetch_blocks_for_messages + _hydrate_archive_block_row driven by _ARCHIVE_BLOCK_QUERY_COLUMNS; _hydrate_archive_file_query_row driven by _ARCHIVE_FILE_QUERY_COLUMNS, whose (output_name, source_expr) pairs also generate the shared _ARCHIVE_FILE_QUERY_SELECT_SQL fragment used by both methods' outer SELECT) so a column added to either projection is added once. Remaining scope (named, not silently dropped): the other 10 query_* methods (query_actions, query_session_actions, query_session_action_occurrences, query_delegations, query_runs, query_observed_events, query_context_snapshots, query_assertions, query_unit_counts, query_unit_multi_counts) each have their own one-off projection with no internal duplicate to collapse the same way; table-driving those would mean re-deriving their SELECT lists from TableColumnSpec plus a query-shape redesign, which risks behavior drift this PR does not take on. Filed as a follow-up scoped exactly to that remaining set rather than left as an implicit gap. Verification: mypy --strict polylogue/storage/sqlite/archive_tiers/ clean (31 files); ruff check/format clean; devtools render all --check clean (no topology/doc drift). Behavior-preserving -- no test changed; existing coverage is the proof: tests/unit/cli/test_query_verbs_runtime.py, tests/unit/cli/test_query_expression.py, tests/unit/cli/test_query_exec_laws.py, tests/unit/archive/test_query_multi_aggregate.py, tests/unit/storage/test_query_unit_time_expression.py, tests/unit/storage/test_archive_tiers_archive.py, tests/unit/storage/test_archive_tiers_write.py -- 705 passed, 1 skipped, 0 failed. Co-Authored-By: Claude <noreply@anthropic.com>
polylogue-dcz5: recorded that the daemon_parse_stage_split flag was already deleted upstream (ef8a4c3) before this session, plus the measured 6.13x free-threaded parse speedup and rollback path this session added (AC items 3/4). polylogue-a7xr.16: recorded the table-driven query_messages/ query_session_messages and query_files/query_session_files slice landed this session, and why the other 10 query_* methods don't fit the same mechanical treatment. polylogue-aif4: new follow-up (child of a7xr.16) scoping the remaining 10 query_* methods precisely. Co-Authored-By: Claude <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe archive tier now centralizes block and file query projections and hydration. The daemon documentation and benchmark cover raw parse prefetch behavior. Issue records now reflect current implementation status and follow-up scope. ChangesArchive query projection refactor
Parse prefetch documentation and benchmark
Issue tracking updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polylogue/storage/sqlite/archive_tiers/archive.py`:
- Around line 1023-1053: Update the shared _ARCHIVE_BLOCK_QUERY_COLUMNS
projection to include the metadata column, then populate
ArchiveBlockRow.metadata in _hydrate_archive_block_row using the stored row
value and preserving None when absent. Ensure both query_messages and
query_session_messages retain fetched block metadata.
In `@tests/benchmarks/test_parse_stage_thread_scaling.py`:
- Around line 27-29: Update the usage command in the module documentation for
test_parse_stage_thread_scaling to invoke the repository-managed devtools test
harness instead of raw pytest, while preserving the existing benchmark, plugin,
and verbosity options.
- Around line 68-103: Update the worker-count calculation near
parallel_threads_effective() so workers is clamped to at least one, and track
whether at least two workers are available. Apply the free-threaded speedup
assertion in the benchmark’s is_free_threaded branch only when workers >= 2,
while preserving the existing slowdown assertion and single-worker sequential
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2664dd65-5186-441c-b187-75210b1ab907
📒 Files selected for processing (4)
.beads/issues.jsonldocs/daemon.mdpolylogue/storage/sqlite/archive_tiers/archive.pytests/benchmarks/test_parse_stage_thread_scaling.py
Comment on lines
+1023
to
+1053
| _ARCHIVE_BLOCK_QUERY_COLUMNS: tuple[str, ...] = ( | ||
| "block_id", | ||
| "message_id", | ||
| "block_type", | ||
| "text", | ||
| "tool_name", | ||
| "tool_id", | ||
| "semantic_type", | ||
| "tool_input", | ||
| "language", | ||
| "tool_result_is_error", | ||
| "tool_result_exit_code", | ||
| ) | ||
|
|
||
|
|
||
| def _hydrate_archive_block_row(row: sqlite3.Row) -> ArchiveBlockRow: | ||
| """Build an ``ArchiveBlockRow`` from a row selected via ``_ARCHIVE_BLOCK_QUERY_COLUMNS``.""" | ||
|
|
||
| return ArchiveBlockRow( | ||
| block_id=str(row["block_id"]), | ||
| message_id=str(row["message_id"]), | ||
| block_type=str(row["block_type"]), | ||
| text=str(row["text"]) if row["text"] is not None else None, | ||
| tool_name=str(row["tool_name"]) if row["tool_name"] is not None else None, | ||
| tool_id=str(row["tool_id"]) if row["tool_id"] is not None else None, | ||
| semantic_type=str(row["semantic_type"]) if row["semantic_type"] is not None else None, | ||
| tool_input=str(row["tool_input"]) if row["tool_input"] is not None else None, | ||
| language=str(row["language"]) if row["language"] is not None else None, | ||
| tool_result_is_error=(int(row["tool_result_is_error"]) if row["tool_result_is_error"] is not None else None), | ||
| tool_result_exit_code=(int(row["tool_result_exit_code"]) if row["tool_result_exit_code"] is not None else None), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve metadata in the shared block projection.
Line 1023 omits metadata, although ArchiveBlockRow defines it. Line 1041 therefore leaves ArchiveBlockRow.metadata at its default None for every fetched block. This changes both query_messages and query_session_messages from returning stored metadata to returning no metadata.
Proposed fix
"semantic_type",
"tool_input",
+ "metadata",
"language",
"tool_result_is_error",
"tool_result_exit_code",
@@
semantic_type=str(row["semantic_type"]) if row["semantic_type"] is not None else None,
tool_input=str(row["tool_input"]) if row["tool_input"] is not None else None,
+ metadata=str(row["metadata"]) if row["metadata"] is not None else None,
language=str(row["language"]) if row["language"] is not None else None,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _ARCHIVE_BLOCK_QUERY_COLUMNS: tuple[str, ...] = ( | |
| "block_id", | |
| "message_id", | |
| "block_type", | |
| "text", | |
| "tool_name", | |
| "tool_id", | |
| "semantic_type", | |
| "tool_input", | |
| "language", | |
| "tool_result_is_error", | |
| "tool_result_exit_code", | |
| ) | |
| def _hydrate_archive_block_row(row: sqlite3.Row) -> ArchiveBlockRow: | |
| """Build an ``ArchiveBlockRow`` from a row selected via ``_ARCHIVE_BLOCK_QUERY_COLUMNS``.""" | |
| return ArchiveBlockRow( | |
| block_id=str(row["block_id"]), | |
| message_id=str(row["message_id"]), | |
| block_type=str(row["block_type"]), | |
| text=str(row["text"]) if row["text"] is not None else None, | |
| tool_name=str(row["tool_name"]) if row["tool_name"] is not None else None, | |
| tool_id=str(row["tool_id"]) if row["tool_id"] is not None else None, | |
| semantic_type=str(row["semantic_type"]) if row["semantic_type"] is not None else None, | |
| tool_input=str(row["tool_input"]) if row["tool_input"] is not None else None, | |
| language=str(row["language"]) if row["language"] is not None else None, | |
| tool_result_is_error=(int(row["tool_result_is_error"]) if row["tool_result_is_error"] is not None else None), | |
| tool_result_exit_code=(int(row["tool_result_exit_code"]) if row["tool_result_exit_code"] is not None else None), | |
| ) | |
| _ARCHIVE_BLOCK_QUERY_COLUMNS: tuple[str, ...] = ( | |
| "block_id", | |
| "message_id", | |
| "block_type", | |
| "text", | |
| "tool_name", | |
| "tool_id", | |
| "semantic_type", | |
| "tool_input", | |
| "metadata", | |
| "language", | |
| "tool_result_is_error", | |
| "tool_result_exit_code", | |
| ) | |
| def _hydrate_archive_block_row(row: sqlite3.Row) -> ArchiveBlockRow: | |
| """Build an ``ArchiveBlockRow`` from a row selected via ``_ARCHIVE_BLOCK_QUERY_COLUMNS``.""" | |
| return ArchiveBlockRow( | |
| block_id=str(row["block_id"]), | |
| message_id=str(row["message_id"]), | |
| block_type=str(row["block_type"]), | |
| text=str(row["text"]) if row["text"] is not None else None, | |
| tool_name=str(row["tool_name"]) if row["tool_name"] is not None else None, | |
| tool_id=str(row["tool_id"]) if row["tool_id"] is not None else None, | |
| semantic_type=str(row["semantic_type"]) if row["semantic_type"] is not None else None, | |
| tool_input=str(row["tool_input"]) if row["tool_input"] is not None else None, | |
| metadata=str(row["metadata"]) if row["metadata"] is not None else None, | |
| language=str(row["language"]) if row["language"] is not None else None, | |
| tool_result_is_error=(int(row["tool_result_is_error"]) if row["tool_result_is_error"] is not None else None), | |
| tool_result_exit_code=(int(row["tool_result_exit_code"]) if row["tool_result_exit_code"] is not None else None), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@polylogue/storage/sqlite/archive_tiers/archive.py` around lines 1023 - 1053,
Update the shared _ARCHIVE_BLOCK_QUERY_COLUMNS projection to include the
metadata column, then populate ArchiveBlockRow.metadata in
_hydrate_archive_block_row using the stored row value and preserving None when
absent. Ensure both query_messages and query_session_messages retain fetched
block metadata.
Comment on lines
+27
to
+29
| Run with: | ||
| pytest tests/benchmarks/test_parse_stage_thread_scaling.py --benchmark-enable -p no:xdist -v | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the managed test harness.
Line 28 instructs users to run raw pytest. Use devtools test so repository harness hooks run.
Proposed fix
- pytest tests/benchmarks/test_parse_stage_thread_scaling.py --benchmark-enable -p no:xdist -v
+ devtools test tests/benchmarks/test_parse_stage_thread_scaling.py --benchmark-enable -p no:xdist -vAs per coding guidelines, “Prefer devtools test over raw pytest so tests run through the managed repository harness.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Run with: | |
| pytest tests/benchmarks/test_parse_stage_thread_scaling.py --benchmark-enable -p no:xdist -v | |
| """ | |
| Run with: | |
| devtools test tests/benchmarks/test_parse_stage_thread_scaling.py --benchmark-enable -p no:xdist -v | |
| """ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/benchmarks/test_parse_stage_thread_scaling.py` around lines 27 - 29,
Update the usage command in the module documentation for
test_parse_stage_thread_scaling to invoke the repository-managed devtools test
harness instead of raw pytest, while preserving the existing benchmark, plugin,
and verbosity options.
Source: Coding guidelines
Comment on lines
+68
to
+103
| is_free_threaded = parallel_threads_effective() | ||
| workers = min(16, (__import__("os").cpu_count() or 2) - 2) or 1 | ||
|
|
||
| seq_root = tmp_path / "sequential" | ||
| raw_ids = build_independent_raw_corpus(seq_root, raw_count=_RAW_COUNT, avg_payload_bytes=_AVG_PAYLOAD_BYTES) | ||
| sequential_seconds = _time_parse(seq_root, raw_ids, ingest_workers=1) | ||
|
|
||
| par_root = tmp_path / "parallel" | ||
| raw_ids_2 = build_independent_raw_corpus(par_root, raw_count=_RAW_COUNT, avg_payload_bytes=_AVG_PAYLOAD_BYTES) | ||
| assert raw_ids_2 == raw_ids, "corpus builder must be deterministic for a fair before/after comparison" | ||
| parallel_seconds = _time_parse(par_root, raw_ids, ingest_workers=workers) | ||
|
|
||
| speedup = sequential_seconds / max(parallel_seconds, 1e-9) | ||
| print( | ||
| f"\nparse-stage thread scaling (interpreter={sys.version.split()[0]}, " | ||
| f"free_threaded={is_free_threaded}, workers={workers}, " | ||
| f"raw_count={_RAW_COUNT}, avg_payload_bytes={_AVG_PAYLOAD_BYTES}): " | ||
| f"sequential={sequential_seconds:.4f}s, parallel={parallel_seconds:.4f}s, speedup={speedup:.2f}x" | ||
| ) | ||
|
|
||
| # The one correctness property that must never regress: threaded dispatch | ||
| # must never be dramatically slower than sequential. On a GIL build this | ||
| # ratio hovers near 1.0 (0.93x-0.96x was the polylogue-7mtf control-run | ||
| # finding -- thread overhead with no parallel win, not a regression). On | ||
| # a genuinely free-threaded build it should show a real multi-x win. Both | ||
| # cases pass this floor; only a GIL-mistaken-for-free-threaded dispatch | ||
| # (which would reintroduce the ~5000x writer-latency hazard this whole | ||
| # gate exists to prevent) would plausibly show catastrophic slowdown here. | ||
| assert parallel_seconds < sequential_seconds * 1.5, ( | ||
| f"threaded parse dispatch was slower than sequential by more than noise " | ||
| f"(sequential={sequential_seconds:.4f}s, parallel={parallel_seconds:.4f}s) -- " | ||
| "if free_threaded=True this is unexpected and worth investigating before trusting " | ||
| "the daemon's off-writer-hold warm to actually help." | ||
| ) | ||
| if is_free_threaded: | ||
| assert speedup > 1.5, ( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not require a threaded speedup when no threaded worker count is available.
Line 69 returns -1 on a one-CPU host and 1 on two- or three-CPU hosts. _parse_unique_retained_raws uses its sequential branch when ingest_workers <= 1. Lines 102-103 then require a speedup above 1.5x on a free-threaded build, so this benchmark fails deterministically on those hosts.
Clamp the worker count and apply the free-threaded speedup assertion only when at least two workers are used.
Proposed fix
+import os
import sys
import time
@@
- workers = min(16, (__import__("os").cpu_count() or 2) - 2) or 1
+ workers = min(16, max(1, (os.cpu_count() or 1) - 2))
@@
- if is_free_threaded:
+ if is_free_threaded and workers > 1:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| is_free_threaded = parallel_threads_effective() | |
| workers = min(16, (__import__("os").cpu_count() or 2) - 2) or 1 | |
| seq_root = tmp_path / "sequential" | |
| raw_ids = build_independent_raw_corpus(seq_root, raw_count=_RAW_COUNT, avg_payload_bytes=_AVG_PAYLOAD_BYTES) | |
| sequential_seconds = _time_parse(seq_root, raw_ids, ingest_workers=1) | |
| par_root = tmp_path / "parallel" | |
| raw_ids_2 = build_independent_raw_corpus(par_root, raw_count=_RAW_COUNT, avg_payload_bytes=_AVG_PAYLOAD_BYTES) | |
| assert raw_ids_2 == raw_ids, "corpus builder must be deterministic for a fair before/after comparison" | |
| parallel_seconds = _time_parse(par_root, raw_ids, ingest_workers=workers) | |
| speedup = sequential_seconds / max(parallel_seconds, 1e-9) | |
| print( | |
| f"\nparse-stage thread scaling (interpreter={sys.version.split()[0]}, " | |
| f"free_threaded={is_free_threaded}, workers={workers}, " | |
| f"raw_count={_RAW_COUNT}, avg_payload_bytes={_AVG_PAYLOAD_BYTES}): " | |
| f"sequential={sequential_seconds:.4f}s, parallel={parallel_seconds:.4f}s, speedup={speedup:.2f}x" | |
| ) | |
| # The one correctness property that must never regress: threaded dispatch | |
| # must never be dramatically slower than sequential. On a GIL build this | |
| # ratio hovers near 1.0 (0.93x-0.96x was the polylogue-7mtf control-run | |
| # finding -- thread overhead with no parallel win, not a regression). On | |
| # a genuinely free-threaded build it should show a real multi-x win. Both | |
| # cases pass this floor; only a GIL-mistaken-for-free-threaded dispatch | |
| # (which would reintroduce the ~5000x writer-latency hazard this whole | |
| # gate exists to prevent) would plausibly show catastrophic slowdown here. | |
| assert parallel_seconds < sequential_seconds * 1.5, ( | |
| f"threaded parse dispatch was slower than sequential by more than noise " | |
| f"(sequential={sequential_seconds:.4f}s, parallel={parallel_seconds:.4f}s) -- " | |
| "if free_threaded=True this is unexpected and worth investigating before trusting " | |
| "the daemon's off-writer-hold warm to actually help." | |
| ) | |
| if is_free_threaded: | |
| assert speedup > 1.5, ( | |
| import os | |
| import sys | |
| import time | |
| is_free_threaded = parallel_threads_effective() | |
| workers = min(16, max(1, (os.cpu_count() or 1) - 2)) | |
| seq_root = tmp_path / "sequential" | |
| raw_ids = build_independent_raw_corpus(seq_root, raw_count=_RAW_COUNT, avg_payload_bytes=_AVG_PAYLOAD_BYTES) | |
| sequential_seconds = _time_parse(seq_root, raw_ids, ingest_workers=1) | |
| par_root = tmp_path / "parallel" | |
| raw_ids_2 = build_independent_raw_corpus(par_root, raw_count=_RAW_COUNT, avg_payload_bytes=_AVG_PAYLOAD_BYTES) | |
| assert raw_ids_2 == raw_ids, "corpus builder must be deterministic for a fair before/after comparison" | |
| parallel_seconds = _time_parse(par_root, raw_ids, ingest_workers=workers) | |
| speedup = sequential_seconds / max(parallel_seconds, 1e-9) | |
| print( | |
| f"\nparse-stage thread scaling (interpreter={sys.version.split()[0]}, " | |
| f"free_threaded={is_free_threaded}, workers={workers}, " | |
| f"raw_count={_RAW_COUNT}, avg_payload_bytes={_AVG_PAYLOAD_BYTES}): " | |
| f"sequential={sequential_seconds:.4f}s, parallel={parallel_seconds:.4f}s, speedup={speedup:.2f}x" | |
| ) | |
| # The one correctness property that must never regress: threaded dispatch | |
| # must never be dramatically slower than sequential. On a GIL build this | |
| # ratio hovers near 1.0 (0.93x-0.96x was the polylogue-7mtf control-run | |
| # finding -- thread overhead with no parallel win, not a regression). On | |
| # a genuinely free-threaded build it should show a real multi-x win. Both | |
| # cases pass this floor; only a GIL-mistaken-for-free-threaded dispatch | |
| # (which would reintroduce the ~5000x writer-latency hazard this whole | |
| # gate exists to prevent) would plausibly show catastrophic slowdown here. | |
| assert parallel_seconds < sequential_seconds * 1.5, ( | |
| f"threaded parse dispatch was slower than sequential by more than noise " | |
| f"(sequential={sequential_seconds:.4f}s, parallel={parallel_seconds:.4f}s) -- " | |
| "if free_threaded=True this is unexpected and worth investigating before trusting " | |
| "the daemon's off-writer-hold warm to actually help." | |
| ) | |
| if is_free_threaded and workers > 1: | |
| assert speedup > 1.5, ( |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/benchmarks/test_parse_stage_thread_scaling.py` around lines 68 - 103,
Update the worker-count calculation near parallel_threads_effective() so workers
is clamped to at least one, and track whether at least two workers are
available. Apply the free-threaded speedup assertion in the benchmark’s
is_free_threaded branch only when workers >= 2, while preserving the existing
slowdown assertion and single-worker sequential behavior.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be700cd515
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+608
to
+610
| entirely without a code revert, set `POLYLOGUE_INGEST_PARSE_WORKERS=1` in | ||
| the daemon's environment — `resolve_parse_worker_count` then forces every | ||
| parse call (including the warm) down the `ingest_workers <= 1` sequential |
There was a problem hiding this comment.
When an operator follows this rollback, POLYLOGUE_INGEST_PARSE_WORKERS=1 only affects resolve_parse_worker_count; the daemon warm constructs its independent executor through daemon_parse_stage_worker_count() and reads POLYLOGUE_DAEMON_PARSE_STAGE_WORKERS, so it continues dispatching up to cpu_count - 1 parse threads. The documented escape hatch therefore does not disable the warm's parallelism as claimed; the rollback must set both worker controls (or otherwise disable the warm).
AGENTS.md reference: AGENTS.md:L351-L353
Useful? React with 👍 / 👎.
| process's actual interpreter build, using the real daemon dispatch | ||
| function end to end (not a synthetic executor).""" | ||
| is_free_threaded = parallel_threads_effective() | ||
| workers = min(16, (__import__("os").cpu_count() or 2) - 2) or 1 |
There was a problem hiding this comment.
On a free-threaded host exposing three or fewer CPUs, this expression produces at most one worker (and produces -1 for a one-CPU host), so _parse_unique_retained_raws takes the sequential branch for both measurements. The later is_free_threaded assertion nevertheless requires a 1.5x speedup, causing a deterministic false regression on an otherwise correct interpreter; normalize the count and skip the parallel-speedup assertion when fewer than two workers are available.
Useful? React with 👍 / 👎.
# Conflicts: # .beads/issues.jsonl
Sinity
deleted the
feature/daemon/parse-parallelism-and-select-table-driving
branch
July 31, 2026 07:47
Sinity
added a commit
that referenced
this pull request
Jul 31, 2026
…#3432) ## Summary Finishes polylogue-aif4, PR #3427's follow-up bead scoping the 10 `query_*` methods left untouched after that PR's two exact-duplicate extractions. Audited all 10 for the genuine drift hazard (hand-duplicated column lists/dicts that can silently diverge between sibling methods, not the `TableColumnSpec.select_column_names` shape from the INSERT side, which #3427 already established doesn't fit these bespoke join projections) and extracted the two instances that qualified. ## Problem polylogue-aif4 named 10 remaining `query_*` methods and asked for an honest audit: which subset shares the bead's stated drift hazard (a column list or dispatch table hand-copied between sibling methods) versus which are genuinely one-off projections. Two genuine instances were found: 1. `query_actions` and `query_session_actions` both project the identical sixteen-column action shape (`actions` view joined to `sessions`/`messages`) — hydration already went through the shared `_archive_action_query_row()`, but the SELECT column list text itself was hand-duplicated byte-for-byte in both methods. 2. `query_unit_counts` and `query_unit_multi_counts` each hand-maintained an identical copy of the unit -> row-alias map and the unit -> FROM-clause map used to dispatch a terminal aggregate query across the seven SQL-backed query units — both dicts were byte-identical between the two methods. ## Solution - `polylogue/storage/sqlite/archive_tiers/archive.py`: extracted `_ARCHIVE_ACTION_QUERY_COLUMNS` / `_ARCHIVE_ACTION_QUERY_SELECT_SQL` (same `(output_name, source_expr)` pattern PR #3427 used for the file-query projection) and wired both `query_actions` and `query_session_actions` to it. Extracted `_QUERY_UNIT_ROW_ALIAS` and `_query_unit_from_sql_by_unit()` and wired both `query_unit_counts` and `query_unit_multi_counts` to them. **Left alone, with reasons (per aif4's honest-audit framing):** - `query_session_action_occurrences` — selects from raw `blocks` (aliased `u`/`r`, no follow-up relation) instead of the `actions` view, deliberately staying cheap on very large sessions per its own docstring. Output columns rhyme with `query_actions` but the column *sources* genuinely differ; forcing it onto the shared fragment would either lose that cost tradeoff or fake follow-up columns that were never computed. - `query_delegations`, `query_files`/`query_session_files` (already done in #3427), `query_blocks`, `query_assertions` — each a single one-off projection with no sibling to collapse. - `query_runs`, `query_observed_events`, `query_context_snapshots` — structurally rhyme (relation-CTE prefix + join sessions + hydrate via a typed projector) but each hydrates through a *different* domain function (`projected_run_from_row`, `observed_event_from_row`, `context_snapshot_from_row`) with different predicate/order-by shapes. aif4's own note says any table-driving here should start from `run_projection_relations.py`, not `archive.py` — out of scope for this bead. No query-shape redesign, no behavior change: same SQL text is generated, same columns, same aliases, same order. ## Verification - `mypy --strict polylogue/storage/sqlite/archive_tiers/` — clean (via `devtools verify --quick`, exit 0; steps `03-mypy` and `04-render-all` both passed). - `devtools verify --quick` (pre-push gate) — exit 0, all 19 steps green. - Behavior-preserving refactor, no test changed: `devtools test tests/unit/cli/test_query_verbs_runtime.py tests/unit/archive/test_query_multi_aggregate.py tests/unit/storage/test_query_unit_time_expression.py` — 71 passed. `devtools test tests/unit/storage/test_archive_tiers_archive.py tests/unit/cli/test_query_composition_laws.py tests/unit/cli/test_query_expression.py tests/unit/cli/test_query_support_runtime.py` — 482 passed, 1 skipped. `devtools test tests/unit/cli/test_query_exec_laws.py` — 91 passed. Total 644 passed, 1 skipped, 0 failed, across the two extracted surfaces' full test coverage — no test needed to change. - Not run: the heavy full `test` suite (per-PR CI skip convention; runs post-merge). Ref polylogue-aif4. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
Sinity
added a commit
that referenced
this pull request
Jul 31, 2026
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
1 participant
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.
Summary
Finishes two pieces of infrastructure that landed but whose benefit was never proven or completed: measures and documents the free-threaded (3.14t) parse-parallelism win polylogue-dcz5 exists for, and table-drives the two exact-duplicate
query_*SELECT/hydration pairs inarchive.pythat polylogue-a7xr.16 left undone.Problem
polylogue-dcz5: the bead's own 2026-07-29 audit found
daemon_parse_stage_splitstillFalsein production, so the thread-parallel census parse path never ran despite the free-threaded interpreter being live. Investigation here found that gate had already been deleted on master (ef8a4c3d0, same day) — the off-writer-hold parse warm is now unconditional and self-degrading. What remained genuinely open was AC item 3 (measured 7mtf-style benchmark numbers) and item 4 (rollback documentation) — neither existed.polylogue-a7xr.16: column-spec table-driving landed for the messages/blocks INSERT path (
write.py, PR #2879) but the SELECT side (archive.py's 14query_*methods) was untouched — the bead's own note admitted "SELECT pattern demonstrated, ready for extension." Reviewers measured 503 (now 448, pre-existing drift) hand-writtenrow[col]accessors still there.Solution
dcz5: added
tests/benchmarks/test_parse_stage_thread_scaling.py, which calls the real daemon dispatch function (_parse_unique_retained_raws) sequentially and thread-parallel against an identical synthetic Codex corpus, on this host's livepython3.14tfree-threaded interpreter (the same buildpolyloguedruns in production). Documented the result plus a full rollback path (env-var override, revert commits) indocs/daemon.md's new "Free-Threaded (3.14t) Parse Parallelism" section, including explicit confirmation the warm never holds the writer lock — safe regardless of polylogue-de2a's separate, still-open writer-hold contention problem.a7xr.16:
archive.py'squery_*methods are bespoke multi-table-join projections, not full-table reads, soTableColumnSpec.select_column_names(built for write.py's INSERT) doesn't fit them directly without a query-shape redesign — out of scope for a behavior-preserving refactor. Found and fixed the genuine mechanically-safe instance of the bead's stated drift hazard instead: two pairs of methods (query_messages/query_session_messages,query_files/query_session_files) that hand-duplicated the SAME column list + hydration logic byte-for-byte. Extracted each into one shared module-level helper (_fetch_blocks_for_messages/_hydrate_archive_block_rowdriven by_ARCHIVE_BLOCK_QUERY_COLUMNS;_hydrate_archive_file_query_rowdriven by_ARCHIVE_FILE_QUERY_COLUMNS, whose(output_name, source_expr)pairs also generate the shared_ARCHIVE_FILE_QUERY_SELECT_SQLfragment used by both methods' outer SELECT). Filedpolylogue-aif4(child of a7xr.16) scoping the other 10query_*methods precisely, rather than silently dropping them.Verification
tests/benchmarks/test_parse_stage_thread_scaling.py --benchmark-enable -p no:xdist -v -sagainst a scratch archive root:sequential=0.1901s, parallel(16 workers)=0.0310s, speedup=6.13x, free_threaded=True— consistent with polylogue-7mtf's 3.9x-9.6x control-run range.mypy --strict polylogue/storage/sqlite/archive_tiers/— clean (31 files).ruff check/ruff format --check— clean.devtools render all --check— clean (no topology/doc drift).tests/unit/cli/test_query_verbs_runtime.py,test_query_expression.py,test_query_exec_laws.py,tests/unit/archive/test_query_multi_aggregate.py,tests/unit/storage/test_query_unit_time_expression.py,test_archive_tiers_archive.py,test_archive_tiers_write.py— 705 passed, 1 skipped, 0 failed.devtools verify --quick(pre-push gate) — exit 0.testsuite (per-PR CI skip convention; runs post-merge).Ref polylogue-dcz5, polylogue-a7xr.16. Follow-up filed: polylogue-aif4.
🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Documentation
Maintenance