Skip to content

Delegate Coords and Spool task construction to the node layer - #48

Merged
d-chambers merged 1 commit into
mainfrom
refactor/node-delegation
Aug 4, 2026
Merged

Delegate Coords and Spool task construction to the node layer#48
d-chambers merged 1 commit into
mainfrom
refactor/node-delegation

Conversation

@d-chambers

Copy link
Copy Markdown
Contributor

Description

Continues the canvas/headless parity work from #47: Coords and Spool were the last two widgets that re-implemented node logic instead of delegating to it, so the task that ran on the canvas could differ from the task exported into a saved workflow, and a headless run could diverge from the canvas. After this PR the node layer is the single owner of task construction and validation for both.

Coords

  • CoordsTask now validates eagerly and executes lazily: preflight(patch) runs exactly the validation run() would, raising a structured CoordsValidationError (kind/label/detail). The widget builds its task only via NODE_SPEC.build_task, preflights it, and maps failures onto its existing error banners — its four duplicated validators (mapping, selection, transpose, set-coords) are deleted, along with the eight-branch hand-built task constructor. Canvas-run and exported tasks are now always identical.
  • resolve_set_coord is a shared free function used by run() and the widget's draft validation, so both accept and reject exactly the same set-coords inputs.
  • coords_task_from_params promotes the set_coords draft fields the way canvas rehydration does (drafts win; the *_applied_* mirror is used only when no draft is present). A headless author filling only set_coords_dim/start/stop/step previously got a silent no-op; now they get the coordinate update.
  • One deliberate UX change: data-flipping a non-dimension coordinate now reports through the specific "Invalid flip selection" banner at preflight instead of a generic operation failure at runtime (one test updated accordingly).

Spool

  • New apply_spool_transforms + parse_spool_scalar in nodes/spool.py are the single definition of the select → chunk pipeline, used by SpoolTask.run, SpoolTransformTask.run, the widget's snapshot executor (both settings and input-spool modes, previously two near-identical 30-line blocks), and the display recompute.
  • Parity fix: a chunk value that parses to None (e.g. the literal text None) now disables chunking headlessly — previously it chunked with a None value while the canvas no-opped. Ellipsis (...) parsing is preserved (ast.literal_eval handles it).
  • Deleted widget copies: _apply_chunk_transform, _apply_select_transform, _parse_chunk_scalar, _iter_active_select_filters, and the test-only _snapshot_loader/_load_from_example; their tests now exercise load_spool_from_settings and the shared transforms at the node layer.

Net −180 lines in src/. CHANGELOG updated under Unreleased → Fixed.

Counterpart review: the Codex CLI was unavailable (usage limit until Aug 8), so no counterpart review was run; an adversarial self-review record covering the same risk areas (closure late-binding, validation ordering, draft-promotion precedence, Ellipsis parsing, settings-sync freshness) is in the untracked .scratch/review_node_delegation.md. That self-review caught and fixed one real issue: the draft-promotion precedence initially preferred the applied mirror over drafts, which contradicts canvas rehydration semantics.

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes.
  • documented new behavior with docstrings and a CHANGELOG entry.
  • included tests (new tests/test_nodes/test_coords.py; node-level source-routing/chunk-parsing tests in test_spool_node.py; widget tests repointed at the delegated paths).
  • run prek run --all-files and pytest tests (2106 passed, 44 skipped; one unrelated canvas test flaked once in test_orange_qt.py and passes in isolation, with its file, and on a full-suite rerun).
  • for hot-path changes, run python scripts/bench_compare.py --baseline main (table below) and added the benchmark label.

Benchmarks

Full suite (core + qt) vs origin/main — threshold 20%: 0 slower, 0 faster, 33 compared, control −6.1% (ok):

