spike(rows-first): two-message state_change protocol behind env gate#787
Draft
paddymul wants to merge 3 commits into
Draft
spike(rows-first): two-message state_change protocol behind env gate#787paddymul wants to merge 3 commits into
paddymul wants to merge 3 commits into
Conversation
Proves the wire shape for "show rows first, compute stats after" without going async. Synchronous server, just message reordering + an ``IOLoop.call_later`` yield between the two ``initial_state`` frames so any ``infinite_request`` from the client gets serviced in between. Gated on ``BUCKAROO_ROWS_FIRST_SPIKE=1``. Default off so existing ``test_load_expr`` tests (which expect a single rebroadcast frame per state change) keep working unchanged. Mechanism: 1. State-change handler sets ``dataflow._defer_summary_sd = True``, then extracts state. The trait cascade still runs (``cleaned`` → ``processed_result`` → ``populate_df_meta``), but ``_summary_sd`` short-circuits — the analysis-pipeline pass is skipped. 2. Phase 1 ``initial_state`` is sent immediately. 3. ``IOLoop.call_later(0.01, _send_stats_update)`` schedules the deferred compute. The 10ms delay gives any ``infinite_request`` the client fires in response to phase 1 a real time window to arrive and get serviced before the stats compute starts. 4. Phase 2: ``recompute_summary_sd()`` triggers the analysis run (which fires ``_populate_sd_cache`` + ``_merged_sd`` downstream), then a second ``initial_state`` is sent. Spike tests in ``tests/unit/server/test_rows_first_spike.py``: - ``test_state_change_emits_two_initial_state_messages``: pins the two-message wire shape. - ``test_infinite_request_between_phases_returns_rows``: pins the key win — ``infinite_request`` between the two phases is serviced and its parquet frame arrives before phase 2. Not for merge as-is — this is exploration on top of #785 to validate the protocol before doing a real (un-gated) implementation. Open questions: - xorq path: ``XorqServerDataflow`` may not route through this ``_defer_summary_sd`` short-circuit; the first test had to drop a "stats actually differ between phases" assertion that was failing on the xorq backend. - 10ms delay is arbitrary. Real prod tuning would measure end-to-end and pick a value that reliably loses the race to a network-bound ``infinite_request`` without feeling sluggish. - Cache hit fast path: when ``filt_sd_key`` is already in ``summary_stats_cache`` (search-box backspace etc.), there's no reason to two-message. Spike doesn't short-circuit that. Sits on top of PR #785. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review-driven cleanup on the spike, no behaviour change: - Hoist ``from tornado.ioloop import IOLoop`` out of ``_handle_buckaroo_state_change`` to the module top (CLAUDE.md bans in-function imports). - Extract ``_apply_state_and_broadcast`` so phase 1 and phase 2 share one copy of session-update + component_config merge + per-client send. Phase 1 still owns the ``session.buckaroo_state = new_state`` assignment. - Rewrite the ``_ROWS_FIRST_SPIKE_PHASE2_DELAY_S`` comment so it doesn't claim 10ms beats "typical browser-to-server WS round-trip" (only true for localhost). Flag the remote-deployment caveat. - Drop the dead ``monkeypatch.setenv`` in the spike test fixture — the module-level constant is resolved at import, so only the ``setattr`` actually flips the gate. Spike tests still pass; the flag-off ``test_load_expr`` suite is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Draft / spike — not for merge. Validates the wire shape for "show rows first, compute stats after" without going async. Synchronous server, just message reordering + an
IOLoop.call_lateryield.Sits on top of #785 (uses
_defer_summary_sd+recompute_summary_sdhooks that this PR adds to the dataflow).What it proves
buckaroo_state_changeproduces twoinitial_stateframes on the wire.infinite_requestfired by the client between the two frames is serviced and its parquet rows arrive before phase 2. Thecall_later(0.01)between phase 1 and phase 2 gives inbound socket reads a real time window to win the race.Test:
tests/unit/server/test_rows_first_spike.py(2 cases).How to try it
Set the env flag and exercise normally:
Default (flag off) keeps today's single-frame rebroadcast. Existing
test_load_exprtests pass unchanged.Open questions
XorqServerDataflowmay not route through the_defer_summary_sdshort-circuit on the baseDataFlow. First spike test had to drop a "stats actually differ between phases" assertion because it was failing on xorq. This is the most important item to resolve before un-gating — without that assertion the spike test would still pass ifrecompute_summary_sd()were a no-op, so we can't detect regression of the spike's whole point.state_changerace. Between phase 1 broadcast and thecall_laterfiring, a secondstate_changecan land. Phase 2 then computes stats against aprocessed_dfthat already reflects the second state, but the broadcast payload mixes phase-2 stats with what the user thinks is the first interaction. The spike doesn't try to address this; needs a serialise / cancel-previous strategy before un-gating.infinite_request.filt_sd_keyfor the new state is already insummary_stats_cache(search-box backspace, oscillation), there's no compute to defer — phase 2 would be near-instant. The spike doesn't short-circuit and emits two frames anyway, which is wasteful.type: "initial_state". JS will re-apply full state twice. Worth deciding whether phase 2 should be a distinct type (e.g.stats_update) so the front-end can target a cheaper re-render of just the stats-driven UI, rather than re-running the fullinitial_statehandler.Stack
?keyprefix marks an optional pinned row #777 ✅ merged (JS?keypinned-rows)merged_sd)🤖 Generated with Claude Code