Repository navigation
test(onnx): fix magika idle-thread test on macOS with ORT 1.31 - #4194
rominparekh wants to merge 5 commits into
Conversation
…session The headroomlabs-ai#4187 regression test allowed a fixed 2 extra OS threads. ORT 1.31 registers Core ML as a built-in execution provider that claims the Apple NPU and GPU, and ort's default EP selection policy (MaxEfficiency, i.e. prefer NPU) then runs every session on Core ML. That starts 5-7 idle Apple Neural Engine threads per session, so the test failed on macOS although no intra-op pool exists. Build a control session with one intra-op thread first. It goes through the same EP selection, so the threads it adds become the allowance. The test still fails when the fix is reverted (macOS ORT 1.31: 12 vs 9, ORT 1.29: 7 vs 2; Linux ORT 1.31: 7 vs 1). Closes headroomlabs-ai#4191
Rust CI only ran on Ubuntu, so the macOS-only ORT 1.31 Core ML thread behaviour behind headroomlabs-ai#4191 went unnoticed. Add a macos-26 matrix entry that reuses the ONNX Runtime setup and runs cargo test; fmt and clippy stay Ubuntu-only and the test (ubuntu) check name is unchanged.
PR governanceThis PR follows the template and is marked ready for human review. |
| let before = settled_thread_count(); | ||
| let control = magika::Session::builder() | ||
| .with_intra_threads(1) | ||
| .build() | ||
| .expect("build pool-free control session"); | ||
| let allowance = settled_thread_count().saturating_sub(before); |
There was a problem hiding this comment.
🔍 Test-only fix conflicts with contribution policy
The contribution policy excludes test/CI-only PRs for known main failures. This change updates only the regression test and CI for #4191; maintainer approval of an exception remains necessary.
Was this helpful? React with 👍 or 👎 to provide feedback.
KennethWKZ
left a comment
There was a problem hiding this comment.
Thanks for this. Closed #4193 in favour of it. Three notes:
-
The new
test (macos)job never reached the magika test. It failed earlier onccr_backends::in_memory_get_refreshes_idle_ttl(an entry accessed within its idle window must stay alive, leftNone). That test touches the entry every 60 ms against a 120 ms idle TTL, and the macOS runner's sleep overshoot is enough to expire it. Cargo stops at the first failing test binary, so the log ends attests/ccr_backends.rsandmagika_idle_threadsnever runs (job log). Giving that test more slack on macOS, or runningcargo test --workspace --no-fail-faston the macOS leg, would let this PR's own check show up there. -
The allowance includes the control's persistent threads.
allowanceis everything the control adds while it's alive. The threads it leaves running afterdrop(control)(the 2–4 your comment mentions) are already in the secondbefore, though. If the magika session reuses those rather than starting its own, the comparison gets that many threads of extra headroom, which is part of why macOS has only 12 vs 9 to spare. Loggingsettled_thread_count()afterdrop(control)would show whether the allowance could be just the threads that go away with the session. -
The Core ML default. I'm happy to open the issue on the
magikacrate asking for a pass-through forwith_auto_device/ the EP selection policy on itsBuilder, so headroom can pin magika to the CPU if maintainers want that. Say the word, or tell me if you'd rather track it here first.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Hi @rominparekh - thanks for the PR - there is a test failing - can you help fix it? |
JerrettDavis
left a comment
There was a problem hiding this comment.
Reviewed head b245ce2, its inline feedback and documented local measurements. Current main is an ancestor, formatting passes, and the Ubuntu Rust test job succeeded. The control-session approach correctly distinguishes provider-created idle threads from a simple fixed allowance, with the documented low-core limitation retained.
The newly added macOS job is not yet qualified: job 115651049966 in run 38502033892 failed in ccr_backends::in_memory_get_refreshes_idle_ttl at line 173 (None versus Some("hot entry")). Cargo stopped before executing magika_idle_threads, so the hosted run does not yet verify the regression this PR fixes. This is an existing test outside the PR's code changes; I am not attributing its failure to the new allowance calculation.
Please ensure the macOS workflow executes the targeted Magika integration test even when another suite fails, and retain the full-suite check. The unrelated idle-TTL failure needs maintainer qualification or a justified test-fixture repair before this can be green for human merge review. No contributor CI action is requested. Coordinate with #4193's CPU-time alternative so we retain a single effective regression guard and avoid conflicting replacements of this test.
JerrettDavis
left a comment
There was a problem hiding this comment.
Reviewed updated head 345480f against the previous requested changes and discussion. The macOS workflow now runs the dedicated Magika integration test even if another full-suite binary fails, while retaining the full-suite check. The idle-TTL fixture uses wider timing margins and still proves both sliding retention beyond the original expiry and eventual expiry after inactivity; runtime storage logic is unchanged.
The previously missing hosted evidence is now present: both Ubuntu and macOS full Rust suites passed, and the dedicated macOS Magika test executed and passed without skipping. Its log reports start=2, control alive=4, control dropped=4, and zero extra Magika threads against allowance 2. The documented low-core limitation of a thread-count allowance remains; this is not a universal proof against every small spinning pool.
Current main is an ancestor, GitHub reports conflict-free, and current-head checks are terminal with 50 successful check runs plus a successful commit status, including Docker native E2E. Formatting and whitespace checks passed locally. Existing comments, the earlier CI failure and the test-only contribution-policy note were considered in this maintainer review.
Approved for final human merge review, replacing the earlier validation hold. No contributor runtime logic changed, contributor CI action requested, or merge performed.
|
Thanks for the review and approval @JerrettDavis . Can this PR be merged? |
Description
tests/magika_idle_threads.rs(added in #4189) fails on macOS with ONNX Runtime 1.31. Building the magika session leaves 8 extra threads against a fixed allowance of 2.The cause is not a thread pool. ORT 1.31 registers Core ML as a built-in EP that claims the Apple NPU/GPU (microsoft/onnxruntime#31975). The
ort2.0.0-rc.12 crate sets theMAX_EFFICIENCY(=PREFER_NPU) EP selection policy on everySessionBuilder::new(). So magika's session now runs on Core ML / Apple Neural Engine (ANE), and building it starts 5–7 ANE service threads that are idle, not spinning. The fixed allowance of 2 can't account for that.The test now builds a pool-free control session (
magika::Session::builder().with_intra_threads(1)) first and uses the threads it adds as the allowance. The control goes through the same EP selection, so whatever the EP starts cancels out, while a reintroduced intra-op pool still shows up on top.As a precaution, Rust CI now also runs
cargo testonmacos-26. It only ran on Ubuntu, which is why this slipped through.Closes #4191
Type of Change
Changes Made
crates/headroom-core/tests/magika_idle_threads.rs: measure the allowance against a pool-free control session instead of a fixed2, with comments explaining the ORT 1.31 Core ML/ANE cause. Test-only; no production code changes..github/workflows/rust.yml: turn thetestjob into a matrix withubuntu-latestandmacos-26, using the same ONNX Runtime setup step. Ubuntu keeps itstest (ubuntu)check name and runs the same steps as before. macOS runs onlycargo test;cargo fmtandcargo clippystay on Ubuntu.fail-fast: false, so one OS failing doesn't cancel the other.Testing
cargo fmt --all -- --check,cargo clippy --workspace -- -D warnings,cargo test --workspacewithORT_DYLIB_PATHset so the magika tests actually runpytest): only the pre-pushci-precheck-pythonsubset was run (177 passed, 4 skipped); the full suite was notruff check .): no Python files changed; pre-commit hooks passedmypy headroom): N/A, no Python changesTest Output
cargo test -p headroom-core --test magika_idle_threads, with the #4187 fix (with_intra_threads(1)) in place and with it temporarily reverted:Full Rust suite on macOS with ORT 1.31.0 (the same command the new
macos-26job runs):Pre-push
make ci-precheckon this branch:Real Behavior Proof
pip install onnxruntime==1.31.0and 1.29.0 dylibs viaORT_DYLIB_PATH; Linux via Dockerrust:1.95(aarch64) with pip onnxruntime 1.31.0.pip install 'onnxruntime==1.31.0',export ORT_DYLIB_PATH=<capi/libonnxruntime.*.dylib>,cargo test -p headroom-core --test magika_idle_threads), onmainand on this branch; then the same test with the fix(onnx): magika detector session still spins idle cores (#2540 covered only the Python factory) #4187with_intra_threads(1)fix temporarily reverted, on macOS 1.31, macOS 1.29 and Linux 1.31. To find the cause, a rawortprobe built the magika model withintra_threads=1under the default policy and underwith_auto_device(PreferCPU), counting threads withps -Mand listing the EP devices.mainwith ORT 1.31 (8 extra vs 2) and passes on this branch, including the reporter's exact steps. With the fix(onnx): magika detector session still spins idle cores (#2540 covered only the Python factory) #4187 fix reverted it fails on all three setups (12 vs 9, 7 vs 2, 7 vs 1), so it still guards fix(onnx): magika detector session still spins idle cores (#2540 covered only the Python factory) #4187. Probe: EP devices are CPU, CoreML/NPU and CoreML/GPU. The default policy adds 6–7 threads and takes ~420 ms to build;PreferCPUadds 0 and takes ~12 ms.macos-26CI job on GitHub runners (it runs on this PR), ORT 1.30.0 at runtime, x86_64 Linux, Intel macOS, and low-core macOS hosts. Details:Runtime Rollout Safety
Review Readiness
Checklist
CHANGELOG.md— it is generated by release-please from my Conventional Commit PR title (a CI guard enforces this)Screenshots (if applicable)
N/A
Additional Notes
ort's default policy; nobody chose it. Visible cost: a one-off ~400 ms session build plus 5–7 idle threads. Per-call latency and output parity were not measured. Pinning it to CPU needswith_auto_device(PreferCPU), which magika 1.1.0'sBuilderdoesn't expose. An environment-level CPU EP does not override the policy. This is raised on test(onnx): magika_idle_threads fails on macOS with ORT 1.31 (Neural Engine threads, not an intra-op pool) #4191 as a possible follow-up issue on themagikacrate.