benchmark                                                baseline       head    median       min status
-------------------------------------------------------------------------------------------------------
...estWaterfallBenchmarks::test_compute_default_levels     9.19ms     9.93ms    +8.0%    +1.9%   --
...aterfallBenchmarks::test_render_alternating_patches    79.20ms    84.52ms    +6.7%    -3.9% noisy   (baseline rel stdev 13%)
...s.py::TestTaskBenchmarks::test_callable_source_hash    980.5us     1.01ms    +2.7%    +1.8%   --
...marks.py::TestTaskBenchmarks::test_task_fingerprint     6.01ms     6.17ms    +2.5%    +0.4%   --
...marks.py::TestTaskBenchmarks::test_port_spec_cached    948.3us    965.7us    +1.8%    +0.1% noisy   (baseline rel stdev 11%)
...py::TestWaterfallBenchmarks::test_render_same_patch    86.63ms    88.17ms    +1.8%    +8.6%   --
...stTaskBenchmarks::test_function_task_class_creation    322.1us    327.6us    +1.7%    -1.6%   --
...rks.py::TestGraphBenchmarks::test_validate_uncached    311.3us    315.9us    +1.5%    -0.5%   --
...py::TestGraphBenchmarks::test_deserialize_from_dict    113.7us    114.7us    +0.9%    +0.7%   --
...TestPlotAxesBenchmarks::test_nearest_value_unsorted     1.68ms     1.69ms    +0.5%    +1.1% noisy   (baseline rel stdev 11%)
...rks.py::TestGraphBenchmarks::test_serialize_to_dict     96.5us     96.8us    +0.4%    -0.9%   --
...chmarks.py::TestTaskBenchmarks::test_port_spec_cold    158.0us    157.9us    -0.1%    -1.5%   --
...estWaterfallBenchmarks::test_should_reset_identical     1.56ms     1.55ms    -0.6%    +0.7% noisy   (baseline rel stdev 12%)
...::TestGraphBenchmarks::test_ensure_validated_cached    677.5us    672.3us    -0.8%    -2.1%   --
...s.py::TestWiggleBenchmarks::test_render_small_patch   135.73ms   134.03ms    -1.3%    +0.8%   --
...s.py::TestTaskBenchmarks::test_extract_return_names     1.19ms     1.18ms    -1.4%    -0.2%   --
...arks.py::TestGraphBenchmarks::test_build_chain_pipe    738.4us    724.7us    -1.9%    -0.4%   --
...TestPlotAxesBenchmarks::test_nearest_axis_ascending     1.77ms     1.74ms    -1.9%    -2.6%   --
...ks.py::TestExecutionBenchmarks::test_get_provenance     65.7us     64.3us    -2.0%    -1.1%   --
...arks.py::TestGraphBenchmarks::test_topological_sort    255.8us    249.4us    -2.5%    -3.8%   --
...s.py::TestImportBenchmarks::test_import_spool_utils     5.70ms     5.52ms    -3.3%    -1.6% noisy   (baseline rel stdev 17%)
...chmarks.py::TestExecutionBenchmarks::test_run_chain    308.0us    297.8us    -3.3%    +0.2%   --
...marks.py::TestExecutionBenchmarks::test_run_diamond    202.9us    195.8us    -3.5%    -0.9%   --
...chmarks.py::TestExecutionBenchmarks::test_map_chain     1.48ms     1.40ms    -5.1%    -0.9% noisy   (baseline rel stdev 16%)
...mplingBenchmarks::test_percentile_levels_subsampled    925.0us    877.9us    -5.1%    -0.3%   --
...estPlotAxesBenchmarks::test_nearest_axis_descending     2.01ms     1.91ms    -5.2%    -5.0%   --
...::TestExecutionBenchmarks::test_build_runtime_specs    854.9us    810.4us    -5.2%    -0.1% noisy   (baseline rel stdev 16%)
...nnotationMaskBenchmarks::test_box_mask_object_dtype    10.79ms    10.02ms    -7.2%    -4.1%   --
...s.py::TestExecutionBenchmarks::test_stream_pipeline    106.2us     98.2us    -7.6%    -1.4% noisy   (baseline rel stdev 23%)
...ks.py::TestAnnotationMaskBenchmarks::test_span_mask    395.9us    365.7us    -7.6%    -5.7%   --
...TestAnnotationMaskBenchmarks::test_box_mask_numeric     3.03ms     2.75ms    -9.2%    -1.2% noisy   (baseline rel stdev 11%)
...rks.py::TestSamplingBenchmarks::test_strided_sample    157.8us    143.2us    -9.2%    -6.0% noisy   (baseline rel stdev 14%)
...arks.py::TestImportBenchmarks::test_import_workflow    11.02ms     9.19ms   -16.6%    -6.2% noisy   (baseline rel stdev 12%)

The worst median (+8.0%, waterfall default-levels) is in code this PR does not touch and its min-delta is +1.9% — machine noise, not a regression.

The Coords and Spool widgets were the last two that re-implemented node
logic instead of delegating to it, so their canvas behavior could drift
from a headless run of the same saved parameters.

Coords:
- CoordsTask validates eagerly and executes lazily (_validated_call);
  preflight(patch) runs the exact validation run() would, raising a
  structured CoordsValidationError (kind/label/detail).
