Skip to content

Fix: pass pin-resolved PTO_ISA_ROOT by value / -D - #1416

Closed
yanghaoran29 wants to merge 1 commit into
hw-native-sys:mainfrom
yanghaoran29:fix/issue-1403-pto-isa-root-by-value
Closed

yanghaoran29 wants to merge 1 commit into
hw-native-sys:mainfrom
yanghaoran29:fix/issue-1403-pto-isa-root-by-value

Conversation

@yanghaoran29

Copy link
Copy Markdown
Contributor

Summary

  • Stop smuggling the managed pto-isa checkout through 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).
  • Drop ambient env exports in runtime_compiler, build_runtimes, conftest, scene_test, runtime_fatal_codes, and bake the path in l0_swimlane.
  • Update docs/a5-sdma-overlay.md with the 2026-07-21 CANN 9.1.T500 repro (libopapi.so probe, 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).

Test plan

  • pytest tests/ut/py/test_runtime_builder.py tests/ut/py/test_pto_isa.py (71 passed)
  • CI green on this PR
  • Spot-check: host cmake configure with overlay ON shows -DPTO_ISA_ROOT= and no reliance on ambient PTO_ISA_ROOT

Fixes #1403
Related to #1315

@coderabbitai

coderabbitai Bot commented Jul 21, 2026 •

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 498c8cdd-f423-4f5f-9aef-979adb181f70

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change removes PTO-ISA environment-variable transport, stores resolved paths in runtime state or metadata, passes paths to host CMake with -DPTO_ISA_ROOT, updates generated collection workflows, and revises related tests and a5 overlay documentation.

Changes

PTO-ISA path transport

Layer / File(s) Summary
Runtime PTO-ISA resolution
simpler_setup/runtime_compiler.py, simpler_setup/build_runtimes.py, conftest.py, simpler_setup/scene_test.py
Pinned PTO-ISA paths are retained in runtime state or metadata, while eager initialization no longer exports PTO_ISA_ROOT.
Host CMake path contract
simpler_setup/runtime_builder.py, src/a2a3/.../CMakeLists.txt, src/a5/.../CMakeLists.txt, tests/ut/py/test_runtime_builder.py, tests/st/runtime_fatal_codes/test_runtime_fatal_codes.py
Host builds pass PTO_ISA_ROOT through CMake defines, and both host CMake files consume the CMake variable instead of the environment. Tests verify the define and remove test-side environment export.
Collection tooling and overlay procedure
simpler_setup/tools/l0_swimlane.py, docs/a5-sdma-overlay.md
Generated collection scripts embed the resolved path, standalone task environments stop exporting it, and the a5 overlay documentation describes the revised gating and re-enable workflow.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

Possibly related PRs

Sequence Diagram(s)

sequenceDiagram
  participant RuntimeCompiler
  participant RuntimeBuilder
  participant HostCMake
  participant PTOISAHeaders
  RuntimeCompiler->>RuntimeBuilder: provide resolved PTO-ISA path
  RuntimeBuilder->>HostCMake: pass -DPTO_ISA_ROOT=path
  HostCMake->>PTOISAHeaders: use path/include
  HostCMake-->>RuntimeBuilder: configure host build
Loading

Poem

A rabbit hops where env vars flew,
With pinned paths passed cleanly through.
CMake finds the headers bright,
Tests guard the build just right.
SDMA notes now show the way—
No ambient roots to hide or stray.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.94% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: passing the pin-resolved PTO_ISA_ROOT by value instead of through the environment.
Description check ✅ Passed The description is clearly aligned with the changeset and describes the env-to-argument refactor, docs updates, and test plan.
Linked Issues check ✅ Passed The changes match #1403 by removing PTO_ISA_ROOT env transport, passing the resolved path explicitly to Python and CMake, and updating docs/tests.
Out of Scope Changes check ✅ Passed No clearly unrelated code changes stand out; the extra doc detail and test updates still support the PTO_ISA_ROOT transport refactor and related overlay work.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors how the PTO_ISA_ROOT path is propagated to CMake and downstream builds. Instead of exporting it as an ambient environment variable (os.environ["PTO_ISA_ROOT"]), it is now passed explicitly as a CMake define (-DPTO_ISA_ROOT=). This change is implemented across several setup scripts, test configurations, CMake files, and documentation. There are no review comments to assess, and I have no feedback to provide.

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.

@yanghaoran29
yanghaoran29 force-pushed the fix/issue-1403-pto-isa-root-by-value branch 5 times, most recently from 53b5ad2 to 98e84c6 Compare July 21, 2026 03:45
@yanghaoran29
yanghaoran29 force-pushed the fix/issue-1403-pto-isa-root-by-value branch from 98e84c6 to 81e1aad Compare July 21, 2026 06:48
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 bakes the path into run_collect.sh / CACHE PATH
- 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,
  no ambient-export guidance for simpler itself; 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code Health] Stop transporting PTO_ISA_ROOT via the environment; pass the pin-resolved path by value / -D

1 participant