Gate hidden-grid cell rebuilds on surviving the reveal - #1220
Merged
Conversation
Every session's poll pass rebuilt the cell widgets of every enabled grid, visible or not. For a grid nobody views the build is a placeholder: its layers hold no viewer token, so frame flushes skip them, and the reveal's 0→1 activation bumps the layer version and rebuilds the cell from scratch, discarding the hidden build. Skip the rebuild for hidden cells unless another session holds a viewer token (then the plotters have computed state and the build is a real plot pre-warming the tab switch); the stale signature/version records make the reveal pass build the cell once. Widgets of an unviewed grid now lag topology, so completing the plot wizard on a stale empty cell can hit an occupied position; add_cell's ValueError was uncaught in that handler (already reachable via the cross-session race, see #1219) and now shows an error notification instead. The other half of #1219 — deriving PlotGrid occupancy from topology so such regions are never offered — is left open. Also insert rebuilt cells directly: the pass holds the document lock, so the pn.state.execute wrapper ran inline anyway and its removed-cell guard was dead code. Fixes #1216. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SimonHeybrock
pushed a commit
that referenced
this pull request
Aug 8, 2026
PR #1221 implements the plan's phase 1 (topology-derived occupancy). Its analysis also corrects an overstatement here: no occupancy check prevents two sessions completing the wizard simultaneously on the same free region, so #1220's ValueError handler remains necessary rather than being subsumed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsNGwvf1r6xa2JH4qiiJFf
A revealed grid's cells are rebuilt because they are missing from the session's widget map, a path that recorded no last_seen_version. The reveal's 0->1 activation, earlier in the same pass but after the version scan, bumps the version as the layer's first frame computes, so the next pass rebuilt the cell again -- discarding a widget that had already been rendered. The discarded widget's plot stays subscribed to the layer pipe, so the grid kept paying for it: measured on the dummy fixture, 40 pipe subscribers instead of 20 and ~100 ms of poll pass per second instead of ~50 for three visible cells, for the rest of the session. Sample each rebuilt cell's layer versions just before the build and record those. Sampling before rather than after the build keeps a bump arriving from the ingestion thread mid-build detectable on the next pass.
With hidden grids' cell builds deferred, a revealed grid's PlotGrid holds nothing but the empty-cell placeholders it was constructed with. Panel materializes the tab from that state on the switch, and the pass requested by _on_active_tab_changed then builds the cells and patches the placeholders away -- so the reveal is two patches, and in between the user is shown "Click to add plot" over positions that are occupied. _BatchedTabs gains an on_activate hook, called before the materialization cascade. That is the only point at which a change to the tab's content still lands in the same materialization: Panel registers _update_active in its own constructor, so any active watcher added afterwards necessarily runs once the models exist. The hook runs the normal poll pass, skipping only the topology reconcile, which would append to or pop from the tabs it is called from; the version stays unrecorded so the next tick reconciles. The hook logs and swallows exceptions. It sits on the tab-switch path, which nothing else guards, and an escaping exception aborts the materialization that follows and leaves the tab blank for the rest of the session. Swallowing degrades to the deferred build, which is guarded by SessionUpdater. Measured on the dummy fixture's 3-cell grid, 8 reveals per variant, one fresh server each: the placeholder flash (388 ms median) is gone, client time-to-cells drops 1680 -> 1472 ms and its main-thread block 1056 -> 822 ms, while the server pays 460 -> 520 ms for the reveal. Two client-side errors stop firing with it: bokeh#15274 (parent_style, which drops the rest of the patch), whose trigger was the placeholder-to-cell children change, and four blocked CDN resource loads from markup panes built after the tab rendered.
bokeh#15274 fired on every run because revealing a plot-grid tab materialized the empty-cell placeholders and then patched the cells over them. Cells are now built before materialization, so a reveal no longer triggers it and the line is only expected where a rendered grid's children genuinely change.
Import-list conflict only: both sides had widened the plot_orchestrator import in plot_grid_tabs_test.py, main adding CellGeometry/CellId and this branch also GridId. Resolved to the union.
SimonHeybrock
marked this pull request as ready for review
August 10, 2026 08:16
Member
Author
|
LGTM. UX is also better since we do not see the empty cells for a second when first switching to a tab -- instead a blank page shows. |
SimonHeybrock
added a commit
that referenced
this pull request
Aug 10, 2026
Both sides added a test at the same point in plot_grid_tabs_test.py -- this branch's click-swallow test and #1220's ValueError-path test. Kept both. #1220 also changed the premise for the ValueError path. Occupancy now comes from topology, so the grid no longer offers a region topology already holds, and the pre-build populates a grid before its tab materializes, so the deferred-build state the old comment described is gone. The one path left is two sessions in the wizard on the same free region at once; comment and test docstring say so.
This was referenced Aug 10, 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
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.
Fixes #1216. Partially addresses #1219 (the
ValueErrorhandler; topology-driven occupancy is left for a follow-up).What
Every session's poll pass rebuilt the cell widgets of every enabled grid in the shared topology, visible or not — at page load and again on every layer version bump (job restarts, plotter swaps, errors). For a grid with no viewer in any session such a build is a placeholder: frame flushes skip layers without viewer tokens, so the reveal's 0→1 activation bumps the layer version and rebuilds the cell anyway, discarding the hidden build. The cost is per session and serialized on the shared IOLoop, so N parked sessions pay it N times back-to-back on every bump.
The poll pass now skips a hidden cell's rebuild unless another session holds a viewer token on one of its layers — the one case where the build is a real plot that survives the reveal and pre-warms the tab switch. Skipped rebuilds leave the session's signature/version records stale, so the reveal pass builds the cell once; N bumps while hidden coalesce into that single build.
That reveal pass now runs before the tab's Bokeh models are materialized rather than after. Deferring the builds means a revealed grid's
PlotGridholds nothing but the empty-cell placeholders it was constructed with, and Panel materializes the tab from whatever it holds at the moment of the switch. Building afterwards made the reveal two patches, and in between showed the user "Click to add plot" over positions that are occupied._BatchedTabsgained a hook called ahead of the materialization cascade — the only point where a change to the tab's content still lands in the same materialization, since Panel registers_update_activein its own constructor and anyactivewatcher added later necessarily runs once the models exist.Consequences handled along the way:
add_cell'sValueErrorwas uncaught in that handler (already reachable via the cross-session wizard race, Empty-cell click can raise ValueError: PlotGrid occupancy comes from inserted widgets, not topology #1219); it now shows an error notification.pn.state.executewrapper around cell insertion described a deferral that never happens (the pass holds the document lock, so it ran inline); the insert is now direct and the dead removed-cell guard is gone.last_seen_version, so the reveal's activation bump — raised earlier in the same pass, after the version scan — rebuilt it again on the next pass. Harmless before this change, because the widget it discarded had been built while hidden and never rendered; with the gate, the discarded widget has been rendered and its plot stays subscribed to the layer pipe, doubling the grid's render cost for the rest of the session. Rebuilds now record the versions as of just before the build.SessionUpdaterguards.Measured
Fake backend, committed dummy fixture (2 grids, 6 cells, 9 layers), server-side timing of
_build_celland the poll pass plus pipe-subscriber and DynamicMap-callback counts, driven through Playwright;mainvs this branch, 2–3 repeats each.Session parked on Workflows, both grids hidden, no other viewer:
With a second session viewing a grid, the parked session still pre-warms that grid's cells and skips the unviewed one's.
The reveal of the 3-cell grid was measured separately, on the merged branch, 8 reveals per variant with a fresh server for each (a session that closed while viewing the grid still holds a viewer token and would pre-warm the next reveal), toggling only the pre-build hook:
So the reveal trades ~110 ms of shared IOLoop for ~220 ms off the browser and the removal of the flash. The ~50 ms per layer of Bokeh model construction (#1203) is untouched and still dominates what remains. The console errors that stop firing are bokeh#15274 (
parent_style), whose trigger was the placeholder-to-cellchildrenchange on theGridBoxand which drops the rest of the patch, and four blocked CDN resource loads from markup panes built after the tab had rendered (the #1154 fallback path).Absolute figures drift by 15–20% between sessions on a contended devcontainer, and the client half is software-raster Chromium, so read the columns against each other rather than as absolutes. Sub-samples of 3 are not enough to separate the client-side difference from that drift; the table is 8 per arm.
Test plan
Automated tests cover the gate (hidden cell not built, reveal builds it, viewer token preserves the pre-warm build, bumps while hidden defer to the reveal), the single build per reveal, the occupied-position error path, and that a revealed grid's cells exist at the moment Panel materializes the tab.
Manual checks:
Merged in
The subscriber leak this branch first surfaced — rebuilding a cell while its grid is visible left the replaced widget's plot subscribed to the layer pipe — was filed as #1224 and fixed on
mainby #1226, which is merged in here. The reveal numbers above are measured on the merged state.🤖 Generated with Claude Code