003 concurrent scheduler#9
Open
S2thend wants to merge 11 commits into
Open
Conversation
…rtifacts Full speckit artifact set for intra-run concurrent node execution (eager dispatch, first-k merges, graph-definition dispatch order): - spec.md: 4 user stories, FR-001..FR-018, SC-001..SC-008, constitution alignment, clarifications session (eager dispatch + first-k decided) - plan.md: technical context, 15-point constitution check (all pass) - research.md: decisions R1-R11 grounded in the 0.3.0 compatibility audit and empirical probes - data-model.md: state occupancy changes (multi-RUNNING snapshots, straggler records), run-loop entities; snapshot format stays v2 - contracts/scheduler-behavior.md: observable behavior contract C1-C9 - quickstart.md: parallel diamond, first-k racing, k-successes pattern - tasks.md: 33 tasks across setup/foundational/US1-US4/polish - checklists/requirements.md: spec quality gate (all pass)
…nt-scheduler Six findings from /speckit.analyze clarified with maintainer and applied: - U1 (HIGH): define edge classification — an edge is in-loop (back) iff it lies on any cycle (derived from the validator's cycle enumeration); in-loop edges keep 0.3.0 re-entry/hard-stop semantics, forward edges get the record-only straggler rule (spec FR-006, research R5, data-model, contracts C3.6, T007) - I1 (MEDIUM): merge payload snapshotted at fire decision, not slot admission — exactly k results regardless of capacity queuing (FR-005, research R5, data-model, contracts C3.2, T014) - C1 (MEDIUM): record-only straggler rule extended uniformly to partial-input targets (the k=1 case) (FR-005, C3.3, T015) - C2 (MEDIUM): T020 gains constitution-XIV defensive-fallback sub-test - C3 (MEDIUM): new T034 [US2] — stall-detection tests (no false stall with in-flight work; genuine stall still raises) (FR-010) - C4 (LOW): T013 gains queued-node event-silence assertions (FR-007) - D1 (LOW, editorial): FR-004 annotated as the k=fan-in case of FR-005 Total tasks: 34.
…01-T003) Four fixture files generated by the actual 0.3.0 engine plus an 11-test gate module. Baseline verified: 54 passed (43 existing + 11 golden) on the unchanged engine.
…n (T004-T010) Rework Scheduler.run into a completion-driven loop: every ready node is dispatched immediately as an asyncio task (graph-definition order), bounded by the ConcurrencyManager; all finished tasks are harvested before any readiness re-scan. mark_running + NODE_SCHEDULED move to slot admission so queued nodes stay PENDING everywhere observable. Downstream dispatch now classifies edges (in-loop iff on any cycle, from the validator's cycle enumeration): in-loop edges keep exact 0.3.0 re-entry/hard-stop semantics (constitution XIV); forward edges into an already-activated target record the completion only (first-k straggler rule). Partial-input nodes receive their triggering input's result. No-progress error now requires nothing ready AND nothing in flight. Sanctioned test update: the reentry hard-stop unit test pinned the old undifferentiated behavior on an acyclic edge (the audited crash class spec 003 removes); split into back-edge hard-stop (preserved, XIV) + forward-edge record-only. The doctest predicted to need updating in the spec passes unchanged, so the net sanctioned-edit count remains one. Gate: 55 passed (incl. 11-test frozen golden suite), ruff + mypy clean. Smoke: diamond fan-out at capacity 2 runs in 50% of capacity-1 time.
…011-T021, T034) US1: speedup targets met (2 branches/cap2 <=60%, 3/cap3 <=45%), definition-ordered merge payloads under reversed completion, capacity bound with queued-node event silence, first-k exactly-once with straggler recording and fire-decision payload freeze under saturation, partial-input triggering payloads, dispatch-order determinism across 10 hash seeds. US2: golden gate green post-change, sequential-equivalence corpus (pipeline/switch/loop) byte-identical across 10 seeds with strict event alternation, capacity-1 equivalence (gauge + spy-store max-one-running), max_visits under concurrent forward routers, stall detection both ways (no false stall in flight; genuine stall still raises). Spec assumption updated to record the actual sanctioned test edit (the hard-stop unit test split; the predicted doctest passed unchanged). Suite: 70 passed.
…22-T026) US3: original exception types surface unwrapped with a sibling in flight (handler ValueError, unmatched switch route, snapshot-store RuntimeError); after failure surfaces there are zero further events and zero snapshot writes; the cancelled sibling observes CancelledError and never emits completion. Cancellation implementation (cancel-and-await, no TaskGroup) landed in the foundational phase; these tests are its acceptance (T023). US4: resume from a crafted v2 snapshot with two RUNNING nodes re-runs exactly the in-flight nodes and reuses completed results; crash-and- resume roundtrip from a real intermediate snapshot re-executes only unfinished work; every persisted snapshot in both runs stays format version 2. Suite: 76 passed; ruff + mypy clean.
…027-T033) README gains a 'Concurrency Within a Run' section with upgrade notes; the edge-heavy design principle now reflects in-process concurrent dispatch under an explicit capacity policy. Scheduler class docstring gains a fan-out doctest demonstrating definition-ordered merge payloads. New CHANGELOG.md documents 0.4.0 semantics (first-k merges, triggering payloads, deterministic dispatch order, edge classification, failure teardown) and the observable changes for fan-out workflows. docs/ page has no execution-order claims - nothing to sync. Verified: 76 tests passing, doctests green, quickstart patterns run as written, debug tracing silent under default config, ruff + mypy clean.
…ersarial review)
An Opus multi-lens review of the implementation found two real defects,
both fixed with tests:
1. First-k merge cardinality (C3.2/FR-005 contract violation): the
aggregate payload builder appended every completed upstream, ignoring
k. Because the run loop harvests a whole asyncio.wait batch before the
next readiness scan, when more than k inputs complete in one batch the
merge fired once but with >k results, and cardinality varied with
capacity (probe: required=1 diamond -> ['A','B']). Cap the payload at
exactly k in graph-definition order; a no-op when k == fan-in, so
wait-for-all and all single-frontier golden behavior stay
0.3.0-identical.
2. Self-loop re-entry lost (0.3.0 regression): _compute_loop_pairs
derived in-loop edges from Graph._find_cycles, which never reports
length-1 self-cycles, so a self-edge (a->a) was misclassified as a
forward straggler and lost re-entry. Add self-edges to the loop-pair
set explicitly; verified equal against main (3 runs -> {'i': 3}).
Tests: exactly-k cardinality on the batched-completion path,
capacity-invariant cardinality/membership, default-merge wait-for-all,
self-loop re-entry parity, and a priority-admission test closing the
FR-014/C1.3 coverage gap (NODE_SCHEDULED order follows priority, not
definition order). Tightened the golden quorum test to assert exactly-k
(the missing assertion that had masked the bug). Contract C3.2/C3.6 and
research evidence updated.
Suite: 81 passed (was 76); new timing tests stable across 5 runs; ruff,
mypy, doctests clean.
…ion, partial-input tiebreak wording
Addresses four external-review findings:
1. (bug, FR-012/Snapshot-First Recovery) _initial_results_from_snapshot
dropped completed nodes whose payload was None, so a resumed aggregate
saw a shorter input list than a fresh run. Restore completed payloads
unconditionally. Regression test asserts fresh == resumed == [None,'V'].
2. (constitution XV) EventBus.emit silently returned listener exceptions
when no on_error was set. Now log them at warning level (still returned,
still not raised). test_eventbus asserts the warning fires without
on_error and does NOT fire when on_error handles it.
3. (test weakness) The priority-admission test put the best-priority node
first in graph order, so it could not distinguish priority from
dispatch order. Rewritten: the graph-first node gets the WORST priority
and still wins the free slot (FR-018 dispatch order), while the queued
nodes are admitted in strict priority order. Behavior was already
correct per C1.3 ('reorder admission among queued nodes'); only the test
and its docstring overclaimed. No code change — priority governing the
free slot would contradict FR-018 deterministic dispatch.
4. (doc precision) data-model/FR-017/C3.4 said a partial-input node gets
'the input whose completion made it ready', which is ill-defined when
several inputs complete in one batch. Reworded to the deterministic
graph-definition-order tiebreak (matching first-k membership). Tracking
the event-loop-first task was rejected as it would be nondeterministic.
New test pins the same-batch tiebreak.
Suite: 84 passed (was 81); new/reworked tests stable across 5 runs;
ruff, mypy, doctests clean.
…s (membership)
Third external review found the earlier cardinality fix insufficient:
capping the aggregate payload at k in graph-definition order kept exactly
k results but chose the graph-first k, not the k earliest-completing. So
an input that completed in an earlier harvest batch could be dropped when
the k-th completion arrived in a later multi-completion batch. Repro:
graph order a,b,c, required=2, c completes first then a,b in one batch ->
returned ['A','B'], but the k=2 earliest are {c,a} -> ['A','C']. This
violated FR-005/C3.2 ('the k earliest-completing inputs').
Fix: track a monotonic completion sequence in the run loop, assigned in
graph order within each harvest batch (so same-batch ties are
deterministic and results-dict insertion is now deterministic too).
_build_input_payload selects the k winners by that sequence and emits
them in graph-definition order. When k == fan-in this selects all inputs,
so wait-for-all and golden behavior stay 0.3.0-identical. Resume seeds
the sequence for already-completed nodes in graph order (their true
pre-crash order is not persisted; snapshot format stays v2).
This is the fix the FIRST review's fix_suggestion originally proposed and
that I wrongly simplified to graph-first-k. Three regression tests added:
in-run membership, determinism across 10 hash seeds, and resume.
Suite: 87 passed (was 84); new tests stable across 5 runs; ruff, mypy,
doctests clean.
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
1 participant
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.
feat(scheduler): concurrent execution of ready nodes within a single run (parallel fan-out before merge) #8