Test: prove the sim runner publishes launch acceptance - #1649
Conversation
📝 WalkthroughWalkthroughThe change adds weak pipeline-contract providers, exposes simulated accepted-state publication, requires runtime symbols and valid contracts in ChangesPipeline Contract and Launch Acceptance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChipWorker
participant set_task_accepted_state_ctx
participant SimDeviceRunnerBase
participant MailboxFrames
ChipWorker->>set_task_accepted_state_ctx: bind accepted-state pointer at launch
set_task_accepted_state_ctx->>SimDeviceRunnerBase: publish accepted state
SimDeviceRunnerBase->>MailboxFrames: publish sticky _TASK_ACCEPTED word
MailboxFrames-->>ChipWorker: expose launch-acceptance frame
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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 `@tests/st/a2a3/tensormap_and_ringbuffer/test_l3_launch_acceptance.py`:
- Around line 50-91: Import ClassVar and annotate the scene-test metadata fields
CALLABLE and CASES as ClassVar with their existing value types, preserving their
contents and class-level behavior.
🪄 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: 77d4ba1d-bb76-42cd-a3fb-914c8096c599
📒 Files selected for processing (5)
src/common/platform/onboard/host/c_api_shared.cppsrc/common/platform/sim/host/c_api_shared.cppsrc/common/worker/chip_worker.cppsrc/common/worker/pto_runtime_c_api.htests/st/a2a3/tensormap_and_ringbuffer/test_l3_launch_acceptance.py
A ChipTask's sticky acceptance word is written by the platform runner once the run crosses its launch boundary, through the set_task_accepted_state_ctx binding ChipWorker resolves at init. Nothing asserted that it arrives: the two endpoint scene tests that read the word assert it is still 0 before activation, and both are a2a3-onboard-only, so the sim side of the binding had no coverage at all. That gap hid a real defect until hw-native-sys#1587. The sim c_api exported no set_task_accepted_state_ctx, so ChipWorker's then-optional load produced nullptr, both bind sites were skipped, and SimDeviceRunnerBase's publish_task_accepted stored through a null pointer target. A sim child therefore never published acceptance, and the run-level fence (decrement_run_accepts, reached via LocalMailboxEndpoint::read_task_accepted) advanced only when the run reached a terminal phase — the launch fence silently degraded into a completion fence. The test dispatches one ChipTask and asserts the word is set in whichever mailbox frame carried it, which holds on both endpoint shapes: the parent clears the word only when it publishes the next task into that frame. Verified to fail against the pre-hw-native-sys#1587 sim c_api with "the chip worker never published launch acceptance: [0, 0, 0]".
e7ddbab to
6cf7efe
Compare
ChaoWao
changed the title
|
Narrowed after #1587 landed. #1587 merged as One approach difference, resolved in main's favour: for What is left is the test, which main has no equivalent of — the two endpoint tests that read the acceptance word assert it is still |
1 participant
Summary
Adds the missing regression barrier for launch-acceptance publication on sim.
This PR started as the strict-symbol-loading refactor and has been narrowed to
the test. #1587 merged as
2a650f2dwhile this was open and landed the wholerefactor independently — all four symbols strict,
load_optional_symboldeleted, the sim
set_task_accepted_state_ctxexport, the dead nullptr guardsremoved, and the header docs updated. I verified main against my diff
line-by-line and it is equivalent throughout, so I dropped my
src/changeswholesale on rebase rather than churn a mechanism that had already shipped.
The one difference in approach: for
get_pipeline_contract— absent from a5'sruntime makers — I had put a weak depth-1 default in both platform
c_api_shared.cpp, matchingprewarm_config_impl; main instead addedstrong definitions to a5's two
runtime_maker.cpp. Both give a5 thedepth-1 contract, main's is explicit at the cost of the literal appearing in
two more places, and I checked a5's
host_build_graphdefinition sits insideextern "C" {so its linkage is right. Main's version stands.What is left, and why it is worth landing
Nothing asserted the accepted word ever arrives. The two endpoint scene
tests that read it (
worker_async_endpoint,worker_async_fifo) assert it isstill
0before activation — the negative direction — and both area2a3-onboard-only, so the sim side of the binding had no coverage at all.
That gap hid a real defect until #1587. The sim c_api exported no
set_task_accepted_state_ctx, so ChipWorker's then-optional load producednullptr, both bind sites were skipped, andSimDeviceRunnerBase::publish_task_acceptedfound a null target. A sim child therefore never published acceptance, and the
run-level fence —
LocalMailboxEndpoint::read_task_accepted→on_accept→Orchestrator::mark_task_accepted→decrement_run_accepts— advanced onlyonce the run reached a terminal phase, because
acceptance_ready()wassatisfied by its other disjunct. The launch fence silently degraded into a
completion fence on every sim run.
worker.pypasses the accepted-word address on every platform (single-framerun_from_bloband two-frame_launch_native_run, neither platform-gated), andthe sim runner does reach
publish_task_accepted()at its launch boundary — sothis was never "sim doesn't use the accepted word", only an unexported symbol.
The test dispatches one ChipTask and asserts the word is set in whichever
mailbox frame carried it. That holds on both endpoint shapes because the parent
clears the word only when it publishes the next task into that frame, so it
survives completion.
Verified to actually catch the regression: against the pre-#1587 sim c_api
it fails with
Testing
At the rebased base (
2a650f2d):task-submit(task_20260803_010730_25475769728, 1 device) — this test: passed on real siliconFrom the pre-rebase run of the full refactor (superseded by #1587, but the test
was present throughout): cpput 74/74 · pyut 1034 passed / 9 skipped · a2a3sim
sweep 63 PASS 0 FAIL · a5sim sweep 47 PASS 0 FAIL · a2a3 onboard sweep 62 PASS
0 FAIL before the run hit the 600 s
--pto-session-timeoutbudget in its lastgroup on this shared box.