Refactor: reuse native execution thread per runner - #1654
Conversation
📝 WalkthroughWalkthroughThe PR adds asynchronous L2 run handles, concurrent successor preparation, prepared-run compatibility checks, persistent native execution threads, optional runtime capabilities, and expanded lifecycle tests and documentation. ChangesNative pipeline lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Worker
participant ChipWorker
participant DeviceRunner
participant NativeRunState
Client->>Worker: submit L2 run
Worker->>ChipWorker: prepare and launch run
ChipWorker->>DeviceRunner: submit native execution
DeviceRunner->>NativeRunState: publish completion
Worker->>ChipWorker: poll and finalize
Client->>Worker: wait on RunHandle
Worker-->>Client: return completion
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
tests/st/a2a3/host_build_graph/worker_async_fifo/test_worker_async_fifo.py (1)
482-489: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the new test to the standalone-runner hook.
TestWorkerAsyncWholeRunFifo._run_and_validate_l3invokes the three parent tests by name. This subclass does not override that hook, sotest_incompatible_runtime_env_falls_back_to_depth_oneruns under pytest but never runs through the standalone scene runner. Override the hook in the subclass so both entry points cover the TMR fallback.♻️ Proposed override
`@scene_test`(level=3, runtime="tensormap_and_ringbuffer") class TestWorkerAsyncWholeRunFifoTmr(TestWorkerAsyncWholeRunFifo): """TMR uses the common FIFO with shared-arena compatibility fallback.""" + def _run_and_validate_l3(self, worker, compiled_callables, sub_handles, case, **kwargs): + super()._run_and_validate_l3(worker, compiled_callables, sub_handles, case, **kwargs) + platform = str(worker._config["platform"]) # noqa: SLF001 -- scene-test white-box validation + self.test_incompatible_runtime_env_falls_back_to_depth_one(platform, worker) + def test_incompatible_runtime_env_falls_back_to_depth_one(self, st_platform, st_worker):🤖 Prompt for 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. In `@tests/st/a2a3/host_build_graph/worker_async_fifo/test_worker_async_fifo.py` around lines 482 - 489, Override TestWorkerAsyncWholeRunFifoTmr’s _run_and_validate_l3 hook to invoke the inherited standalone-runner tests plus test_incompatible_runtime_env_falls_back_to_depth_one, preserving the existing parent hook behavior so both pytest and standalone execution cover the TMR fallback.tests/ut/py/test_worker/test_host_worker.py (1)
2521-2541: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd acceptance coverage for a deferred L2 run.
This test pins the deferred phase and the later prepare/launch, but no test calls the acceptance wait while a run sits in phase
"deferred". That is the exact gap behind the_wait_run_handle_accepteddefect flagged inpython/simpler/worker.pyat lines 8633-8646, where the method returns immediately for a deferred run. Add an assertion that the acceptance wait blocks until run 2 leaves the deferred phase.🤖 Prompt for 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. In `@tests/ut/py/test_worker/test_host_worker.py` around lines 2521 - 2541, Add coverage in test_incompatible_successor_defers_prepare_without_failing_its_handle for _wait_run_handle_accepted: invoke it for run 2 while worker._l2_runs[2].phase is "deferred" and assert it remains blocked, then release the first run and verify the wait completes only after run 2 leaves the deferred phase. Preserve the existing prepare/launch and handle-completion assertions.python/simpler/worker.py (1)
3174-3176: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument and centralize the depth-one fallback sentinel.
_native_prepare_requires_depth_onematches a free-text substring produced byChipWorker::prepare_native_run_on_slotinsrc/common/worker/chip_worker.cpp(thePTO_RUNTIME_ERR_PREPARED_INCOMPATIBLEbranch). If that message is reworded, this predicate silently returnsFalse, and an incompatible successor becomes a hard submit failure instead of a deferred retry. Extract the literal into a named module constant and name the producing C++ site in the comment, so the coupling is discoverable from both ends.♻️ Proposed change
+# Must match the message emitted by ChipWorker::prepare_native_run_on_slot for +# PTO_RUNTIME_ERR_PREPARED_INCOMPATIBLE in src/common/worker/chip_worker.cpp. +_NATIVE_PREPARE_DEPTH_ONE_SENTINEL = "native prepare requires depth-one fallback" + + def _native_prepare_requires_depth_one(error: BaseException) -> bool: """Recognize the backend's explicit, correctness-preserving fallback.""" - return "native prepare requires depth-one fallback" in str(error) + return _NATIVE_PREPARE_DEPTH_ONE_SENTINEL in str(error)🤖 Prompt for 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. In `@python/simpler/worker.py` around lines 3174 - 3176, Extract the depth-one fallback sentinel string used by _native_prepare_requires_depth_one into a named module-level constant, and have the predicate reference that constant for its substring match. Document the constant or predicate comment with the producing ChipWorker::prepare_native_run_on_slot PTO_RUNTIME_ERR_PREPARED_INCOMPATIBLE site so this cross-language coupling remains discoverable.tests/ut/py/test_worker/test_startup_readiness.py (1)
697-697: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep completion state per native run.
_wait_native_run()sets one chip-widecompleteflag. After the first run completes,_poll_native_run()returnsTruefor every later token. This can hide stale-completion bugs in repeated submissions.Store completion by token, or reset the state during
_prepare_native_run_with_pipeline_lease().Proposed test-double fix
- self.complete = False + self._completed_tokens = set() - return self.complete + return token in self._completed_tokens - self.complete = True + self._completed_tokens.add(token)Also applies to: 719-725
🤖 Prompt for 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. In `@tests/ut/py/test_worker/test_startup_readiness.py` at line 697, Update the test double’s completion tracking used by _wait_native_run() and _poll_native_run() so completion is scoped to each native-run token rather than shared through one chip-wide self.complete flag. Store per-token completion state, or reset self.complete in _prepare_native_run_with_pipeline_lease(), ensuring later submissions cannot inherit completion from an earlier run.src/common/platform/onboard/host/device_runner_base.cpp (1)
409-437: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden
submit_native_execution's single-task invariant.The occupancy check tests
native_execution_task_but notnative_execution_active_. Once the worker thread takes the task withstd::move(native_execution_task_)(line 422),native_execution_task_becomes empty whiletask()is still executing (native_execution_active_staystrueuntil line 427). During that window, a secondsubmit_native_executioncall passes the occupancy check and overwrites the queue slot, even though the first task has not finished.Today, callers only reach this function after
try_acquire_native_runsucceeds, and that exclusive claim is not released until afterwait_for_execution_complete()runs insimpler_finalize_run, so this is not reachable under the current call graph. Addnative_execution_active_to the guard so the function's own invariant does not silently depend on external callers always honoring the acquire/release protocol.Additionally,
task()at line 425 runs with no catch-all in the worker loop. An exception that escapes the submitted lambda callsstd::terminate()and aborts the whole process, not just the one run. The submitted lambda insimpler_launch_runwraps the risky calls internally, but a defensive catch-all here removes the fragility of relying on every future caller getting that coverage right for a thread that is now reused across the runner's lifetime.🔒 Proposed hardening for the occupancy check and task execution
bool DeviceRunnerBase::submit_native_execution(std::function<void()> fn) { std::lock_guard<std::mutex> lock(native_execution_mu_); - if (native_execution_stop_ || native_execution_task_) return false; + if (native_execution_stop_ || native_execution_task_ || native_execution_active_) return false; if (!native_execution_thread_.joinable()) { native_execution_thread_ = create_thread([this]() { std::unique_lock<std::mutex> lock(native_execution_mu_); while (true) { native_execution_cv_.wait(lock, [this]() { return native_execution_stop_ || static_cast<bool>(native_execution_task_); }); if (native_execution_stop_ && !native_execution_task_) return; std::function<void()> task = std::move(native_execution_task_); native_execution_active_ = true; lock.unlock(); - task(); + try { + task(); + } catch (...) { + // A native run task must never crash the persistent worker thread. + } lock.lock(); native_execution_active_ = false; native_execution_cv_.notify_all(); } }); native_execution_thread_create_count_.fetch_add(1, std::memory_order_relaxed); }🤖 Prompt for 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. In `@src/common/platform/onboard/host/device_runner_base.cpp` around lines 409 - 437, Update DeviceRunnerBase::submit_native_execution so its occupancy guard rejects submissions when either native_execution_task_ is queued or native_execution_active_ is true, preserving the single-task invariant. In the worker loop created there, wrap task() execution in a catch-all handler so exceptions from submitted tasks are contained and do not terminate the process; ensure the worker still reacquires the mutex and clears native_execution_active_ afterward.
🤖 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 `@docs/task-flow.md`:
- Around line 730-731: Update the chip_0 native-run path walkthrough to
distinguish active-frame and successor preparation: the active frame prepares
after staging, while a capable successor may be prepared before ACTIVATE but
must not launch until activation. Keep the existing launch, polling,
finalization, and compatibility endpoint behavior unchanged.
In `@python/simpler/worker.py`:
- Around line 8291-8320: Refactor _l2_progress_locked so the FIFO state lock is
separate from the progress-ownership lock, and release the state lock before
blocking in _wait_native_run or sleeping during progress polling. Replace
time.sleep with _l2_progress_cv.wait using the remaining timeout, while
preserving FIFO handoff and completion handling. Ensure nested callers such as
_submit_l2_locked do not leave an RLock recursion level held across native
waits, so RunHandle.done remains non-blocking.
- Line 8322: Rename the callable parameter in _submit_l2_locked to avoid
shadowing Python’s callable builtin, and update all references within the
method, including the RunHandle keepalive tuple. If compatibility requires
retaining the parameter name in surrounding submit or _submit_locked methods,
suppress A002 only on those signatures.
- Around line 8633-8646: Update _wait_run_handle_accepted so L2 runs in the
"deferred" phase continue waiting instead of being treated as accepted; only
exit once the run crosses the launch fence into a non-prepared, non-deferred
state, while preserving existing error propagation.
In
`@tests/st/a2a3/host_build_graph/native_run_lifecycle/test_native_run_lifecycle.py`:
- Around line 136-139: Update the setup around supports_concurrent_prepare to
call chip_worker.supports_concurrent_native_prepare() and store its returned
capability value, rather than storing the bound method; keep the subsequent
concurrent-preparation condition using that boolean result.
- Line 153: Update the run-stream count assertions in the native lifecycle
tests, including the assertion near chip_worker.run_stream_set_create_count and
its concurrent-successor equivalents, so they execute only for the a2a3
platform. Skip these checks for a2a3sim while preserving all other lifecycle
assertions.
In `@tests/ut/py/test_worker/test_startup_readiness.py`:
- Around line 803-814: Wrap the lifecycle assertions around run_handle and
chip_worker events in a try/finally block, and call w.close() in the finally
clause so the initialized worker is always released even when an assertion or
wait fails.
---
Nitpick comments:
In `@python/simpler/worker.py`:
- Around line 3174-3176: Extract the depth-one fallback sentinel string used by
_native_prepare_requires_depth_one into a named module-level constant, and have
the predicate reference that constant for its substring match. Document the
constant or predicate comment with the producing
ChipWorker::prepare_native_run_on_slot PTO_RUNTIME_ERR_PREPARED_INCOMPATIBLE
site so this cross-language coupling remains discoverable.
In `@src/common/platform/onboard/host/device_runner_base.cpp`:
- Around line 409-437: Update DeviceRunnerBase::submit_native_execution so its
occupancy guard rejects submissions when either native_execution_task_ is queued
or native_execution_active_ is true, preserving the single-task invariant. In
the worker loop created there, wrap task() execution in a catch-all handler so
exceptions from submitted tasks are contained and do not terminate the process;
ensure the worker still reacquires the mutex and clears native_execution_active_
afterward.
In `@tests/st/a2a3/host_build_graph/worker_async_fifo/test_worker_async_fifo.py`:
- Around line 482-489: Override TestWorkerAsyncWholeRunFifoTmr’s
_run_and_validate_l3 hook to invoke the inherited standalone-runner tests plus
test_incompatible_runtime_env_falls_back_to_depth_one, preserving the existing
parent hook behavior so both pytest and standalone execution cover the TMR
fallback.
In `@tests/ut/py/test_worker/test_host_worker.py`:
- Around line 2521-2541: Add coverage in
test_incompatible_successor_defers_prepare_without_failing_its_handle for
_wait_run_handle_accepted: invoke it for run 2 while worker._l2_runs[2].phase is
"deferred" and assert it remains blocked, then release the first run and verify
the wait completes only after run 2 leaves the deferred phase. Preserve the
existing prepare/launch and handle-completion assertions.
In `@tests/ut/py/test_worker/test_startup_readiness.py`:
- Line 697: Update the test double’s completion tracking used by
_wait_native_run() and _poll_native_run() so completion is scoped to each
native-run token rather than shared through one chip-wide self.complete flag.
Store per-token completion state, or reset self.complete in
_prepare_native_run_with_pipeline_lease(), ensuring later submissions cannot
inherit completion from an earlier run.
🪄 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: 37d88084-439e-4df6-a0f9-36fe1fe40368
📒 Files selected for processing (32)
docs/callable-identity-registration.mddocs/dynamic-linking.mddocs/task-flow.mddocs/worker-manager.mdpython/bindings/task_interface.cpppython/simpler/task_interface.pypython/simpler/worker.pysrc/a2a3/platform/onboard/host/device_runner.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/docs/RUNTIME_LOGIC.mdsrc/a2a3/runtime/tensormap_and_ringbuffer/host/runtime_maker.cppsrc/a5/platform/onboard/host/device_runner.cppsrc/a5/runtime/host_build_graph/host/runtime_maker.cppsrc/a5/runtime/tensormap_and_ringbuffer/host/runtime_maker.cppsrc/common/platform/onboard/host/c_api_shared.cppsrc/common/platform/onboard/host/device_runner_base.cppsrc/common/platform/onboard/host/device_runner_base.hsrc/common/platform/sim/host/c_api_shared.cppsrc/common/worker/chip_worker.cppsrc/common/worker/chip_worker.hsrc/common/worker/native_run_launch_signal.hsrc/common/worker/native_run_state.hsrc/common/worker/pto_runtime_c_api.htests/st/a2a3/host_build_graph/native_run_lifecycle/kernels/orchestration/long_vector_orch.cpptests/st/a2a3/host_build_graph/native_run_lifecycle/test_native_run_lifecycle.pytests/st/a2a3/host_build_graph/run_stream_reuse/test_run_stream_reuse.pytests/st/a2a3/host_build_graph/worker_async_endpoint/test_worker_async_endpoint.pytests/st/a2a3/host_build_graph/worker_async_fifo/test_worker_async_fifo.pytests/ut/cpp/common/test_trb_runtime_temp_buffer.cpptests/ut/cpp/hierarchical/test_pipeline_contract.cpptests/ut/py/test_host_runtime_abi.pytests/ut/py/test_worker/test_host_worker.pytests/ut/py/test_worker/test_startup_readiness.py
💤 Files with no reviewable changes (3)
- src/a5/runtime/tensormap_and_ringbuffer/host/runtime_maker.cpp
- tests/ut/py/test_host_runtime_abi.py
- src/a5/runtime/host_build_graph/host/runtime_maker.cpp
| | 6 | chip_0 child process | validate the frame and resolve its digest, then publish `FRAME_STAGED`; a successor waits for `ACTIVATE`, while the active frame may proceed immediately | | ||
| | 7 | chip_0 native-run path | after activation, prepare and launch the native run; poll it to completion and finalize it before another staged frame may launch. Compatibility endpoints perform the equivalent operation through blocking `ChipWorker::run` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the walkthrough consistent with successor preparation.
Line 731 states that preparation occurs after activation. This conflicts with Lines 385-403, which allow a capable backend to prepare a successor before ACTIVATE.
State that an active frame prepares after staging, while a capable successor can already be prepared but cannot launch until activation.
🤖 Prompt for 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.
In `@docs/task-flow.md` around lines 730 - 731, Update the chip_0 native-run path
walkthrough to distinguish active-frame and successor preparation: the active
frame prepares after staging, while a capable successor may be prepared before
ACTIVATE but must not launch until activation. Keep the existing launch,
polling, finalization, and compatibility endpoint behavior unchanged.
| completed = False | ||
| progress_error: BaseException | None = None | ||
| try: | ||
| if block and deadline is None: | ||
| self._chip_worker._wait_native_run(front.native_run) | ||
| completed = True | ||
| else: | ||
| completed = bool(self._chip_worker._poll_native_run(front.native_run)) | ||
| except BaseException as exc: # noqa: BLE001 | ||
| progress_error = exc | ||
| completed = True | ||
|
|
||
| if completed: | ||
| self._l2_finish_front_locked(front, progress_error) | ||
| # Launch the successor at the same ordered handoff boundary. | ||
| if self._l2_fifo: | ||
| successor = self._l2_runs[self._l2_fifo[0]] | ||
| if successor.phase == "deferred": | ||
| self._l2_prepare_front_locked(successor) | ||
| if successor.phase == "prepared": | ||
| self._l2_launch_front_locked(successor) | ||
| continue | ||
|
|
||
| if not block: | ||
| return False | ||
| if deadline is not None: | ||
| remaining = deadline - time.monotonic() | ||
| if remaining <= 0: | ||
| return False | ||
| time.sleep(min(remaining, _RUN_HANDLE_WAIT_RECHECK_S)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
_l2_progress_locked holds _l2_progress_cv across the blocking native wait.
Line 8295 calls _wait_native_run and line 8320 calls time.sleep while _l2_progress_cv is held. Every other lane entry point takes the same lock: _run_handle_done, _wait_run_handle, _wait_run_handle_accepted, and _submit_l2_locked. A RunHandle.done probe on run B therefore blocks for the full native execution of run A, and RunHandle.done is documented as a non-blocking read.
The straightforward fix is to replace time.sleep with self._l2_progress_cv.wait(...). That alone is not sufficient here: _l2_progress_cv wraps an RLock, and _submit_l2_locked enters _l2_progress_locked with the lock already held, so wait would release only one recursion level. Separating the FIFO-state lock from the progress-ownership lock, and dropping the state lock around the native wait, restores the non-blocking probe.
🤖 Prompt for 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.
In `@python/simpler/worker.py` around lines 8291 - 8320, Refactor
_l2_progress_locked so the FIFO state lock is separate from the
progress-ownership lock, and release the state lock before blocking in
_wait_native_run or sleeping during progress polling. Replace time.sleep with
_l2_progress_cv.wait using the remaining timeout, while preserving FIFO handoff
and completion handling. Ensure nested callers such as _submit_l2_locked do not
leave an RLock recursion level held across native waits, so RunHandle.done
remains non-blocking.
| return False | ||
| time.sleep(min(remaining, _RUN_HANDLE_WAIT_RECHECK_S)) | ||
|
|
||
| def _submit_l2_locked(self, callable, args, cfg: CallConfig) -> RunHandle: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the callable parameter or silence the rule.
Ruff reports A002: the argument shadows the callable builtin. Ruff flags this changed line, so the rule is enabled for this file.
🔧 Proposed fix
- def _submit_l2_locked(self, callable, args, cfg: CallConfig) -> RunHandle:
+ def _submit_l2_locked(self, callable_handle, args, cfg: CallConfig) -> RunHandle:
assert self._chip_worker is not None
- callable_state = self._resolve_handle(callable, expected_namespace="LOCAL_CHIP")
+ callable_state = self._resolve_handle(callable_handle, expected_namespace="LOCAL_CHIP")Update the RunHandle(self, run_id, (callable, args, cfg)) keepalive tuple at line 8375 to use the new name. If the surrounding submit / _submit_locked signatures must keep the callable name for API compatibility, add # noqa: A002 instead.
🧰 Tools
🪛 Ruff (0.16.0)
[error] 8322-8322: Function argument callable is shadowing a Python builtin
(A002)
🤖 Prompt for 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.
In `@python/simpler/worker.py` at line 8322, Rename the callable parameter in
_submit_l2_locked to avoid shadowing Python’s callable builtin, and update all
references within the method, including the RunHandle keepalive tuple. If
compatibility requires retaining the parameter name in surrounding submit or
_submit_locked methods, suppress A002 only on those signatures.
Source: Linters/SAST tools
| def _wait_run_handle_accepted(self, run_id: int) -> None: | ||
| if self.level == 2: | ||
| with self._l2_progress_cv: | ||
| while True: | ||
| state = self._l2_runs.get(run_id) | ||
| if state is None: | ||
| raise RuntimeError(f"unknown direct L2 run id {run_id}") | ||
| if state.phase != "prepared": | ||
| if state.error is not None: | ||
| raise state.error | ||
| return | ||
| self._l2_progress_locked(run_id, None, block=True) | ||
| assert self._orch is not None | ||
| self._orch._wait_run_accepted(run_id) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
_wait_run_handle_accepted returns early for a deferred L2 run.
The loop exits as soon as state.phase != "prepared". A run in phase "deferred" has not been prepared and has not been launched, so the method reports acceptance for a run that never crossed its launch fence. This is exactly the state produced by the new depth-one fallback path in _submit_l2_locked. Treat "deferred" as not-yet-accepted.
🐛 Proposed fix
def _wait_run_handle_accepted(self, run_id: int) -> None:
if self.level == 2:
with self._l2_progress_cv:
while True:
state = self._l2_runs.get(run_id)
if state is None:
raise RuntimeError(f"unknown direct L2 run id {run_id}")
- if state.phase != "prepared":
+ if state.phase not in ("deferred", "prepared"):
if state.error is not None:
raise state.error
return
self._l2_progress_locked(run_id, None, block=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _wait_run_handle_accepted(self, run_id: int) -> None: | |
| if self.level == 2: | |
| with self._l2_progress_cv: | |
| while True: | |
| state = self._l2_runs.get(run_id) | |
| if state is None: | |
| raise RuntimeError(f"unknown direct L2 run id {run_id}") | |
| if state.phase != "prepared": | |
| if state.error is not None: | |
| raise state.error | |
| return | |
| self._l2_progress_locked(run_id, None, block=True) | |
| assert self._orch is not None | |
| self._orch._wait_run_accepted(run_id) | |
| def _wait_run_handle_accepted(self, run_id: int) -> None: | |
| if self.level == 2: | |
| with self._l2_progress_cv: | |
| while True: | |
| state = self._l2_runs.get(run_id) | |
| if state is None: | |
| raise RuntimeError(f"unknown direct L2 run id {run_id}") | |
| if state.phase not in ("deferred", "prepared"): | |
| if state.error is not None: | |
| raise state.error | |
| return | |
| self._l2_progress_locked(run_id, None, block=True) | |
| assert self._orch is not None | |
| self._orch._wait_run_accepted(run_id) |
🤖 Prompt for 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.
In `@python/simpler/worker.py` around lines 8633 - 8646, Update
_wait_run_handle_accepted so L2 runs in the "deferred" phase continue waiting
instead of being treated as accepted; only exit once the run crosses the launch
fence into a non-prepared, non-deferred state, while preserving existing error
propagation.
| supports_concurrent_prepare = chip_worker.supports_concurrent_native_prepare | ||
| chip_worker._register_callable_at_slot(_SLOT, callable_obj) | ||
| private_slot_registered = True | ||
| public_handle = None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Call the capability method.
Line 136 stores a bound method. A bound method is truthy, so Line 201 enters the concurrent-preparation block even when the backend reports no support.
Call chip_worker.supports_concurrent_native_prepare() before the condition.
🤖 Prompt for 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.
In
`@tests/st/a2a3/host_build_graph/native_run_lifecycle/test_native_run_lifecycle.py`
around lines 136 - 139, Update the setup around supports_concurrent_prepare to
call chip_worker.supports_concurrent_native_prepare() and store its returned
capability value, rather than storing the bound method; keep the subsequent
concurrent-preparation condition using that boolean result.
| first_run = native_run | ||
| expected_stream_count = stream_count_before_prepare + int(supports_concurrent_prepare) | ||
| assert chip_worker.run_stream_set_create_count == expected_stream_count | ||
| assert chip_worker.run_stream_set_create_count == stream_count_before_prepare + 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skip run-stream count assertions on simulation.
CASES includes a2a3sim, but run_stream_set_create_count returns zero on simulation because it uses persistent bootstrap streams. Line 153 expects one new stream and fails on that platform.
Gate this assertion, and the equivalent concurrent-successor assertions, to a2a3.
🤖 Prompt for 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.
In
`@tests/st/a2a3/host_build_graph/native_run_lifecycle/test_native_run_lifecycle.py`
at line 153, Update the run-stream count assertions in the native lifecycle
tests, including the assertion near chip_worker.run_stream_set_create_count and
its concurrent-successor equivalents, so they execute only for the a2a3
platform. Skip these checks for a2a3sim while preserving all other lifecycle
assertions.
| assert not run_handle.done | ||
| assert run_handle.wait() is None | ||
| assert run_handle.done | ||
| chip_worker = w._chip_worker | ||
| assert chip_worker is not None | ||
| assert [event[0] for event in chip_worker.events] == [ | ||
| "prepare", | ||
| "launch", | ||
| "poll", | ||
| "wait", | ||
| "finalize", | ||
| ] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the worker in a finally block.
If run_handle.wait() or the event-order assertion fails, execution skips Line 815. The initialized worker then remains open and its resources are not released. Wrap the lifecycle assertions in try/finally and call w.close() from the finally block.
🤖 Prompt for 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.
In `@tests/ut/py/test_worker/test_startup_readiness.py` around lines 803 - 814,
Wrap the lifecycle assertions around run_handle and chip_worker events in a
try/finally block, and call w.close() in the finally clause so the initialized
worker is always released even when an assertion or wait fails.
70f978f to
4952338
Compare
Rebase the common active-plus-prepared ownership on current main without weakening the uniform pipeline ABI from hw-native-sys#1587 or the runner geometry, stream, and TLS contracts from hw-native-sys#1653. Add generation-bound direct L2 RunHandles, bounded two-slot admission, launch-only acceptance waiting, and deterministic depth-one fallback while preserving HBG inactive-bank preparation. Remove timing-dependent endpoint assertions and keep RequestSession absent.
4952338 to
999e70b
Compare
1 participant
Summary
This is W4 on current
main(b5261a7f), stacked on #1650 and #1588.run()and native wait behavior through per-run completion conditionsThe change adds no scheduling surface and keeps device execution strictly sequential. The execution task sends its final launch fallback notification before publishing completion; after publication it no longer dereferences run-owned state, preventing finalization from destroying state still used by the persistent executor.
Dependency
Merge order: #1650, then #1588, then this PR. GitHub displays the unmerged dependency commits until the earlier PRs merge.
Validation
1054 passed, 14 deselected.75/75 passed.task_20260803_050755_199972530808, all cases passed (7 L3, 6 L2 HBG, 3 L2 TMR); persistent execution thread creation stayed exactly1.b5261a7fwas conflict-free.999e70b73cf50bdffa7f9d5833d28e87523dc74d; its real-hardware test passed intask_20260803_064735_169883129670.No simulation run was used as acceptance evidence.