refactor(ci): extract NPU device-job setup into composite actions#1943
Conversation
Three NPU device jobs in ci.yml (system-tests, dist-system-tests, pypto-lib-model) and daily_ci.yml's qwen3-perf duplicated ~80 lines of identical setup and cleanup. Extract them into two reusable composite actions: - setup-npu-device-job: resilient submodule checkout (incremental, then purge + re-clone on the stale-object-store failure that persistent self-hosted runners hit after a force-pushed PR ref), ptoas + pto-isa install, ccache scoping, per-job conda venv + activate.sh, project build/install. Adds an explicit 'rm -rf venv' so a prior run's venv can never carry over. - kill-orphaned-tasks: the always() cleanup of detached task-submit tasks, matched by checkout dir. Each job replaces its inline setup with a sparse '.github/actions' fetch (uses: ./... resolves against $GITHUB_WORKSPACE, not the *-checkout subdir) plus a single composite call. Behavior preserved: dist passes unset-ring to drop PTO2_RING_* for HCCL; the others keep it. Also make the mechanism-A task-submit payload self-sufficient: _shell_quote_run now prepends 'source activate.sh' so the device child re-derives the CANN env itself instead of relying on task-submit's env snapshot, which can truncate under 32-worker submit concurrency and strand a batch with a missing-ASCEND_HOME_PATH ArtifactSetupError.
![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 introduces GitHub Actions for setting up self-hosted NPU device jobs and cleaning up orphaned task-submit tasks, alongside updating the test runner to source activate.sh for a self-sufficient execution environment. The review feedback highlights three key improvements: adding a guard in the cleanup action to prevent killing unrelated tasks if checkout-path is empty, making the chmod commands for ptoas resilient to missing files, and conditionally sourcing activate.sh in the test runner to preserve local development workflows.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df167ec117
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- _shell_quote_run: source activate.sh only when present
({ [ ! -f activate.sh ] || source activate.sh; }) so a developer running
--execute-via-task-submit without the CI bootstrap falls back to their
configured env instead of failing on a missing file; CI still sources it.
- kill-orphaned-tasks: skip cleanup when checkout-path is empty, so an empty
grep pattern can't match and kill other jobs' tasks on a shared workspace.
- setup-npu-device-job: guard the ptoas chmod calls on file existence so a
packaging variation can't fail the step.
|
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:
📝 WalkthroughWalkthroughThis PR introduces two new composite GitHub Actions— ChangesNPU CI setup/cleanup consolidation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant SetupAction as setup-npu-device-job
participant PtoIsaRepo as pto-isa repo
participant Venv
Workflow->>SetupAction: invoke with checkout-path, pto-isa-commit
SetupAction->>SetupAction: checkout with submodules (retry/clean on failure)
SetupAction->>SetupAction: npu-smi info check
SetupAction->>SetupAction: namespace ccache, relax permissions
SetupAction->>SetupAction: install ptoas from cache or download+verify SHA256
SetupAction->>PtoIsaRepo: clone and checkout pinned commit
SetupAction->>Venv: create conda+venv, write activate.sh
SetupAction->>Venv: install project + deps, verify imports
SetupAction-->>Workflow: ccache stats before/after
sequenceDiagram
participant Workflow
participant KillAction as kill-orphaned-tasks
participant TaskSubmit as task-submit
Workflow->>KillAction: invoke with checkout-path (on always())
KillAction->>TaskSubmit: list tasks matching workspace path
TaskSubmit-->>KillAction: task ids
KillAction->>TaskSubmit: kill each task_* id
KillAction-->>Workflow: complete (errors tolerated)
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/actions/setup-npu-device-job/action.yml (1)
75-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent externalization of cache path.
PTOAS_CACHE_DIRis hardcoded here whileCCACHE_DIR,PTOAS_ROOT, andPTO_ISA_ROOTare documented as caller-supplied job env vars (Line 6-8). For consistency, consider exposingPTOAS_CACHE_DIRthe same way.🤖 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 @.github/actions/setup-npu-device-job/action.yml around lines 75 - 98, The ptoas install step hardcodes PTOAS_CACHE_DIR inside the action instead of treating it like the other caller-supplied env vars. Update the setup-npu-device-job action so PTOAS_CACHE_DIR is declared and documented alongside CCACHE_DIR, PTOAS_ROOT, and PTO_ISA_ROOT, and have the "Install ptoas (local cache)" step read that env var rather than setting a fixed path locally.
🤖 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/actions/setup-npu-device-job/action.yml:
- Around line 67-73: The ccache permission step is overly broad because `chmod
-R a+rwX` makes the shared cache writable by any user on the host. Update the
`Relax shared ccache permissions` step to use a restricted trusted-group
approach instead, such as changing ownership/group and granting group write
access only, so only CI jobs can modify the cache. Keep the fix localized to
this action’s `run` command and preserve the existing best-effort behavior if
the cache is missing.
In @.github/workflows/ci.yml:
- Around line 226-234: Disable credential persistence on the sparse bootstrap
checkouts by updating the actions/checkout usages that only fetch
.github/actions at the workspace root. Add persist-credentials: false to each of
those sparse-checkout steps in the CI workflow, and leave the submodules: true
checkout inside setup-npu-device-job unchanged. Use the existing
actions/checkout@v4 sparse bootstrap steps as the targets for this change.
In @.github/workflows/daily_ci.yml:
- Around line 148-162: The bootstrap checkout used by the “Fetch composite
action definitions” step is still persisting credentials even though it only
needs the local .github/actions definitions; update the actions/checkout@v4
usage there to avoid storing auth and keep credentials only on the later
perf-checkout checkout if submodules require it. Use the “Fetch composite action
definitions” and “Setup NPU device job” steps as the anchors when making this
change.
In `@tests/st/harness/core/test_runner.py`:
- Around line 504-526: The generated activate.sh logic still depends on
GITHUB_WORKSPACE, which can break when the task-submit environment snapshot is
incomplete. Update the activate.sh generation in setup-npu-device-job/action.yml
so the venv path is derived from the script’s own location instead of the
workspace variable, and add fail-fast behavior in that script so a failed
activation aborts immediately. Use the activate.sh payload and the task-submit
command construction as the places to adjust.
---
Nitpick comments:
In @.github/actions/setup-npu-device-job/action.yml:
- Around line 75-98: The ptoas install step hardcodes PTOAS_CACHE_DIR inside the
action instead of treating it like the other caller-supplied env vars. Update
the setup-npu-device-job action so PTOAS_CACHE_DIR is declared and documented
alongside CCACHE_DIR, PTOAS_ROOT, and PTO_ISA_ROOT, and have the "Install ptoas
(local cache)" step read that env var rather than setting a fixed path locally.
🪄 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: 554a0a3b-63e3-4098-8d1f-2e26111ad7ee
📒 Files selected for processing (5)
.github/actions/kill-orphaned-tasks/action.yml.github/actions/setup-npu-device-job/action.yml.github/workflows/ci.yml.github/workflows/daily_ci.ymltests/st/harness/core/test_runner.py
- ci.yml / daily_ci.yml: set persist-credentials: false on the bootstrap sparse checkouts (they only fetch the public .github/actions tree and don't need the token persisted afterward — zizmor artipacked). - daily_ci.yml: scope qwen3-perf to permissions: contents: read. - setup-npu-device-job: derive activate.sh's venv path from BASH_SOURCE instead of $GITHUB_WORKSPACE, so a truncated task-submit env snapshot can't break venv activation in the device child (extends the same snapshot-independence this PR added for ASCEND_HOME_PATH).
2 participants
Summary
Deduplicates the self-hosted NPU device-job boilerplate and hardens per-run state hygiene, plus a supporting task-submit harness fix.
.github/actions/:setup-npu-device-job— resilient submodule checkout (incremental, then purge + re-clone on the stale-object-store failure persistent runners hit after a force-pushed PR ref), ptoas + pto-isa install, ccache scoping, per-job conda venv +activate.sh, project build/install. Adds an explicitrm -rf venvso a prior run's venv can never carry over.kill-orphaned-tasks— thealways()cleanup of detachedtask-submittasks, matched by checkout dir.ci.yml—system-tests,dist-system-tests,pypto-lib-modeleach replace ~80 inline lines with a sparse.github/actionsfetch + one composite call (913 → 603 lines).daily_ci.yml—qwen3-perfreuses the same composite actions (358 → 284 lines).tests/st/harness/core/test_runner.py—_shell_quote_runnow prependssource activate.shso the mechanism-A device child re-derives the CANN env itself instead of depending ontask-submit's env snapshot, which can truncate under 32-worker submit concurrency and strand a batch with a missing-ASCEND_HOME_PATHArtifactSetupError.Behavior is preserved:
distpassesunset-ring: 'true'to dropPTO2_RING_*for HCCL; the other jobs keep the ring env. TheFetch composite action definitionsstep usessparse-checkout: .github/actions+clean: falsebecause localuses: ./...resolves against$GITHUB_WORKSPACE, not the*-checkoutsubdir, and must not wipe sibling job workspaces.Testing
actionlintreports no new findings (only pre-existing self-hosted-label false positives)task-submit)Notes
The task-submit env-snapshot truncation is an infra-side race; sourcing
activate.shin the payload makes the pypto harness self-sufficient rather than depending on that snapshot, matching what the mechanism-B--runpayloads already do.