Refactor: isolate hierarchical worker run state#1459
Conversation
📝 WalkthroughWalkthroughThis PR replaces drain-based orchestration with explicit per-run lifecycle APIs, run-scoped task and error tracking, run-aware synchronized TensorMap operations, scheduler failure callbacks, and post-fence cleanup across C++, Python bindings, workers, tests, and documentation. ChangesRun-fenced runtime
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant Orchestrator
participant Scheduler
participant CommDomain
Worker->>Orchestrator: begin_run()
Worker->>Orchestrator: close_run_submission(run_id)
Scheduler->>Orchestrator: report_task_error(slot, message)
Scheduler->>Orchestrator: on_consumed(task_slot)
Worker->>Orchestrator: wait_run(run_id)
Worker->>CommDomain: execute pending releases
Worker->>Orchestrator: release_run(run_id)
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/simpler/worker.py`:
- Around line 5681-5694: Update the run teardown flow surrounding
`_cleanup_l3_l2_regions()` and `_orch._release_run(run_id)` to preserve any
primary exception from the orchestration callback or `_wait_run()`. Capture
cleanup and release failures for reporting or exception chaining, but ensure the
original run failure is re-raised when one exists; only propagate teardown
failures when no primary exception occurred.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9958a00-5747-4a22-ac63-7a65126cc0e3
📒 Files selected for processing (28)
docs/comm-domain.mddocs/hierarchical_level_runtime.mddocs/orchestrator.mddocs/remote-l3-worker-design.mddocs/scheduler.mddocs/task-flow.mdpython/bindings/worker_bind.hpython/simpler/orchestrator.pypython/simpler/task_interface.pypython/simpler/worker.pysrc/common/hierarchical/orchestrator.cppsrc/common/hierarchical/orchestrator.hsrc/common/hierarchical/ring.cppsrc/common/hierarchical/ring.hsrc/common/hierarchical/scheduler.cppsrc/common/hierarchical/scheduler.hsrc/common/hierarchical/tensormap.cppsrc/common/hierarchical/tensormap.hsrc/common/hierarchical/types.cppsrc/common/hierarchical/types.hsrc/common/hierarchical/worker.cppsrc/common/hierarchical/worker_manager.cppsrc/common/hierarchical/worker_manager.htests/ut/cpp/hierarchical/test_orchestrator.cpptests/ut/cpp/hierarchical/test_scheduler.cpptests/ut/cpp/hierarchical/test_tensormap.cpptests/ut/py/test_worker/test_error_propagation.pytests/ut/py/test_worker/test_l3_l2_orch_comm.py
|
CI triage update: Affected jobs:
The fork token cannot re-run upstream jobs ( |
4d7eb57 to
12b6e19
Compare
72dbbb3 to
7b7b206
Compare
- Track task completion and first errors on per-run fences - Namespace TensorMap entries by run and protect concurrent access - Keep Worker.run blocking while removing global drain dependencies - Cover run identity, failure isolation, and resource lifetime - Keep run bookkeeping non-throwing on the scheduler and worker threads, where an escaping exception would terminate the process - Carry the orchestration failure message onto the run fence so a submission failure is retrievable from the run, not just the caller - Hold runs_mu_ across both the quiescence test and Ring compaction: begin_run takes only that mutex and no slot is allocated without a building run, so a run registered mid-compaction keeps its first slot - Close the run inside the block that releases it, so a failure there cannot strand the run id or skip run-owned resource cleanup
Summary
Worker.run()blocking and returningNoneSeries scope
This is PR 1 of the worker async-preparation series. It intentionally does not expose
RunHandle, admit overlapping runs, or change device ordering. Those behaviors remain in later independently reviewable PRs.Review follow-ups applied
on_task_failed_cbis invoked fromScheduler::worker_done, which runs on aWorkerThreadwhoseon_complete_call sits outside its try/catch, andon_consumed_cbruns insideScheduler::run(), which has no try/catch. Both reach run accounting, so an unknown run id there would terminate the process.find_run()now resolves without throwing, and a count underflow is recorded as the run's error instead of thrown._fail_run_submissiontakes an optional message, so a submission failure is retrievable from the run and not only from the raising caller — required onceRunHandlelands in PR 2.begin_run()takes onlyruns_mu_, so testing quiescence under the scheduler loop mutex alone still allowed: test quiescent → releaseruns_mu_→ another threadbegin_run()+ allocate its first slot →reset_to_empty()invalidates it.compact_if_quiescent()now holdsruns_mu_across both the quiescence test andreset_to_empty(). This is sufficient because both allocator call sites (alloc(),reserve_outputs_and_slot()) are gated behindcurrent_building_run(), so no slot can be allocated while no run is building. Lock order is scheduler loop mutex thenruns_mu_, matching the scheduler'son_consumedpath.close/fail_run_submissioncannot strand the run id or skip run-owned resource cleanup.RunPhase::PREPAREDwhile one run builds at a time.Added regression tests: error reporting and consumption for an already-released run are ignored rather than fatal, and a failed submission carries its message to the fence. The compaction race is fixed structurally (the test and the reset are now under one mutex); a deterministic unit test would need an injection hook to stop a thread mid-compaction, and a timing-based one would be flaky, so none was added.
Testing
ctest --test-dir tests/ut/cpp/build -LE requires_hardware)pytest tests/ut)Tested commit:
7b7b206d.The architecture-probe and hardware runs quoted in earlier revisions were against
4d7eb574, which is no longer this branch's head, and have not been re-run. GitHub simulation CI remains the automated regression stage.