Fix: pass pin-resolved PTO_ISA_ROOT by value and -D - #1418
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR replaces ambient ChangesPTO-ISA pin validation and runtime wiring
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors how the PTO_ISA_ROOT path is propagated and validated across the build and test systems. Instead of relying on the ambient environment variable os.environ["PTO_ISA_ROOT"] (or $ENV{PTO_ISA_ROOT} in CMake), the pin-resolved path is now explicitly passed to CMake as a definition (-DPTO_ISA_ROOT=) and to compilers directly. Additionally, it introduces strict validation to ensure the resolved PTO_ISA_ROOT matches the current pto_isa.pin commit, raises errors for missing build metadata, and incorporates the pin SHA into session compile-cache keys to prevent stale kernel reuse during mid-session pin bumps. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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
🧹 Nitpick comments (1)
tests/ut/py/test_pto_isa.py (1)
360-365: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape the regex metacharacter in the
matchstring.
pytest.raisestreats thematchstring as a regular expression. The unescaped.inpto_isa.pinwill match any character. Escaping it ensures an exact match and resolves the static analysis warning.♻️ Proposed fix
- with pytest.raises(RuntimeError, match="does not match pto_isa.pin"): + with pytest.raises(RuntimeError, match=r"does not match pto_isa\.pin"):🤖 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_pto_isa.py` around lines 360 - 365, Update the pytest.raises match pattern in test_assert_pto_isa_root_matches_pin_rejects_mismatch to escape the dot in “pto_isa.pin”, ensuring the assertion matches the literal error text rather than any character.Source: Linters/SAST tools
🤖 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 @.claude/skills/multi-repo-qwen-setup/SKILL.md:
- Around line 267-270: Remove the PTO_ISA_ROOT export from the simpler
managed-checkout instructions. Update the surrounding guidance to rely on
ensure_pto_isa_root() and automatic pin resolution, or document the external
script’s explicit supported argument/configuration instead; do not teach users
to set an ambient root that can override the pinned checkout.
---
Nitpick comments:
In `@tests/ut/py/test_pto_isa.py`:
- Around line 360-365: Update the pytest.raises match pattern in
test_assert_pto_isa_root_matches_pin_rejects_mismatch to escape the dot in
“pto_isa.pin”, ensuring the assertion matches the literal error text rather than
any character.
🪄 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
Run ID: 476eae82-7bde-412b-a4be-66220bf39d6a
📒 Files selected for processing (18)
.claude/skills/multi-repo-qwen-setup/SKILL.md.claude/skills/review-pr/SKILL.mdconftest.pydocs/a5-sdma-overlay.mddocs/getting-started.mdsimpler_setup/build_runtimes.pysimpler_setup/kernel_compiler.pysimpler_setup/pto_isa.pysimpler_setup/runtime_builder.pysimpler_setup/runtime_compiler.pysimpler_setup/scene_test.pysimpler_setup/tools/l0_swimlane.pysrc/a2a3/platform/onboard/host/CMakeLists.txtsrc/a5/platform/onboard/host/CMakeLists.txttests/st/runtime_fatal_codes/test_runtime_fatal_codes.pytests/ut/py/test_pto_isa.pytests/ut/py/test_runtime_builder.pytests/ut/py/test_scene_test_cache.py
9fb25ac to
8b19b0f
Compare
Stop smuggling the managed pto-isa checkout through os.environ /
$ENV{PTO_ISA_ROOT}. ensure_pto_isa_root() remains the pin source of
truth; RuntimeBuilder passes -DPTO_ISA_ROOT= to host CMake (a2a3 always,
a5 when an async overlay is ON).
- Drop ambient env writes in runtime_compiler, build_runtimes, conftest,
scene_test, and runtime_fatal_codes
- a2a3/a5 host CMakeLists require -DPTO_ISA_ROOT= instead of $ENV
- l0_swimlane resolves the pin once and threads it by value into
workspace generation (run_collect.sh) and smoke_build (-DPTO_ISA_ROOT=);
it no longer injects PTO_ISA_ROOT into the subprocess environment
- Scene-test compile cache keys include the pin SHA
- Embedding platforms fail closed when pto_isa_build.json is missing
- KernelCompiler rejects pto_isa_root that does not match pto_isa.pin
- docs/skills: fail-closed metadata, SIMPLER_PTO_ISA_BUILD_COMMIT naming,
drop ambient-export guidance (incl. the qwen decode_fwd path); MD060
table align
- docs/a5-sdma-overlay.md: libopapi probe, 2026-07-21 9.1.T500 repro,
no Option B on broken CANN; hw-native-sys#1351 marked done
Fixes hw-native-sys#1403
Related to hw-native-sys#1315
2 participants
Summary
os.environ["PTO_ISA_ROOT"]/$ENV{PTO_ISA_ROOT}.ensure_pto_isa_root()remains the pin source of truth; host CMake gets-DPTO_ISA_ROOT=(a2a3 always; a5 when an async overlay is ON).runtime_compiler,build_runtimes,conftest,scene_test,runtime_fatal_codes, and bake the path inl0_swimlane.docs/a5-sdma-overlay.mdwith the 2026-07-21 CANN 9.1.T500 repro (libopapi.soprobe,507018/0x715002a), mark [Code Health] Extend PTO-ISA build/run version guard to a5 onboard SDMA overlay (missed after #1179) #1351 done, and keep SDMA default OFF (no Option B on broken CANN).Fixes #1403
Related to #1315