feat: pulseplot v1 — native live pulse component replaces PNG display - #67
Conversation
2014ef5 to
b9622cb
Compare
…m raw data, replacing PNG display The solve emits pulse telemetry as additive run.log lines and the inspector renders it natively — theme-aware SVG, updating every iteration (5 Hz cap) instead of every 6th via PNG. iter_<N>.png stays a run-dir/archival artifact (plot_pulse remains canonical for publication-grade output) but the inspector no longer reads it, which also structurally removes the mid-write PNG read race. Wire format (prototype-grade, additive to the stdout tee — unknown lines are ignored by all existing consumers, β contract freeze untouched): AMICODE_PULSE_META drives=2 knots=50 labels="a_1","a_2" bounds=-0.2:0.2,-0.2:0.2 AMICODE_PULSE iter=6 dt=0.204082 a=<values ,-sep; drives ;-sep> (~1KB/iter) - run_dir_reader: pinned grammar (NUM domain shared with AMICODE_ITER, incl. Inf/NaN) + PulseStream cross-line policy: records before meta dropped; last meta wins and its shape governs; count-mismatched records and inconsistent metas ignored (duplicate metas are expected — the tailer re-reads from offset 0 on truncation). - Delivery: replay ingest forwards meta + the NEWEST record only (no history burst); the live tailer forwards meta and each record in order (on a live run the tailer is the only meta carrier); replay-seeded meta arms the live stream across mid-flight run switches; LogTailer.poke() self-attaches when run.log appears early. - Host: postPulse with pre-materialization buffering (meta + newest record, replayed before any buffered completion) and a 5 Hz trailing-edge throttle. New webview messages: pulsemeta, pulse. - Component: media/ui/components/pulseplot.ts — stacked per-drive panels matching plot_pulse (zero-order-hold stairs, bounds band + dashed limits, per-drive labels, shared time axis); fixed-order categorical series tokens riding VS Code's theme chart colors (dev fallbacks validated light + dark); >512 knots stride-decimated; owns idle / warming / no-pulse-data states. - Template: PulseEmitCallback on the same solver-agnostic (primal, iter) hook as LivePulsePlotCallback, delegating to it and printing the lines. (update! qualified — Makie exports the same name; drive component is :u with :a fallback.) - Demo re-recorded from a converged solve (F=0.9998): run.log carries META + 61 records (round-trip verified, 0 dropped); PNGs + pulse.jld2 NOT bundled — the demo ships only what replay exercises. - Dead code from the PNG display path removed: preview component, frame delivery/buffering/dedup, ITER_PNG_RE, img-src CSP grant, runs-root/workspace resource roots. - Dev harness (pnpm run dev:pulseplot) for visual iteration + render bench; excluded from the VSIX. Implements #66 (closed; design record and scope amendment there). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f4dc575 to
5b90af0
Compare
Rchari1
left a comment
There was a problem hiding this comment.
Reviewed the full diff (grammar/policy core, delivery, host, component, template emitter, tests, demo, packaging). This is a well-built slice — verdict at the bottom.
What I verified
- Grammar +
PulseStreampolicy: the pinned regexes handle the%.6gemitter output (incl.Inf/NaN, negative boundslo:hi); the cross-line policy (records-before-meta dropped, last-meta-wins + governs shape, mismatched records/metas ignored) is enforced in one gate both delivery paths feed, and the tests pin every edge including the truncation re-read. - Ordering races are solved by design, not by flushing: the view's
pulsecase is plot-only ("the badge must not regress"), so a throttled record landing aftercompletedis legal — this cleanly replaces the old flush-before-complete coupling. Buffered replay order (meta → newest record → completion) keeps terminal state the last word. - Delivery: replay forwards meta + newest record only (no history burst); a new meta invalidates stale records (
newestPulse = undefined); replayed meta arms the live stream for mid-flight switches;LogTailer.poke()self-attach closes a real FSEvents race (andattach()is now double-attach-safe). - Security tightened:
img-srcdropped from the CSP entirely, runs-root + workspace roots removed fromlocalResourceRoots— the view renders purely from message data. Nice side effect of the migration. - Template emitter: delegates to
LivePulsePlotCallbackfirst (PNG cadence intact — the #43 iter-0 + guard frames still write), qualifiedupdate!(the Makie collision is real), try/catch so telemetry can never kill a solve, META emitted once before the solve. - Packaging:
dev/**VSIX-ignored, harness build artifact gitignored, demo re-recorded (META + 61 records verified inrun.log).
[should-fix, this PR or a named fast-follow] AGENTS.md is now misleading — agent-authored scripts get a dead inspector. The run-dir-contract section (line 68) still says iter_<N>.png is "the live plot the Inspector shows", and never mentions AMICODE_PULSE_META/AMICODE_PULSE. Post-this-PR that's false: PNGs are archival; the inspector renders only from the pulse lines. The template covers the happy path (agents copy it), but an agent that hand-authors a script per AGENTS.md — including the documented DTO-0.9.6 fallback path — will emit PNGs the inspector ignores and no pulse lines, so the panel sits on "warming up" for the entire solve and then jumps to completed. The PR's follow-up list has a "GA contract-docs pass", but AGENTS.md is the agent-facing prompt, not docs — I'd update its contract bullet (point at the template's PulseEmitCallback idiom, mark the lines prototype-grade) before an agent freelances a script.
[nit] stale docstring: file_watcher.ts:117 — the poll-backstop rationale still says it guards against missed "iter_*.png creations … (no live frames)"; frames are gone, the poll now backstops latest swings / FINISHED / tailer-poke.
Merge mechanics: stacked on #65 (kate/inspector-view-reorg) — #65 merges first, then this (or re-target). CI is green on the stack head; 94 unit tests incl. the grammar/policy/buffering/throttle suites.
Verdict: good to merge once #65 lands, with the AGENTS.md drift addressed here or as an immediate named follow-up — it's the only finding with real user impact, and it's a docs-level fix. Everything else is nit-grade. (Not merging — per Raghav's call.)
| Piccolo.NamedTrajectories.update!(traj, collect(view(primal, 1:(traj.dim * traj.N))); type = :data) | ||
| end | ||
| # Drive component name differs by problem flavor (:u current, :a legacy). | ||
| A = something(traj.u, traj.a, missing) |
There was a problem hiding this comment.
[nit] something(traj.u, traj.a, missing) — if NamedTrajectories' property access throws for a missing component (rather than returning nothing), the :a fallback is unreachable: traj.u raises before something can fall through, and the outer catch eats it (warn, no telemetry). Works for this template's problem (you verified :u on real solves), but if the legacy-:a fallback is meant to be real, a components-membership check (:u ∈ keys(traj.components) ? traj.u : traj.a-style) would make it actually reachable. Harmless either way thanks to the try/catch — worst case is no pulse telemetry with a warning.
There was a problem hiding this comment.
Fixed in 7eed2c3 with the membership check (:u in traj.names ? traj.u : …). One precision on the failure mode: something is eager, so traj.a was evaluated even on the :u path — it worked because NamedTrajectories returns nothing for missing components (that's how we found :a was wrong in the first place: eachrow(::Nothing)). But that's undocumented behavior to lean on, so agreed the membership check is strictly better. Verified against a saved real trajectory (picks :u, 2×50) + a fresh converged solve (F=0.9998, 61 records).
| } | ||
|
|
||
| postIterationRecord(rec: { iter: number; f_val: number; inf_pr: number; inf_du: number }): void { | ||
| if (!this.view) return; // ok to drop iter records if not visible — image stream is the canonical signal |
There was a problem hiding this comment.
[nit] Stale comment — "image stream is the canonical signal" refers to the removed PNG path. Post-#66 the run.log lines are the canonical signal (as the bufferedPulseMeta docstring above correctly says: "the log line is the canonical signal — dropped means gone").
There was a problem hiding this comment.
Fixed in 7eed2c3 — comment now states the real rationale (next record refreshes the row seconds later; switchToRun's replay re-ingests history; pulse events are buffered precisely because the log line is their only delivery).
… docstring Review should-fix: AGENTS.md (the agent-facing prompt) still described iter_<N>.png as "the live plot the Inspector shows" and never mentioned the pulse lines — a hand-authored script per that contract would get a dead live plot (stuck on warming, then the no-data hint). The contract section now leads with AMICODE_PULSE_META/AMICODE_PULSE (prototype-grade shapes shown), points at the template's PulseEmitCallback idiom (incl. the DTO-0.9.6 fallback path), and re-labels PNGs as archival. Review nit: the poll-backstop docstring in file_watcher.ts still cited iter_*.png frame creations; it now describes what the tick actually backstops (latest swings, FINISHED, tailer poke/self-attach). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both review findings addressed in the follow-up commit: AGENTS.md's run-dir-contract section now leads with the |
…l-signal comment - Template: drive-component selection now uses a names-membership check instead of something(traj.u, traj.a) — the fallback no longer leans on property access returning `nothing` for missing components (eager `something` evaluates both anyway). Verified on a real saved trajectory (picks :u, 2×50) and a full solve (converged F=0.9998, 61 pulse records). - run_inspector: the drop-iter-records-pre-materialization comment cited the removed "image stream is canonical" rationale; it now states the actual one (next record refreshes the row; replay re-ingests on switch; pulse events are buffered because the log line is their only delivery). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Stacked on #65 — merge that first.
What this does
The inspector's live pulse now renders natively from raw solve data — fully replacing PNG display. The solve emits the pulse's knot values per iteration as additive
run.loglines; the extension parses, routes, and renders them as a theme-aware SVG component.iter_<N>.pngfiles are still written by every solve (archival/publication artifacts —plot_pulseremains canonical there) but the inspector no longer reads them.Wire format (prototype-grade, candidate GA)
Additive lines in the
run.logstdout tee — consumers that don't know them ignore them (anchored-regex no-match), so the β contract freeze is untouched:Numeric domain shared with
AMICODE_ITER(incl.Inf/NaN). Cross-line policy: records before meta dropped; last meta wins and its shape governs; count-mismatched records and internally inconsistent metas ignored (duplicate metas are expected — the tailer re-reads from offset 0 on truncation).The pieces
PulseStreampolicy — pure, in the vscode-free reader; unit-tested against the pinned grammar incl. all edge cases.LogTailer.poke()now self-attaches whenrun.logappears before its retry timer fires.postPulsewith pre-materialization buffering (meta + newest record, replayed before any buffered completion) and the same 5 Hz trailing-edge throttle the PNG path used. New webview messages:pulsemeta,pulse.media/ui/components/pulseplot.ts: stacked per-drive panels matchingplot_pulse(zero-order-hold stairs, bounds band + dashed limits, per-drive labels, shared time axis). Fixed-order categorical series tokens inbrand.cssride VS Code's theme chart colors; dev fallbacks validated (dataviz six checks) light + dark. >512 knots stride-decimated. Owns idle / warming / no-pulse-data states.PulseEmitCallbackin the solve template, on the same solver-agnostic(primal, iter)hook asLivePulsePlotCallback, delegating to it and printing the lines. (Two real-solve-discovered fixes:update!must be qualified — Makie exports the same name — and the drive component is:u, not:a.)run.logcarries META + 61 records (round-trip verified, 0 dropped). "Replay demo run" exercises the whole pipeline.pnpm run dev:pulseplot(visual iteration + AC8 render bench); excluded from the VSIX.Not in scope (follow-ups)
Hover/scrub interactivity · convergence trace (objective + rollout fidelity, needs an
AMICODE_FIDline) · populations/leakage panel · GA contract-docs pass for the pulse lines · upstream atomic-save fix in Piccolo's live-plot callback.Verification
94 unit tests green (grammar, policy, both delivery paths, buffering, throttle, seam), typecheck + build clean, two real solves through amico-run (converged, F ≥ 0.9998), parser round-trip on the real log: 1 meta + 61 records, 0 dropped.
🤖 Generated with Claude Code