Repository navigation
Test: give the native-overlap proof a pipeline-level negative control - #1857
Conversation
Fixes hw-native-sys#1855 `assert_native_overlap` had a negative control at analyzer level only: tests/ut/py/test_strace_timing.py proves the analyzer rejects a bad log, but from hand-built span records. Nothing showed that a pipeline which is not staging a successor produces a log the analyzer rejects. Between the pipeline and the verdict sits a chain — which spans are emitted, where their endpoints land, `bind` standing in for preparation, `runner_run` being a host wall span that includes caller polling — and if any link made the two intervals intersect regardless of real concurrency, 39/39 adjacent pairs would still be green. Worse, the disabling condition was the skip condition: `pipeline_depth < 2` is "this lane cannot stage a successor", so the one state in which the property must fail was the one where the test declined to run, and a skip reports green. The three submission loops now share one driver, so each negative arm differs from the positive one in exactly one variable: - inflight_limit=1 — capability and config unchanged, the submissions simply never coexist. Requires NativeOverlapError. Matching the message separates a real rejection from the vacuous "need at least two complete native runs" one. - a diagnostics config — `allow_prepared_successor` folds in `CallConfig::diagnostics_any()`, because a collector's setup mutates runner-global state that is not yet per-epoch. The lane declines to stage rather than raising, so submissions still succeed and goldens still pass; nothing else would notice. `enable_scope_stats` is the lightest of the five flags and which one is set does not matter. The depth check becomes an assert on `supports_concurrent_native_prepare`, which folds depth, the capability symbol, and `initialized_`. Neither input is reachable from a CallConfig — every onboard runtime declares depth 2 and returns 1 from the capability impl — so a false there is a regression, not a differently configured box. The platform gate still runs first, since the sim platform hardcodes the capability to 0. The arms also move off callable id 0. The L2 st_worker is session-scoped and the framework's own default test_run holds id 0 for the session; the original test avoided the collision only by sorting alphabetically before test_run, which two new arms would not. Verified on a2a3 silicon: the full tests/st/a2a3/host_build_graph sweep is green before (33 passed) and after (35 passed, twice). Inverting both new arms — giving the serial arm inflight_limit=2 and dropping the diagnostic flag — makes both report DID NOT RAISE, which is what shows they are wired to the variable they claim to move rather than passing incidentally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe async pipeline stress test now uses shared helpers for pipeline execution and native-overlap checks. It adds serial-submission and diagnostics controls that require ChangesNative overlap controls
Merge Risk: ⚪ Minimal · up to This change strengthens native-overlap negative coverage and documents the behavior without changing production code or runtime behavior; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Test
participant AsyncPipeline
participant NativeOverlapAnalyzer
Test->>AsyncPipeline: submit overlapping or control runs
AsyncPipeline-->>Test: return trace spans and retired results
Test->>NativeOverlapAnalyzer: assert_native_overlap(trace spans)
NativeOverlapAnalyzer-->>Test: accept overlap or raise NativeOverlapError
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 |
Four corrections left behind by #1845 and #1857. No behavior changes. #1857's comment on _ARM_CALLABLE_ID got the causality wrong while reaching the right conclusion. It called the L2 st_worker session-scoped; the sibling conftest overrides it to scope="class" precisely so this white-box stress gets a worker whose slot table starts empty. The collision it describes is therefore inside the class, not across the session: the framework's inherited test_run registers through Worker.register, which takes the lowest free slot (0) and caches the handle on the test class without unregistering, so id 0 stays occupied for the remaining tests. A file whose subject is what the arms prove should not misstate why they are wired the way they are. #1857's diagnostics arm asserts a fallback the project intends to remove. concurrent_native_prepare_supported_impl keeps collector-bearing configurations sequential only until their state is per-epoch; when that lands and diagnostics_any() leaves allow_prepared_successor, the arm fails with the same "did not overlap" it currently requires, and the correct response is to delete it rather than restore the serialization. Nothing said so. The two negative arms differ in durability — one run in flight cannot overlap under any admission policy, so the serial arm is permanent — and that difference decides what to do when either goes red, so state it in both the test and docs/dfx/host-trace.md. #1845 left set_host_log_state with no declaration anywhere: the sim AICPU backend defines it and both sim device_runner.cpp files resolve it by name, while its sibling set_log_level, resolved by the same load_sym call two lines above, is declared in aicpu/device_log.h. Declare it there too, with the state struct only forward-declared so neither <dlfcn.h> nor the state layout reaches device targets. The dlsym caller stays string-resolved, as set_log_level's does. #1845's KeepsExistingInitArgumentOrder asserted that a function's address is non-null, which is a tautology; the real guard was that its helper compiled, and gcc can warn on the comparison. Express the intent directly as a static_assert on ChipWorker::init's type, which fails with a named diagnostic naming the rule it protects. Verified by reordering two parameters and confirming the build stops. Testing: ctest -LE requires_hardware --timeout 300 — 100/100 pass. Both aicpu/device_log.cpp backends syntax-check against the new declaration. pre-commit clean except clang-tidy, whose hook venv cannot import simpler and fails identically on untouched files.
Fixes #1855
Problem
assert_native_overlaphad a negative control at analyzer level only.tests/ut/py/test_strace_timing.pyproves the analyzer rejects a bad log — but from hand-built span records. Nothing demonstrated that a pipeline which is not staging a successor produces a log the analyzer rejects.Between the pipeline and the verdict sits a chain: which spans are emitted, where their endpoints land,
bindstanding in for preparation, andrunner_runbeing a host wall span that includes caller polling. If any link made the two intervals intersect regardless of real concurrency, 39/39 adjacent pairs would still be green and nothing in the suite would notice.Second, the disabling condition was the skip condition.
pipeline_depth < 2is "this lane cannot stage a successor", so the one state in which the property must fail was the one where the test declined to run — and a skip reports green.Change
The three submission loops now share one
_drive_pipeline, so each negative arm differs from the positive one in exactly one variable:inflight_limit=1did not overlapenable_scope_statssetdid not overlapMatching the message is load-bearing: it separates a real rejection from the vacuous
need at least two complete native runsone.The diagnostics arm covers a fallback that is otherwise silent.
allow_prepared_successorfolds inCallConfig::diagnostics_any()because a collector's setup mutates runner-global state that is not yet per-epoch. The lane's check declines to stage rather than raising, so submissions still succeed and goldens still pass — nothing else would notice. (The rawprepare_native_runadmission error for a diagnostic successor was already covered bynative_run_lifecycle; the lane's quiet fallback, and the analyzer's rejection of the resulting log, were not.)The depth skip becomes an assert on
supports_concurrent_native_prepare, which folds depth, the capability symbol, andinitialized_. Neither input is reachable from aCallConfig— every onboard runtime declares depth 2 (PTO_PIPELINE_MAX_DEPTHis 2) and returns 1 from the capability impl — so a false there is a regression, not a differently configured box. The platform gate still runs first, since the sim platform hardcodes the capability to 0.The arms also move off callable id 0: the L2
st_workeris session-scoped and the framework's own defaulttest_runholds id 0 for the session. The original test avoided that collision only by sorting alphabetically beforetest_run, which two new arms did not — it surfaced asregister_callable failed with code -1.Per the issue's "consider making the skip loud", this stays in tests and docs. The silent serialization becomes asserted, not announced — no
src/change.Verification
On a2a3 silicon, via
task-submit:tests/st/a2a3/host_build_graphsweep green before (33 passed, rc=0) and after (35 passed, rc=0, reproduced twice). The delta is exactly the two new arms.tests/ut/py/test_strace_timing.py: 26 passed (analyzer untouched).inflight_limit=2and dropping the diagnostic flag makes both arms reportDID NOT RAISE. That is what shows they are wired to the variable they claim to move rather than passing incidentally, and it independently confirms a diagnostic flag really does serialize the lane.Note for reviewers
One sweep during development reported two intermittent
worker_async_fifoL3 child failures. It did not reproduce in three later sweeps (including one on a pristine tree), the file passes in isolation, and the harness that caught it truncated the child output. Recorded locally rather than claimed as either a regression or a known flake.🤖 Generated with Claude Code