round 39 opus review — v0.1.45 serve/tokenizer/ple ports + astra qsa patch #

read-only review. no source edits, gpu runs or service actions by me. scope: prompt cache, logit_bias cache, telemetry fallback, ple dedup, astra qsa patch. paths are relative to vendor/strata/ unless noted.

what i compared #

• /tmp/v0145_serve_ple.patch applied to a clean git archive HEAD copy (/tmp/r39_audit_tree, exit 0). the patch does not apply to the dirty tree (serve/server.py:3852 conflicts with the local STRATA_OMIT_IMAGES_WHEN_NO_VISION block); that full git apply failed atomically, so nothing in it landed. the serve/tokenizer parts went in through the serve owner (working-tree server.py = the clean-applied copy plus the local omission block). PLE went in as only upstream commit 36381540 (results/round39/ple.patch: ngram.cpp + ple_reader_test.cpp, no mutex). the later upstream commit that adds the std::recursive_mutex was not imported; the mutex was never in our tree, so nothing was dropped or reverted.
• /tmp/astra_r39_qsa/astra_r39_qsa.patch against current metal/.

verdict #

no remaining correctness blocker once the logit_bias _last removal (already done by the serve owner) is in. three should-fix items before benchmark numbers or a persona eval are attributed to this build: telemetry cost on darwin (S1), answer_room default (S2), prompt-cache cap (S3).

should-fix #

S1. telemetry runs ioreg and creates a Metal device in the server every second (darwin) #

• serve/server.py:6485 serve() always calls start_telemetry(); on darwin gpu_reader now returns _Metal (serve/telemetry.py gpu_reader), so the sampler thread runs ioreg -l -w0 -r -c IOAccelerator every second (measured here on the M5: 39 ms wall, 57 KB of output; the patch says 22 ms) and _Metal.read() calls metal_working_set() on every sample, which does MTLCreateSystemDefaultDevice() (create rule: +1 retain that is never released) and the objc lookups each time. before the port, darwin had no gpu reader, so this is new work in the serving process during decode.
• fix: compute the working set once (_Metal.__init__) and reuse it in read() and in Telemetry.static.
• test: patch telemetry.metal_working_set with a counter, build _Metal(run=fixture), call read() 3x, assert 1 call. before citing any tok/s from this build: paired decode A/B with the sampler on vs gpu_reader forced to _Nvml (same engine, same prompts).
• semantics (not a blocker; it only matters for opt-in min_free_vram_mib / image policy): on this M5 at idle "In use system memory"=15007744 (15 MB) vs "Alloc system memory"=15473033216 (15.5 GB). free = working set − in-use probably reports far more free than there is while the wired arena is up. needs one check with the engine loaded before anyone turns on min_free_vram_mib on the mac.

S2. answer_room is on by default and changes every thinking request on our config #

• serve/server.py:4206-4214: with no reasoning_budget_tokens, every thinking request with max_new >= 1024 gets budget = max_new − max(512, max_new/4). our config has fit_max_tokens: true and 32K context, so a request without max_tokens gets max_new = room and a budget of 75% of it, plus a print on every request (max_new <= 32768 is always true at 32K ctx).
• when it fires, REASONING_WRAP_UP ("I have thought about this long enough; time to give my answer.") is injected into the reasoning stream, and the generation splits into a second engine pass. frontend.py additionally turns Anthropic thinking.budget_tokens into a hard budget.
• effort low keeps this rare, but persona/antislop evals before and after the port are not comparable unless this is pinned. decide explicitly: "answer_room": false in models/K512_iq2_xs/strata-k512-iq2xs.json for parity runs, or accept and note it. the per-request print should go behind a once-flag either way.
• model-card sampling (model_card_sampling): checked: a no-op for our config, because the sampling block sets temperature, top_p, top_k and presence_penalty, which covers every key in both qwen4exp sets.

S3. PromptEncoder keep has no upper bound #

• serve/server.py Service init: keep = conversation_slots(engine) is taken from --conversation-cache-slots with no clamp; the engine accepts up to INT32_MAX (src/program/generate.cpp:2113). each entry holds the full prompt str (1–4 B/char: kaomoji/CJK push the str to UCS-2/4), its ids list (8 B/token) and its mark lists, so ~2–5 MB at 100K tokens. our config passes no slots, so keep = 4 and this is latent.
• fix: keep=min(slots, 16) (or a total-chars budget). test: conversation_slots stub returning 10_000 → assert svc.prompts.keep <= 16; feed 20 unrelated prompts → len(entries) <= keep.

