refactor(build): drop SIMPLER_PTO_ISA_COMMIT override channels#1218
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:
📝 WalkthroughWalkthroughRemoves ChangesRemove SIMPLER_PTO_ISA_COMMIT, rely on pto_isa.pin
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related issues
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 removes the SIMPLER_PTO_ISA_COMMIT environment and CMake variables, simplifying the resolution order for the pto-isa repository checkout and updating the documentation and tests accordingly. The review feedback highlights opportunities to clean up the unit tests by removing unused monkeypatch fixtures and mocks, as well as deleting a redundant test case that is already covered by a parametrized test.
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/getting-started.md (1)
188-188: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove stale
PTO_ISA_COMMITreference.The parenthetical "(CI
PTO_ISA_COMMIT)" is outdated—CI no longer uses that variable since this PR removesSIMPLER_PTO_ISA_COMMITwiring from workflows. The trigger is now thepto_isa.pinfile itself. Consider rephrasing to "Pinned pto-isa commit changes" or similar.Also, "set
PTO_ISA_ROOTat a pre-checked-out clone" should be "setPTO_ISA_ROOTto a pre-checked-out clone".🤖 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 `@docs/getting-started.md` at line 188, The getting-started docs still refer to the obsolete CI `PTO_ISA_COMMIT` variable, so update the wording around the `pto_isa.pin` change trigger to remove that reference and describe it as pinned pto-isa commit changes instead; also fix the `PTO_ISA_ROOT` guidance in the same section to say it should be set to a pre-checked-out clone, using the existing documentation text near the ASCEND_HOME_PATH example as the location cue.
🤖 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 @.github/workflows/ci.yml:
- Around line 232-247: Add --require-pto-isa to each DFX smoke pytest invocation
in the CI workflow so these jobs fail immediately when PTO-ISA cannot be
resolved or cloned; update the affected smoke steps (for example the
l2_swimlane, PMU, and args_dump commands) to pass this flag alongside the
existing pytest options, matching the behavior used by the main scene-test jobs.
In `@docs/developer-guide.md`:
- Line 188: Update the pto-isa rebuild guidance in the developer guide to remove
the stale CI environment variable reference in the “Pinned pto-isa commit” note,
since the source of truth is now the pto_isa.pin default rather than a CI
override; also reword the PTO_ISA_ROOT guidance in that same entry to use “to”
or “point ... to” instead of “at” for the pre-checked-out clone. Keep the change
localized to the table row that mentions pto_isa.pin, PTO_ISA_ROOT, and
build_runtimes.py.
---
Outside diff comments:
In `@docs/getting-started.md`:
- Line 188: The getting-started docs still refer to the obsolete CI
`PTO_ISA_COMMIT` variable, so update the wording around the `pto_isa.pin` change
trigger to remove that reference and describe it as pinned pto-isa commit
changes instead; also fix the `PTO_ISA_ROOT` guidance in the same section to say
it should be set to a pre-checked-out clone, using the existing documentation
text near the ASCEND_HOME_PATH example as the location cue.
🪄 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: e849e8b9-4aba-4c93-9c6a-263445470ed8
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/sanitizers.ymlCMakeLists.txtdocs/developer-guide.mddocs/getting-started.mdsimpler_setup/pto_isa.pytests/ut/py/test_pto_isa.py
💤 Files with no reviewable changes (1)
- CMakeLists.txt
yanghaoran29
force-pushed
the
down_spic
branch
2 times, most recently
from
9cfe82e to
f12a51f
Compare
Follow up hw-native-sys#1196: now that managed pto-isa defaults to pto_isa.pin, the CMake define and Python env override channel (SIMPLER_PTO_ISA_COMMIT) is redundant with the pin-as-single-source-of-truth model. Drop only that channel; keep the CI's PTO_ISA_COMMIT pinning (read-pto-isa exports pto_isa.pin as PTO_ISA_COMMIT, which the scene/UT steps still pass via --pto-isa-commit to keep host_runtime.so and test kernels on one revision, issue hw-native-sys#1067). - pto_isa.py: resolve_pto_isa_commit no longer falls back to the SIMPLER_PTO_ISA_COMMIT env var; CLI > pin only. - CMakeLists.txt: drop the SIMPLER_PTO_ISA_COMMIT cache variable and the --pto-isa-commit forwarding to build_runtimes.py. - ci.yml: the four onboard install steps lose the cmake.define.SIMPLER_PTO_ISA_COMMIT override and fall through to the pin default (== PTO_ISA_COMMIT); the --pto-isa-commit pytest passes are retained. - getting-started.md / developer-guide.md: document PTO_ISA_ROOT and manual build_runtimes.py --pto-isa-commit as the install-time override path (the CMake define is gone). - test_pto_isa.py: rewrite env-priority cases to CLI + pin. Install/run override still available via --pto-isa-commit CLI (run-time), PTO_ISA_ROOT, or manual build_runtimes.py --pto-isa-commit (install-time).
2 participants
Follow up #1196: now that managed pto-isa defaults to
pto_isa.pin, the CMake define and Python env override channelSIMPLER_PTO_ISA_COMMITis redundant with the pin-as-single-source-of-truth model. This PR drops only that channel.Scope clarification: the CI's
PTO_ISA_COMMITmechanism is kept. Theread-pto-isaaction still readspto_isa.pinand exports it asPTO_ISA_COMMIT, and the scene/UT steps still pass--pto-isa-commit ${{ env.PTO_ISA_COMMIT }}sohost_runtime.soand the test-time kernels stay on one pto-isa revision (issue #1067). Only theSIMPLER_PTO_ISA_COMMIThalf of the resolution tier is removed.pto_isa.py:resolve_pto_isa_commitno longer falls back to theSIMPLER_PTO_ISA_COMMITenv var; CLI > pin only.CMakeLists.txt: drop theSIMPLER_PTO_ISA_COMMITcache variable and the--pto-isa-commitforwarding tobuild_runtimes.py.ci.yml: the four onboard install steps lose thecmake.define.SIMPLER_PTO_ISA_COMMIToverride and fall through to the pin default (whichread-pto-isaexports asPTO_ISA_COMMIT); the--pto-isa-commitpytest passes are retained.getting-started.md/developer-guide.md: documentPTO_ISA_ROOTand manualbuild_runtimes.py --pto-isa-commitas the install-time override path (the CMake define is gone).test_pto_isa.py: rewrite env-priority cases to CLI + pin.Install/run override still available via
--pto-isa-commitCLI (run-time),PTO_ISA_ROOT, or manualbuild_runtimes.py --pto-isa-commit(install-time).