Add CPU emergency lane (ci-self-cpu) with /run-cpu admin button - #1598
Conversation
📝 WalkthroughWalkthroughCI now supports manual diff inputs, centralized SHA selection, and runner-aware simulation jobs. Linux simulations can use self-hosted CPU runners while macOS remains hosted. Scene and DFX tests use the selected Python environment, with documentation updated accordingly. ChangesCI routing and simulation execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant diff-refs
participant SimulationRunner
participant Python
participant Pytest
Workflow->>diff-refs: resolve base_sha and head_sha
diff-refs->>SimulationRunner: provide workflow diff context
SimulationRunner->>Python: configure runner-specific environment
Python->>Pytest: select $PYTHON
Pytest->>SimulationRunner: run scene and DFX smoke tests
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 |
14b97d2 to
3fb4879
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
258-273: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPip cache is restored after the self-hosted venv already installed everything.
Cache pip packages(267-273 / 412-418) runs afterSet up Python ... (self-hosted)(258-266 / 403-410), but the self-hosted branch does its ownpip install torch .../pip install '.[test]'inside that earlier step. SinceInstall dependencies(which runs after the cache-restore step) is skipped for self-hosted (if: !contains(matrix.runner, 'self-hosted')), the self-hosted job never benefits from the restored~/.cache/pip— it re-downloads torch and test deps from PyPI on every run, defeating the point of caching and undercutting the perf win of moving to a local CPU pool.Move the
Cache pip packagesstep above bothSet up Pythonsteps so the venv install can hit the cache.Also applies to: 403-418
🤖 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/workflows/ci.yml around lines 258 - 273, Move the “Cache pip packages” step before both Python setup steps, including the self-hosted setup identified by “Set up Python ${{ matrix.python-version }} (self-hosted)”. Preserve its existing cache configuration so the self-hosted venv installations can reuse the restored pip cache.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
223-250: 🩺 Stability & Availability | 🔵 TrivialConfirm the self-hosted
cpupool is provisioned before this merges.The self-hosted branch only asserts tool presence (
command -v ninja/g++/dot) instead of installing — under the defaultbash -estep shell, any of these being missing will hard-fail the job immediately, for every future PR touching a2a3/a5, until the pool is fixed.Please confirm the
self-hosted, cpurunner pool already hasninja,g++(or a compilerg++-15can symlink to), andgraphviz(dot) pre-installed before this routing goes live.Also applies to: 368-395
🤖 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/workflows/ci.yml around lines 223 - 250, Ensure the self-hosted, cpu runner pool is provisioned before enabling this workflow routing: install and verify ninja, g++, and graphviz (dot), with a usable compiler available for the g++-15 symlink fallback. Confirm the pool configuration covers the self-hosted branch in “Set up C++ compiler” for all affected matrix jobs.
207-347: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftHeavy duplication between
st-sim-a2a3andst-sim-a5(compiler setup, Python setup,$PYTHONselection repeated ~8× per job).
st-sim-a2a3(204-347) andst-sim-a5(349-477) duplicate the entire "Set up C++ compiler" block, both Python-setup steps, and the 4-linePYTHON=python; if [[ ... self-hosted ... ]]; then PYTHON=.venv/bin/python; fisnippet in every single pytest step (scene test + 4 DFX smokes × 2 jobs = 10 repetitions). This duplication is exactly how the pip-cache-ordering bug above ended up copy-pasted into both jobs — any future fix has to be applied twice, correctly, or it silently diverges again.Two independent, complementary fixes:
- Compute
$PYTHONonce (right after the Python-setup steps) and export via$GITHUB_ENV, so every later step just does"$PYTHON" -m pytest ...without re-deriving it.- Longer-term, consider extracting the compiler/Python setup into a composite action shared by both jobs.
♻️ Minimal fix: compute `$PYTHON` once
+ - name: Select Python interpreter + run: | + PYTHON=python + if [[ '${{ matrix.runner }}' == *self-hosted* ]]; then + PYTHON=.venv/bin/python + fi + echo "PYTHON=$PYTHON" >> "$GITHUB_ENV" + - name: Run pytest scene tests (a2a3sim) run: | - PYTHON=python - if [[ '${{ matrix.runner }}' == *self-hosted* ]]; then - PYTHON=.venv/bin/python - fi "$PYTHON" -m pytest examples tests/st --platform a2a3sim --device 0-15 -v \ --pto-session-timeout 600 --require-pto-isa(repeat removal for every DFX smoke step in both
st-sim-a2a3andst-sim-a5)🤖 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/workflows/ci.yml around lines 207 - 347, Reduce duplicated Python selection in both st-sim-a2a3 and st-sim-a5 by computing the interpreter once after the Python setup steps and exporting it through GITHUB_ENV. Update the scene-test and every DFX smoke step to invoke the exported PYTHON directly, removing each repeated self-hosted conditional; leave compiler setup and other job behavior unchanged.
🤖 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 32-35: Update all three actions/checkout@v5 steps, including the
steps near the referenced self-hosted checkouts, to set persist-credentials to
false in their with configuration while preserving the existing fetch-depth
settings.
- Around line 10-19: Update the Resolve diff refs step to pass workflow_dispatch
inputs through the step env rather than interpolating them directly in the
shell, and read those environment variables when resolving refs. Handle base_sha
and head_sha independently so each supplied value is preserved while only the
missing field falls back to its computed default; ensure the resulting outputs
continue to feed downstream steps safely.
In `@docs/ci.md`:
- Line 133: Update the CI documentation sentence describing the sim Linux leg to
remove “when available” and state that it is routed unconditionally to the
self-hosted `cpu` pool, with macOS remaining GitHub-hosted; do not imply
automatic fallback.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 258-273: Move the “Cache pip packages” step before both Python
setup steps, including the self-hosted setup identified by “Set up Python ${{
matrix.python-version }} (self-hosted)”. Preserve its existing cache
configuration so the self-hosted venv installations can reuse the restored pip
cache.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 223-250: Ensure the self-hosted, cpu runner pool is provisioned
before enabling this workflow routing: install and verify ninja, g++, and
graphviz (dot), with a usable compiler available for the g++-15 symlink
fallback. Confirm the pool configuration covers the self-hosted branch in “Set
up C++ compiler” for all affected matrix jobs.
- Around line 207-347: Reduce duplicated Python selection in both st-sim-a2a3
and st-sim-a5 by computing the interpreter once after the Python setup steps and
exporting it through GITHUB_ENV. Update the scene-test and every DFX smoke step
to invoke the exported PYTHON directly, removing each repeated self-hosted
conditional; leave compiler setup and other job behavior unchanged.
🪄 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 Plus
Run ID: c70b9a1c-9c45-46ba-af0b-849519b9671d
📒 Files selected for processing (3)
.github/workflows/ci.ymldocs/ci.mdsimpler_setup/runtime_compiler.py
85e456b to
ee6a6c8
Compare
When GitHub-hosted runners are congested, a repo admin can validate a PR head on the repo-level self-hosted runners: - ci-self-cpu.yml checks out the PR head (repository + ref inputs, so fork PRs work), then runs T1 (no-hardware Linux: pre-commit, ut, packaging, profiling-flags-smoke, st-sim) on [self-hosted, cpu] and T3 (NPU jobs) on [self-hosted, a2a3/a5]; T2 (macOS) is intentionally absent. A lane-local detect-changes (same NON_CODE vocabulary as ci.yml) skips jobs a diff cannot affect. - ci-self-cpu-button.yml turns a "/run-cpu" PR comment into a lane run, gated on repo-admin permission only. It calls the lane via workflow_call: issue_comment from fork PRs runs with a read-only token, which cannot dispatch workflows.
ee6a6c8 to
da7ba4c
Compare
ChaoWao
changed the title
2 participants
Replace the router-based sim-to-self-hosted-cpu routing with a manual emergency lane, triggered on demand when GitHub-hosted runners are congested:
ci-self-cpu.yml— emergency lane: checks out the PR head (repository+refinputs, so fork PRs work), runs T1 (no-hardware Linux: pre-commit, ut, packaging, profiling-flags-smoke, st-sim-{a2a3,a5}) on[self-hosted, cpu]and T3 (NPU: ut-a2a3 / st-onboard-a2a3 / ut-a5 / st-onboard-a5) on[self-hosted, a2a3/a5]; T2 (macOS) is intentionally absent. A lane-localdetect-changes(same NON_CODE vocabulary asci.yml) skips jobs a diff cannot affect.ci-self-cpu-button.yml— a/run-cpuPR comment command, gated on repo-admin permission only (getCollaboratorPermissionLevel == admin); calls the lane viaworkflow_callbecauseissue_commentfrom fork PRs runs with a read-only token, which cannot dispatch workflows.docs/ci.md— documents the lane, triggers, gating, and the cpu-runner provisioning contract.Why manual rather than automatic: GitHub Actions has no preferential runner selection, so "github-first, cpu-fallback" is not expressible in
runs-on— the earlier router (dispatch + queue-timeout fallback) was dropped in favor of a human decision point. The cpu runner is unused in normal times and only occupied when an admin triggers the lane.Testing
docs/ci.md): cpu-runner provisioning (dnf toolchain + torch aarch64 wheel) and verifying the admin gate works under the fork-PR read-only token.🤖 Generated with Claude Code