checked and fine #

prompt cache exactness / mutation / concurrency (tools/strata_tokenizer.py PromptEncoder) #

• the margin argument holds for _encode_matching: a match decision at q depends only on text[q:q+max_special_len] and on whether q is in a plain span; the finditer state before a cut c (end of a taken match) is identical when c + max_special_len - 1 <= same. call_end-style lookbehind does not exist in the literal alternation.
• fuzz (/tmp/r39_fuzz.py, /tmp/r39_fuzz2.py, real Swift tokenizer from models/K256_reap_l2_sum/tokenizer, 33 specials, max len 21): conversations with appends, mid-inserts, truncations, specials split across appends (<|im_sta + rt|>), random plain spans, store=False mixed in, keep 1/2/4. 38,033 cases, 8,529 of them reusing a prefix, 0 mismatches vs tok.encode(text, parse_special=True, plain=...).
• mutation: encode(store=True) returns list(ids), store=False returns a list that is not stored, entries are replaced and never changed in place; prepare never mutates ids in place (images build out, force builds a new list), so forget_prompt(encoded_ids) matches. the local omission block runs before encode_prompt (serve/test_image_omission.py covers it).
• concurrency: snapshot under the lock, BPE outside it, replace under the lock against the current list: a race loses reuse at worst, never data.
• pick_slot TURN_ID = 248045 matches <|im_start|> in our vocab.
• parser _wrapped_call_end fast path: sound, because call_end only ever returns positions of a literal </tool_call>, and _call_end_checked is reset on every path that leaves the call state.

logit_bias (serve/logit_bias.py) #

• _last identity+len staleness: found by the parent, already removed by the serve owner.
• content key json.dumps: no collisions that change the result: {1: x} and {"1": x} serialize alike and normalize alike; float/bool keys, NaN and inf never normalize, so they are never stored; False vs 0 and 1 vs "1" serialize differently. results are copied (normalize → dict(...)), and the engine key is an immutable str. the lazy entry[1] write is a benign race (same string).
• cost note only: without _last, a 65K-entry list pays json.dumps on each of the ~5 calls per request. compute the key once in the route if that shows up in profiles.
• note: the Metal engine does not report logit_bias=1, so validate_request refuses non-empty biases on our backend anyway.

PLE dedup (src/kernels/ngram.cpp gather_batch) #

• any-slot matching plus matched[t] reuse is correct (keys are row ids; the table is immutable). dropping n_prefetch >= n_tokens is correct now that duplicates map to one slot. draining every ticket before returning an error fixes the old early return, which left tickets in flight writing into slots that would be reused.
• Metal uses Direct mode (metal/src/strata_window_cli.cpp:945), so this path is live: MTP prefetch_rows (metal/src/engine/window.cpp:2972) feeds the verify window's gather_batch (T = spec+1 ≤ 16). prompt chunks (2048) take the slow path, which drops the prefetches; that is fine.
• the later upstream recursive_mutex commit was not imported (only 36381540 was transplanted), so the table's threading is unchanged from our baseline. on Metal that baseline is fine: ple_th (window.cpp:3434) is joined inside the forward, and the MTP prefetch runs after it, so they never overlap. other backends (the desktop HIP build shares this file; upstream's mutex comment is about prompt-path / decode-window overlap there) carry the same unchanged baseline hazard and are untested in this round. record it in the port notes as "not imported". even with the mutex, issue() copies into impl_->rows before its pending check (pre-existing), so a second issue() while one is in flight corrupts the first ticket's keys: move the pending check first if this ever becomes multi-threaded.

astra qsa patch #