- The widget builds its task only via NODE_SPEC.build_task and maps
  preflight failures onto its existing banners; the four widget-side
  validator copies (mapping, selection, transpose, set-coords) are gone.
- resolve_set_coord is shared by run() and the widget's draft validation.
- coords_task_from_params promotes set_coords draft fields the way canvas
  rehydration does (drafts win; applied mirror used when no draft), so a
  headless caller filling only the draft fields no longer gets a silent
  no-op.
- Data-flip of a non-dim coordinate now reports through invalid_selection
  at preflight instead of operation_failed at runtime.

Spool:
- New apply_spool_transforms and parse_spool_scalar in nodes/spool.py are
  the single definition of the select -> chunk stages, used by both node
  tasks and the widget's snapshot executor and display recompute.
- A chunk value parsing to None (e.g. the text 'None') now disables
  chunking headlessly, matching the canvas chunk controls.
- Deleted the widget copies: _apply_chunk_transform,
  _apply_select_transform, _parse_chunk_scalar,
  _iter_active_select_filters, and the test-only _snapshot_loader and
  _load_from_example; their tests now target the node layer.

New Qt-free tests in tests/test_nodes/test_coords.py and additions to
tests/test_nodes/test_spool_node.py.
@d-chambers d-chambers added the benchmark Run the CodSpeed benchmark workflow on this PR label Aug 4, 2026
@d-chambers
d-chambers merged commit 25476ae into main Aug 4, 2026
10 of 11 checks passed
@d-chambers
d-chambers deleted the refactor/node-delegation branch August 4, 2026 13:41
d-chambers added a commit that referenced this pull request Aug 4, 2026
Close out the three remaining small divergences from the architecture
review (following #47 and #48):

Select:
- SelectionState.apply_select_params now accepts patch=None and builds
  the display-only state itself; the widget's hand-built copy of that
  branch (basis derivation + field-by-field state assembly) is gone.

Filter:
- The mode whitelists moved to nodes/filter.py as FilterMode /
  SavgolFilterMode Literals; the params models validate them, so a
  headless SavgolFilterParams(mode='reflect') fails at validation with
  a clear message instead of inside SciPy. The widget's combo options
  are derived from the same Literals via get_args, so UI and validation
  cannot drift.
- The legacy Gaussian-settings migration stays widget-side on purpose:
  it handles the pre-row-list flat-settings shape, which cannot reach
  the typed params layer (pre-_state workflows load as defaults).

PlayAudio:
- All Qt-free DSP moved to nodes/playaudio.py: patch validation, rate
  inference, PCM normalization (prepare_pcm_audio), default time scale,
  volume-to-dB mapping, output-rate clamping, and playback resampling.
  Only the QAudioSink/QAudioFormat half remains in the widget.
- The node's time_scale/volume_percent params are now consumed: the new
  render_audition(patch, params) returns device-ready PCM headlessly.

New tests: tests/test_nodes/test_playaudio.py (params-driven audition),
mode-validation tests in test_filter_params.py; the DSP unit tests now
target the node functions.
d-chambers added a commit that referenced this pull request Aug 4, 2026
Close out the three remaining small divergences from the architecture
review (following #47 and #48):

Select:
- SelectionState.apply_select_params now accepts patch=None and builds
  the display-only state itself; the widget's hand-built copy of that
  branch (basis derivation + field-by-field state assembly) is gone.

Filter:
- The mode whitelists moved to nodes/filter.py as FilterMode /
  SavgolFilterMode Literals; the params models validate them, so a
  headless SavgolFilterParams(mode='reflect') fails at validation with
  a clear message instead of inside SciPy. The widget's combo options
  are derived from the same Literals via get_args, so UI and validation
  cannot drift.
- The legacy Gaussian-settings migration stays widget-side on purpose:
  it handles the pre-row-list flat-settings shape, which cannot reach
  the typed params layer (pre-_state workflows load as defaults).

PlayAudio:
- All Qt-free DSP moved to nodes/playaudio.py: patch validation, rate
  inference, PCM normalization (prepare_pcm_audio), default time scale,
  volume-to-dB mapping, output-rate clamping, and playback resampling.
  Only the QAudioSink/QAudioFormat half remains in the widget.
- The node's time_scale/volume_percent params are now consumed: the new
  render_audition(patch, params) returns device-ready PCM headlessly.

New tests: tests/test_nodes/test_playaudio.py (params-driven audition),
mode-validation tests in test_filter_params.py; the DSP unit tests now
target the node functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmark Run the CodSpeed benchmark workflow on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant