[CI] Split arm-ci into a standalone workflow#6699
Conversation
9c05f8f to
02b3976
Compare
The arm-ci job lived in build.yaml (Docker + Tests) alongside every test-* job. GitHub only exposes "Re-run failed jobs" after the whole run completes, so a fast test job that failed early could not be retried until arm-ci's slow arm64 build+render finished (up to its 60-minute timeout on the scarce DGX Spark runners). arm-ci is continue-on-error (informational) and never needed to hold the run open. Move it to .github/workflows/arm-ci.yml as its own run, with independent status and an independent "Re-run failed jobs". Extract the change-detection gate into a reusable workflow, .github/workflows/detect-changes.yml, called by both build.yaml and arm-ci.yml with their own path patterns. arm-ci triggers on every PR and gates the arm64 build behind that detector + `if:`, so it is safe to promote to a required status check later: an irrelevant PR skips the job (green to branch protection) rather than never creating the check, which a workflow-level paths filter would do (leaving a required check pending forever). This mirrors the changes+if pattern build.yaml already uses for its required test jobs. The detector lists each workflow's real inputs, including the root files Dockerfile.base copies (pyproject.toml, environment.yml, isaaclab.*, .dockerignore) that the old gating missed, applied to both callers. build.yaml keeps its inline config job and all test-* jobs unchanged; its changes job now calls the shared reusable workflow.
02b3976 to
dfbc442
Compare
Greptile SummaryThis PR splits the
Confidence Score: 4/5Safe to merge — the extraction is a structural reorganization with no behavioral changes to the test jobs themselves; ARM CI runs independently on this PR and the Load Config job already confirms the change-detection plumbing works end-to-end. The core migration is faithful (arm-ci steps copied verbatim), the changes+if: required-check pattern is preserved, and the fail-safe API-error path carries over correctly. The two observations are naming (run_docker_tests in a generic reusable workflow) and a duplicated tag formula that could drift silently — neither affects current behavior. detect-changes.yml deserves a second look on the run_docker_tests output name if more workflows adopt this reusable gate in the future. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
PR([PR / Push Event]) --> BW[build.yaml Docker + Tests]
PR --> AW[arm-ci.yml ARM CI]
BW --> BC{changes job calls detect-changes.yml}
AW --> AC{changes job calls detect-changes.yml}
DC[detect-changes.yml reusable workflow] --> |run_docker_tests output| BC
DC --> |run_docker_tests output| AC
BC -- run_docker_tests=true --> BCONFIG[config job build.yaml]
BC -- run_docker_tests=false --> BSKIP([test-* jobs skipped green to branch protection])
AC -- run_docker_tests=true --> ACONFIG[config job arm-ci.yml]
AC -- run_docker_tests=false --> ASKIP([arm-ci skipped green to branch protection])
BCONFIG --> BTESTS[test-* matrix jobs ubuntu-latest]
ACONFIG --> ARMJOB[arm-ci job self-hosted arm64 continue-on-error]
ARMJOB --> ARTIFACT[Upload arm-ci-reports]
Reviews (1): Last reviewed commit: "[CI] Split arm-ci into a standalone work..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| outputs: | ||
| run_docker_tests: | ||
| description: "Whether the caller's gated jobs should run for this event." | ||
| value: ${{ jobs.detect.outputs.run_docker_tests }} |
There was a problem hiding this comment.
run_docker_tests, which is semantically correct for build.yaml but misleading for arm-ci.yml, where callers must write needs.changes.outputs.run_docker_tests == 'true' to gate an ARM build. A generic name like should_run would communicate the reusable intent without implying Docker, and wouldn't require any build.yaml change since build.yaml's changes job reference would just rename the output it reads.
| outputs: | |
| run_docker_tests: | |
| description: "Whether the caller's gated jobs should run for this event." | |
| value: ${{ jobs.detect.outputs.run_docker_tests }} | |
| outputs: | |
| should_run: | |
| description: "Whether the caller's gated jobs should run for this event." | |
| value: ${{ jobs.detect.outputs.run_docker_tests }} |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| - id: image_tag | ||
| shell: bash | ||
| env: | ||
| EVENT_NAME: ${{ github.event_name }} | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| REF_NAME: ${{ github.ref_name }} | ||
| SHA: ${{ github.sha }} | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| if [ "$EVENT_NAME" = "pull_request" ]; then | ||
| ref_component="pr-${PR_NUMBER}" | ||
| else | ||
| ref_component="$REF_NAME" | ||
| fi | ||
|
|
||
| # Sanitize the ref name for use as a Docker tag suffix. | ||
| sanitized_ref=$(echo "$ref_component" | sed 's/[^a-zA-Z0-9._-]/-/g') | ||
| echo "ci_image_tag=isaac-lab-ci:${sanitized_ref}-${SHA}" >> "$GITHUB_OUTPUT" | ||
| echo "CI image tag: isaac-lab-ci:${sanitized_ref}-${SHA}" |
There was a problem hiding this comment.
ci_image_tag formula divergence risk
The ci_image_tag computation (pr-<number> vs. ref_name, sanitize, append SHA) is duplicated between arm-ci.yml and build.yaml's config job. Both produce independent tags for their respective images, which is correct, but if the naming formula is ever changed in one file (e.g., adding a prefix, changing the separator), the other must be updated manually. A stale discrepancy would be invisible until someone tries to correlate an arm64 image tag with an x86 image tag from the same run. At minimum, a comment in each config job pointing to the other would help future editors keep them in sync.
There was a problem hiding this comment.
this is valid but I would say non-blocking since this is already commented here and is a nit
- Rename the reusable detect-changes output run_docker_tests -> should_run, since the shared gate now drives arm-ci as well as the Docker + Tests jobs; both callers updated. - Add cross-reference comments in each config job noting the ci_image_tag formula is intentionally duplicated (independent -arm64 and x86 images) and must be kept in sync.
| # Dockerfile.base copies (pyproject.toml, environment.yml, isaaclab.*) that a | ||
| # path filter would otherwise miss. | ||
| changes: | ||
| name: Detect Changes |
There was a problem hiding this comment.
suggestion (non-blocking): Both callers name the job Detect Changes, should we add a Detect Changes (ARM) and Detect Changes (Docker)
There was a problem hiding this comment.
The info is available at workflow level, e.g. ARM CI / Detect Changes / Detect Changes (pull_request).
|
overall PR looks good, made a non-blocking comment. we can merge |
Convert the shared change-detection gate from a reusable workflow (.github/workflows/detect-changes.yml) to a composite action (.github/actions/detect-changes), matching the repo convention: all shared CI logic lives under .github/actions/ and there were no reusable workflows before. This also flattens the check name a reviewer flagged: a reusable-workflow call renders as 'ARM CI / Detect Changes / Detect Changes' (workflow / caller job / inner job), whereas the composite action runs as steps in a single job -> 'ARM CI / Detect Changes', distinguished from the Docker + Tests copy by the workflow name. The changes job now checks out sparsely and calls the action, like every other action caller in the repo.
The job's check now reads 'ARM CI / Build & Test' instead of the redundant 'ARM CI / arm-ci' — the workflow name already conveys the platform, so the job name should describe what it does (build + marker tests). Job id stays 'arm-ci'.
3 participants
Description
1. Summary
Moves the
arm-cijob out ofbuild.yaml("Docker + Tests") into its own workflow,.github/workflows/arm-ci.yml, and extracts the change-detection gate into a reusable workflow,.github/workflows/detect-changes.yml, shared by both.Problem.
arm-cishared the "Docker + Tests" run with everytest-*job. GitHub only exposes "Re-run failed jobs" once the entire run completes, so a fast test job that failed early could not be retried untilarm-ci's slow arm64 build + render finished (up to its 60-minute timeout on the scarce DGX Spark runners).arm-ciiscontinue-on-error(informational) and never needed to hold the run open.Fix.
arm-cinow owns its own workflow run — independent status, independent Re-run failed jobs, and no coupling to the main workflow's lifecycle.2. What changed
.github/workflows/arm-ci.yml— thearm-cijob (verbatim:continue-on-error, 60-minute timeout, same build / marker-test / cartpole-smoke steps) plus an inlinedconfigstep, gated by the shared change detector..github/workflows/detect-changes.yml— reusable (workflow_call) change-detection gate; each caller passes its ownpatterns.build.yaml— thearm-ciblock is removed; itschangesjob now calls the reusable workflow (same patterns);configand alltest-*jobs are unchanged.3. Design notes
arm-citriggers on every PR and gates the arm64 build behind the sharedchangesdetector +if:. An irrelevant PR skips the job (which branch protection reads as green) rather than never creating the check. Soarm-cican be promoted to a required status check later with zero workflow change — a workflow-levelpaths:filter could not (a not-triggered required check stays pending forever). This is the samechanges+if:patternbuild.yamlalready uses for its required test jobs.patterns(<ERE regex> :: <description>per line).build.yamlkeeps its exact patterns;arm-ciadditionally lists its real inputs —pyproject.toml,environment.yml,isaaclab.*(the root filesDockerfile.baseCOPYs andresolve-ov-pinsreads) — which the old gating missed.main/develop/release/**always runs (post-merge integration);pull_requestruns only when arm-relevant paths change.arm-ci.yml(only the base-image name/tag + CI image tag it needs) — not shared, sincearm-cibuilds its own independent-arm64image.4. Validation
./isaaclab.sh -fclean on all changed files; YAML parses.build.yaml's downstreamneeds.changes.outputs.run_docker_testsreferences preserved (build,build-curobo); inline detect logic fully removed.Load Configjob succeeds andarm-ciis confirmed not a required check today.5. Notes
arm-cistayscontinue-on-error/ non-required for now; the structure above makes promoting it to a required check a branch-protection-only change.