Route sim Linux CI to self-hosted CPU runners - #1595
Conversation
Use the self-hosted CPU runner pool for sim Linux jobs, avoid sudo, and run those jobs through the repository venv.
|
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSimulation CI jobs for ChangesSimulation CI portability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Matrix as Simulation matrix
participant Runner as GitHub Actions runner
participant Python as Selected Python interpreter
participant Tests as Scene and DFX tests
Matrix->>Runner: Select hosted or self-hosted runner
Runner->>Python: Configure python or .venv/bin/python
Python->>Tests: Execute "$PYTHON" -m pytest
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)
docs/ci.md (1)
105-116: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMake the
ubuntu-latestcapacity callout less definitive.GitHub-hosted runner CPU allocations are documented/expected to change and larger runners exist, so avoid presenting “4-vCPU
ubuntu-latest” as a fixed portability target. Keep the current logical-CPU / cgroup-ignoring note foros.cpu_count().🤖 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` around lines 105 - 116, Revise the runner-capacity wording near the Sim jobs description to make the ubuntu-latest CPU allocation an illustrative or currently expected value rather than a fixed 4-vCPU portability target, and acknowledge that GitHub-hosted runner sizes can change or vary. Preserve the existing explanation that os.cpu_count() reports host logical CPUs and ignores cgroup CPU quotas.Source: MCP tools
🤖 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 192-194: Relocate the g++-15 compiler shims in both the a2a3 block
at .github/workflows/ci.yml#L192-L194 and the a5 block at
.github/workflows/ci.yml#L337-L339 from RUNNER_TEMP/bin to a workspace-relative
directory such as GITHUB_WORKSPACE/.ci-bin, then append that same directory to
GITHUB_PATH in each block.
- Around line 218-225: The self-hosted Python setup steps use an unversioned
interpreter instead of the matrix-selected version. In .github/workflows/ci.yml
lines 218-225 and 363-370, update the venv creation command to invoke the quoted
matrix-specific interpreter python"${{ matrix.python-version }}" so missing
versions fail rather than falling back to an arbitrary python3.
- Around line 181-182: Disable persisted checkout credentials on both
actions/checkout steps in .github/workflows/ci.yml at lines 181-182 and 326-327
by adding the persist-credentials: false setting under each step’s with
configuration; no other workflow changes are needed.
In `@docs/ci.md`:
- Around line 11-13: Update the runner-mapping documentation in docs/ci.md so
the ut job is listed only on its actual hosted Ubuntu/macOS matrix, removing
self-hosted [cpu] from its mapping. Keep the self-hosted [cpu] designation
limited to st-sim-* Linux jobs unless the CI workflow is explicitly changed to
run ut there.
---
Nitpick comments:
In `@docs/ci.md`:
- Around line 105-116: Revise the runner-capacity wording near the Sim jobs
description to make the ubuntu-latest CPU allocation an illustrative or
currently expected value rather than a fixed 4-vCPU portability target, and
acknowledge that GitHub-hosted runner sizes can change or vary. Preserve the
existing explanation that os.cpu_count() reports host logical CPUs and ignores
cgroup CPU quotas.
🪄 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: 310900eb-530b-4024-8c2c-a6cb7d3fa0c0
📒 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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
sed -n '160,200p' .github/workflows/ci.yml
echo
sed -n '310,340p' .github/workflows/ci.yml
echo
echo "== checkout occurrences =="
rg -n "actions/checkout|persist-credentials|pytest|python|uv|pip" .github/workflows/ci.ymlRepository: hw-native-sys/simpler
Length of output: 15037
🌐 Web query:
actions/checkout persist-credentials default true local Git config
💡 Result:
In the actions/checkout GitHub Action, the persist-credentials input determines whether the authentication token or SSH key used for the checkout is configured in the local Git environment, allowing subsequent steps in the same job to run authenticated Git commands [1][2][3]. The default value for persist-credentials is true [1][3]. Key details regarding this setting include: 1. Functionality: When set to true (default), the action configures the Git credential helper or local Git config so that subsequent steps can perform authenticated operations (e.g., git fetch, git push) without needing to manually re-authenticate [2][3]. 2. Security Improvement: As of version v6.0.0, the action was updated to improve credential security [3][4]. Instead of storing credentials directly in the local.git/config file, it now stores them in a separate, more secure location (under $RUNNER_TEMP) while still maintaining the functionality for authenticated Git commands [3][4]. 3. Opt-out: Users can explicitly set persist-credentials: false if they do not want the credentials to be available to subsequent steps in the job [2][3]. 4. Context: While there have been community requests to change this default to false to reduce the risk of accidental exposure of the GITHUB_TOKEN, the default remains true as of July 2026 to ensure backward compatibility and ease of use for standard workflow requirements [5][6].
Citations:
- 1: https://github.com/actions/checkout/blob/cbb722410c2e876e24abbe8de2cc27693e501dcb/action.yml
- 2: https://github.com/marketplace/actions/checkout?version=v3.6.0
- 3: https://github.com/actions/checkout/tree/v6.0.0
- 4: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 5: Change the default value of persist-credentials to false actions/checkout#1687
- 6: Remove
persist-credentialsor change the default tofalseactions/checkout#485
Disable persisted checkout credentials before executing tests.
actions/checkout preserves workflow credentials for later Git commands by default; the following pytest steps execute repository-controlled code under the same job identity. Set persist-credentials: false here unless the job needs authenticated Git access afterward.
.github/workflows/ci.yml#L181-L182: addwith: persist-credentials: falseto the checkout step..github/workflows/ci.yml#L326-L327: add the same setting to the checkout step.
🧰 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, Disable persisted checkout
credentials on both actions/checkout steps in .github/workflows/ci.yml at lines
181-182 and 326-327 by adding the persist-credentials: false setting under each
step’s with configuration; no other workflow changes are needed.
Source: Linters/SAST tools
| mkdir -p "$RUNNER_TEMP/bin" | ||
| ln -sf "$(command -v g++)" "$RUNNER_TEMP/bin/g++-15" | ||
| echo "$RUNNER_TEMP/bin" >> "$GITHUB_PATH" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the compiler shim inside the assigned workspace.
Both branches create $RUNNER_TEMP/bin, outside the working directory. Use a workspace-relative directory (for example, $GITHUB_WORKSPACE/.ci-bin) before appending it to GITHUB_PATH.
.github/workflows/ci.yml#L192-L194: relocate the a2a3g++-15shim..github/workflows/ci.yml#L337-L339: relocate the a5g++-15shim.
As per coding guidelines, “Create new subdirectories only under the assigned working directory; when uncertain about modifying another area, ask the user first.”
📍 Affects 1 file
.github/workflows/ci.yml#L192-L194(this comment).github/workflows/ci.yml#L337-L339
🤖 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 192 - 194, Relocate the g++-15
compiler shims in both the a2a3 block at .github/workflows/ci.yml#L192-L194 and
the a5 block at .github/workflows/ci.yml#L337-L339 from RUNNER_TEMP/bin to a
workspace-relative directory such as GITHUB_WORKSPACE/.ci-bin, then append that
same directory to GITHUB_PATH in each block.
Source: Coding guidelines
| - name: Set up Python ${{ matrix.python-version }} (self-hosted) | ||
| if: ${{ contains(matrix.runner, 'self-hosted') }} | ||
| run: | | ||
| python3 -m venv --system-site-packages .venv | ||
| source .venv/bin/activate | ||
| pip install --upgrade pip | ||
| pip install torch --index-url https://download.pytorch.org/whl/cpu | ||
| pip install '.[test]' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt around ci.yml =="
wc -l .github/workflows/ci.yml
sed -n '160,240p' .github/workflows/ci.yml
sed -n '310,385p' .github/workflows/ci.yml
echo
echo "== python-version matrix usages =="
rg -n "python-version|matrix\.runner|python3 -m venv|source \.venv/bin/activate" .github/workflows/ci.ymlRepository: hw-native-sys/simpler
Length of output: 9748
Enforce the matrix Python version on self-hosted runners.
The self-hosted setup still uses the bare python3 interpreter instead of the matrix value, so python3 can resolve to any preinstalled interpreter despite the job declaring Python 3.10. Use a versioned/missing-fail-safe interpreter here too:
.github/workflows/ci.yml#L218-L225: usepython"${{ matrix.python-version }}"(quote it)..github/workflows/ci.yml#L363-L370: apply the same interpreter selection for a5.
📍 Affects 1 file
.github/workflows/ci.yml#L218-L225(this comment).github/workflows/ci.yml#L363-L370
🤖 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 218 - 225, The self-hosted Python
setup steps use an unversioned interpreter instead of the matrix-selected
version. In .github/workflows/ci.yml lines 218-225 and 363-370, update the venv
creation command to invoke the quoted matrix-specific interpreter python"${{
matrix.python-version }}" so missing versions fail rather than falling back to
an arbitrary python3.
| 2. **Runner matches hardware tier** — no-hardware tests run on `ubuntu-latest`; platform-specific tests run on self-hosted runners with the matching label (`a2a3`, `a5`). | ||
| 3. **`--platform` is the only filter** — pytest uses `--platform` + the `requires_hardware` marker; ctest uses label `-LE` exclusion. No `-m st`, no `-m "not requires_hardware"`. | ||
| 4. **sim = no hardware** — `a2a3sim`/`a5sim` jobs run on github-hosted runners alongside unit tests. | ||
| 4. **sim = no hardware** — `a2a3sim`/`a5sim` keep the macOS leg on GitHub-hosted runners, while the Linux leg can be routed to the local `cpu` self-hosted pool to escape the 4-vCPU ceiling. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the no-hardware runner mapping consistent.
Principle 2 says no-hardware tests run on ubuntu-latest, while principle 4 and the table also classify simulation Linux jobs as self-hosted [cpu]. More importantly, .github/workflows/ci.yml currently runs ut only on its hosted Ubuntu/macOS matrix, so listing self-hosted [cpu] for ut is inaccurate. Limit the self-hosted CPU mapping to st-sim-*, or update the workflow if ut should also run there.
Also applies to: 145-145
🤖 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` around lines 11 - 13, Update the runner-mapping documentation in
docs/ci.md so the ut job is listed only on its actual hosted Ubuntu/macOS
matrix, removing self-hosted [cpu] from its mapping. Keep the self-hosted [cpu]
designation limited to st-sim-* Linux jobs unless the CI workflow is explicitly
changed to run ut there.
|
Closing duplicate PR; keeping #1570 as the canonical runner-routing PR. |
1 participant
Fresh PR to verify CI trigger behavior after the runner routing change.