• qsa_merge2 is dispatched only by metal/tests/token_chain.cpp:249 and metal/tests/qsa_parity.cpp:49,159,254. the engine dispatches qsa_merge (v1) at metal/src/engine/window.cpp:1285,2737,3662 and metal/src/engine/prompt_chain.inc:262. qsa2.metal is still compiled into lqsa_ (window.cpp:763), so the change is compile-surface only: zero runtime effect, no end-to-end attribution. the unroll keeps the fma accumulation order (it only hoists loads), which matches astra's bitwise result.
• qsa_prompt.metal clamped-row edits are in the !coop_k4 branches only; on the M5, Window::coop_k4() probes true, so they run only with COOP_K4=0 or on M1/M4. it is a safety port, with no M5 default-path effect. row 0 / scale 0 for empty cells is in bounds whenever the cache holds ≥1 cell.
• moe_mm_parity TK=64 / v2||v3: test-only. the baseline default was already broken (missing mm_gu_42_16_64_16_2, exit 3).

tests i ran (cpu only, PYTHONDONTWRITEBYTECODE=1, from vendor/strata) #

• python -m unittest serve.test_prompt_encoder serve.test_logit_bias serve.test_image_omission serve.test_model_card_sampling serve.test_admit_order: 33 tests OK (before the serve owner's _last fix landed).
• python -m unittest serve.test_server.MacTelemetry serve.test_server.AmdTelemetry: 7 OK.
• python tools/test_strata_tokenizer.py: 11 OK, 3 skipped.
• PromptEncoder fuzz above: 0 mismatches.
• not run: anything on the gpu, ple_reader_test (the parent ran it), the full serve suite.

followup v2 — sdk ownership check for S1 #

• MacOSX.sdk/System/Library/Frameworks/Metal.framework/Headers/MTLDevice.h (CommandLineTools sdk on this machine): MTL_EXTERN id <MTLDevice> __nullable MTLCreateSystemDefaultDevice(void) API_AVAILABLE(macos(10.11), ios(8.0)) NS_RETURNS_RETAINED; so the caller owns +1, and each uncached metal_working_set() call leaked one retain on the device singleton. that is a refcount, not memory per call, and the cost is the per-sample framework call. with the working set cached once, a single unreleased +1 at startup is acceptable. an optional objc_msgSend(dev, sel_registerName("release")) after reading would make it exact, but it is not needed.
• the same header says a call switches a dual-gpu intel mac to the high-power gpu. it does not apply to apple silicon, but it is one more reason to call it once and not every second.
• answer_room: parent approved keeping the live default; it stays disclosed in S2, with no comparable eval numbers claimed.

followup v3 — hardening review (final sources) #

• the fingerprints in results/round39/integration-source-sha256.json match the working tree for all 27 files.
• telemetry: _Metal.__init__ reads the working set once (self.working_set), and both read() and Telemetry.static reuse it. metal_working_set() releases the device in a finally (objc_release, matching the NS_RETURNS_RETAINED annotation); the early if not dev return happens before the try, so a null is never released. covered by test_working_set_is_read_once_for_samples_and_static plus the release assertion. what remains per sample: the 1 Hz ioreg subprocess (39 ms wall measured earlier); its cost on decode is still not A/B measured. Service.free_vram_mib() still builds a fresh reader per call (2 ioreg runs + 1 working-set read), but only on the opt-in and image paths.
• prompt cache: PROMPT_ENCODER_KEEP_MAX = 16 caps slots from args and from INFO, and the default stays 4. covered by IncrementalPrompts (asserts 10**9 → 16 on both paths). tools/strata_tokenizer.py is byte-identical to the fuzzed version, so the 38,033-case result still applies to the final source.
• logit_bias: _last is fully removed and only the bounded JSON-keyed cache remains; nothing else changed.
• reran on the final sources (cpu): test_prompt_encoder, test_logit_bias, test_image_omission, MacTelemetry, AmdTelemetry: 33 OK; IncrementalPrompts: 9 OK.
• verdict: S1 (per-sample device creation) and S3 (cap) are resolved. S2 is accepted and disclosed by the parent.

followup v4 — provenance of results/round39/model-ab/ (read-only; no processes launched) #

• ngram-baseline.cpp is byte-identical to git show HEAD:src/kernels/ngram.cpp (ngram.cpp was clean before the round). against the current file the diff is 15+/11-, i.e. the 36381540 dedup only.
• host archive: 34 members, same names and order as build-metal/libstrata_metal_host.a. extracted and hashed: only ngram.cpp.o and the ranlib __.SYMDEF index differ, and the swapped member equals model-ab/ngram.cpp.o. no other linked archive defines PleTable::gather_batch (libstrata_metal_kernels.a only references it), so link order cannot pick a stale copy. the disassembly of PleTable::gather_batch differs between baseline-cli (340 lines) and candidate-cli (391 lines).
• candidate-cli is byte-identical to build-metal/strata_window_cli, built at 11:59:22, after the last round39 source write (qsa files 11:59:21, ngram.cpp 11:56:32).
• shaders: the runtime directory comes from STRATA_METAL_KERNELS (metal/src/mtl_rt.mm:51), with no on-disk library cache in mtl_rt. candidate-kernels/ equals the current metal/kernels/ file for file (22/22). baseline-kernels/ differs from it only in qsa2.metal and qsa_prompt.metal, matches /tmp/astra_r39_qsa/baseline exactly, and matches the sha256 values in astra's metadata.json for qsa/qsa2/qsa_prompt. the engine-private msl (mtp.metal, mtp_chain.metal, ple.metal, slop4.h) loads from the compiled-in absolute path for both arms; all four were last written before the round (02:57–08:11, and slop4.h on 10-17), so this is shared and unchanged.
• consequence for interpreting results: on the M5 the shader delta is inert (qsa_merge2 is not dispatched, and the non-coop branches are not taken), so the arms differ in effect only by the PLE gather_batch dedup in the MTP verify path.
• suggestions, not blockers: (1) both arms should emit identical token ids, because the dedup returns the same bytes; compare the generated ids per pair as a correctness gate. (2) the 10K case decodes ~31 tokens in ~0.9 s, which is too short a window to resolve a 2% decode delta with 3 pairs; call that case's decode inconclusive unless the spread is clearly under 2%, and read prefill separately.

followup v5 — reading the paired CLI A/B (logs only: results/round39/model-ab/runs.json, summary.json) #

amended at the parent's request: everything below is descriptive. nothing here is timed beyond what runs.json records, and no statistical bound is claimed.

• output token IDs equal in all 5 measured pairs (and the warm pair). aggregate STRATA_SPEC rounds/drafted/accepted are equal within each case for every run of both arms (10K: 13/23/18; 2K: 39/99/88). equal aggregates support comparable work; they do not establish an identical acceptance trajectory, which is not logged and is not inferred from the output IDs.
• drift: decode ms/token rises overall through the session, 28.65 → 33.05 over the 10K series, but not monotonically (10K decode 920.70 ms, then 915.71 ms in consecutive runs; 2K 4213.72 then 4143.13). it stays at 31.5–33.2 in the 2K series. 10K prefill goes 21.6 s → 30.4 s. this affects both arms. sustained-load throttling on a fanless Air is plausible but unmeasured (no thermal readings), so no cause is claimed.
• descriptive drift-adjusted fit (OLS: value ~ run index + arm, computed from runs.json; n = 6 and n = 4, so no confidence intervals are claimed):
  • 10K decode: −5.9 ms (−0.6%), residuals up to ±22 ms.
  • 10K prefill: −431 ms (−1.7%), residuals up to ±1.0 s.
  • 2K decode: +64.6 ms (+1.6%), residuals ±49 ms, 1 dof.
  • 2K prefill: +210 ms (+3.2%), residuals ±232 ms, 1 dof.
• prefill as a same-path comparison: prompt chunks call gather_batch with T = 2048 (one 1984-token chunk for 2K; 4×2048 + 1988 for 10K). old and new code take the same slow path for T > 16, and the shader changes are not executed on the M5 default path. the prefill arm differences nonetheless have opposite signs (−1.7%, +3.2%). this shows the run-to-run instability of this setup; it is not a statistically established noise floor.
• code-path note (an operation count, kept separate from the measurements; it does not enter the verdict): in decode, the verify windows call gather_batch with T ≤ 4. the new matching adds at most 4×16 key comparisons of 64 B each and issues no more SSD read tickets than the old code (its fast path covers every case the old fast path did; the non-fast path is unchanged). its latency was not timed.
• verdict: inconclusive for small performance changes. no consistent slowdown is visible in 10K; the 2K decode differences (+1.5–1.7% per pair) cannot be separated from the drift and instability in this run. a cooled, stationary experiment would be needed to resolve differences at this scale. the parent's wording in audits/r39_integration.md matches this.