Route sim Linux CI to self-hosted CPU runners - #1591
Conversation
📝 WalkthroughWalkthroughSimulation CI jobs now support hosted and self-hosted runners, configure tools and Python per runner type, and execute tests through the selected interpreter. CI documentation reflects the mixed Linux CPU and macOS runner strategy. ChangesSimulation runner strategy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant Runner
participant PythonEnvironment
participant Pytest
GitHubActions->>Runner: Select hosted or self-hosted runner
Runner->>PythonEnvironment: Configure toolchain and Python
PythonEnvironment->>Pytest: 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 |
![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
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
167-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftEntire setup block duplicated between
st-sim-a2a3andst-sim-a5. The C++ compiler setup and both Python setup branches are copy-pasted verbatim, creating drift risk since both jobs must be kept manually in sync.
.github/workflows/ci.yml#L167-L226: extract into a shared composite action (e.g..github/actions/setup-sim-toolchain) parameterized bymatrix.python-version, used byst-sim-a2a3..github/workflows/ci.yml#L309-L371: call the same composite action fromst-sim-a5instead of duplicating the block.🤖 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 167 - 226, Extract the duplicated C++ compiler and Python setup steps into a shared composite action, such as setup-sim-toolchain, parameterized by matrix.python-version. Update the st-sim-a2a3 block at .github/workflows/ci.yml:167-226 to invoke the action, and replace the duplicated setup block in st-sim-a5 at .github/workflows/ci.yml:309-371 with the same invocation; preserve the existing runner-specific behavior and environment.
241-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
PYTHONexecutable selection duplicated across ~10 pytest steps in both jobs. ThePYTHON=python; if [[ self-hosted ]]; then PYTHON=.venv/bin/python; fiblock is copy-pasted into every pytest invocation; centralizing it once (e.g. exportingPYTHONvia$GITHUB_ENVin the "Set up Python" steps) removes the duplication in a single fix.
.github/workflows/ci.yml#L241-L247: drop the inlinePYTHON=block in thest-sim-a2a3scene-test step; rely on a job-level$PYTHONenv var..github/workflows/ci.yml#L256-L260: same removal in thedep_gen smokestep..github/workflows/ci.yml#L271-L308: same removal in the four remaining DFX smoke steps (dep_gen (host_build_graph),l2_swimlane,PMU,args_dump)..github/workflows/ci.yml#L386-L392: same removal in thest-sim-a5scene-test step..github/workflows/ci.yml#L401-L435: same removal in thest-sim-a5DFX smoke steps.🤖 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 241 - 247, Centralize Python executable selection in the job-level “Set up Python” steps by exporting the resolved value through GITHUB_ENV, using python for hosted runners and .venv/bin/python for self-hosted runners. Remove the duplicated PYTHON assignment and conditional blocks from all listed pytest steps: .github/workflows/ci.yml lines 241-247, 256-260, 271-308, 386-392, and 401-435; each step should reuse the job-level PYTHON variable.
196-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnreachable hosted-Linux apt-get branch in both jobs. The matrix only has a self-hosted Linux entry and a
macos-latesthosted entry, sorunner.os == "Linux"is only ever true for the self-hosted case; the apt-get/sudo install branch can never execute in either job.
.github/workflows/ci.yml#L196-L205: remove (or annotate as intentionally-dormant, e.g. reserved for a future hosted-Linux matrix entry) thesudo apt-getbranch inst-sim-a2a3..github/workflows/ci.yml#L341-L350: same cleanup in the mirrored branch inst-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 196 - 205, Remove or explicitly annotate the unreachable hosted-Linux apt-get/sudo installation branch in both st-sim-a2a3 at .github/workflows/ci.yml lines 196-205 and st-sim-a5 at lines 341-350; apply the same cleanup to both mirrored workflow sections without changing the active self-hosted Linux or macOS paths.
🤖 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 181-182: Add with.persist-credentials set to false on the Checkout
repository step in both simulator jobs, st-sim-a2a3 at .github/workflows/ci.yml
lines 181-182 and st-sim-a5 at lines 326-327; no other checkout steps require
changes.
In `@docs/ci.md`:
- Line 132: Update the CI documentation sentence describing the sim Linux leg so
it states that the leg requires the self-hosted ["self-hosted", "cpu"] runner,
without implying fallback to ubuntu-latest when that pool is unavailable. Keep
the macOS GitHub-hosted description unchanged.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 167-226: Extract the duplicated C++ compiler and Python setup
steps into a shared composite action, such as setup-sim-toolchain, parameterized
by matrix.python-version. Update the st-sim-a2a3 block at
.github/workflows/ci.yml:167-226 to invoke the action, and replace the
duplicated setup block in st-sim-a5 at .github/workflows/ci.yml:309-371 with the
same invocation; preserve the existing runner-specific behavior and environment.
- Around line 241-247: Centralize Python executable selection in the job-level
“Set up Python” steps by exporting the resolved value through GITHUB_ENV, using
python for hosted runners and .venv/bin/python for self-hosted runners. Remove
the duplicated PYTHON assignment and conditional blocks from all listed pytest
steps: .github/workflows/ci.yml lines 241-247, 256-260, 271-308, 386-392, and
401-435; each step should reuse the job-level PYTHON variable.
- Around line 196-205: Remove or explicitly annotate the unreachable
hosted-Linux apt-get/sudo installation branch in both st-sim-a2a3 at
.github/workflows/ci.yml lines 196-205 and st-sim-a5 at lines 341-350; apply the
same cleanup to both mirrored workflow sections without changing the active
self-hosted Linux or macOS paths.
🪄 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: 6707c57d-428e-43f0-8cdf-4ca622927cb4
📒 Files selected for processing (2)
.github/workflows/ci.ymldocs/ci.md
| - name: Checkout repository | ||
| uses: actions/checkout@v5 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing persist-credentials: false on checkout in both simulator jobs. Both jobs can now run on a self-hosted runner where the workspace may persist across job runs, widening the exposure window for the checkout-persisted GITHUB_TOKEN; neither job needs to push, so credentials should not be persisted.
.github/workflows/ci.yml#L181-L182: addwith: persist-credentials: falseto theCheckout repositorystep inst-sim-a2a3..github/workflows/ci.yml#L326-L327: addwith: persist-credentials: falseto theCheckout repositorystep inst-sim-a5.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 181-182: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 1 file
.github/workflows/ci.yml#L181-L182(this comment).github/workflows/ci.yml#L326-L327
🤖 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 181 - 182, Add
with.persist-credentials set to false on the Checkout repository step in both
simulator jobs, st-sim-a2a3 at .github/workflows/ci.yml lines 181-182 and
st-sim-a5 at lines 326-327; no other checkout steps require changes.
Source: Linters/SAST tools
| ### Scheduling constraints | ||
|
|
||
| - Sim scene tests and no-hardware unit tests run on github-hosted runners (no hardware). | ||
| - Sim scene tests and no-hardware unit tests run on no-hardware runners; the sim Linux leg is now pointed at the `cpu` self-hosted pool when available, while macOS stays GitHub-hosted. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not imply an automatic fallback when the self-hosted pool is unavailable.
The workflow always includes the ["self-hosted", "cpu"] matrix leg; if no matching runner is online, GitHub Actions queues that leg rather than falling back to ubuntu-latest. Replace “when available” with wording that describes the required runner, or implement an explicit fallback.
🤖 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/ci.md` at line 132, Update the CI documentation sentence describing the
sim Linux leg so it states that the leg requires the self-hosted ["self-hosted",
"cpu"] runner, without implying fallback to ubuntu-latest when that pool is
unavailable. Keep the macOS GitHub-hosted description unchanged.
2 participants
Recreate the self-hosted CPU sim routing PR from the current fixed branch state.