Skip to content

Reduce mac CI cost#19708

Merged
metascroy merged 5 commits into
mainfrom
reduce-mac-cost
May 21, 2026
Merged

Reduce mac CI cost#19708
metascroy merged 5 commits into
mainfrom
reduce-mac-cost

Conversation

@metascroy
Copy link
Copy Markdown
Contributor

@metascroy metascroy commented May 20, 2026

Skip macOS CoreML/MPS jobs on PRs that don't touch their relevant source. Non-PR events (push to main, schedule, workflow_dispatch, ciflow/* tag) bypass the gate so HUD signal on main is preserved. Linux/Windows jobs untouched.

@pytorch-bot
Copy link
Copy Markdown

pytorch-bot Bot commented May 20, 2026

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19708

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

❌ 6 Pending, 1 Unclassified Failure

As of commit 869c776 with merge base 3d86cc7 (image):

UNCLASSIFIED FAILURE - DrCI could not classify the following job because the workflow did not run on the merge base. The failure may be pre-existing on trunk or introduced by this PR:

  • Check Labels / Check labels (gh) (this job did not run on the merge base, so DrCI cannot tell whether the failure is pre-existing)
    RuntimeError: GraphQL query

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 20, 2026
@metascroy
Copy link
Copy Markdown
Contributor Author

@claude review this PR

@claude
Copy link
Copy Markdown

claude Bot commented May 20, 2026

Claude finished @metascroy's task in 3m 37s —— View job


Review of PR #19708 — Reduce mac CI cost

  • Read changed files and understand the CI gating logic
  • Check path filters for correctness and completeness
  • Review the changed-files reusable workflow pattern
  • Post review feedback

Overall Assessment

The approach is sound: use a lightweight _get-changed-files.yml reusable workflow to emit the file list, then gate expensive macOS jobs with contains() checks. The github.event_name != 'pull_request' bypass correctly preserves signal on push/schedule/dispatch.

Detailed Findings

1. contains() substring semantics — works in your favor

contains() does substring matching on the space-separated file list, so contains(files, 'backends/apple/coreml') matches any file under that directory. This is the right approach for directory-prefix gating, and false positives (job runs when not strictly needed) are the safe direction.

One nit: contains(files, 'examples/models') in test-models-macos-coreml (trunk.yml:815) will also match files like examples/models_v2/... if such a path ever exists. Low risk in practice, but worth noting.

2. Missing paths that could affect gated jobs

  • test-llama-runner-macos uses examples/models/llama/install_requirements.sh in its script, but that path isn't in its if: filter. A change solely to that file won't trigger the job. (Covered by examples/models/llama matching as a prefix — actually this IS covered since contains(files, 'examples/models/llama') would match examples/models/llama/install_requirements.sh. Scratch this, it's fine.)

  • Core runtime changes (runtime/, exir/, extension/module/) could break any backend. The nightly schedule is the safety net here — a reasonable trade-off given the cost savings. Just confirming this is intentional.

3. test-huggingface-transformers-macos combined condition (trunk.yml:989-1000)

The nested parenthesization combining the fork check with the path gate is correctly structured:

if: |
  !github.event.pull_request.head.repo.fork &&
  (
    github.event_name != 'pull_request' ||
    contains(...)
  )

On non-PR events, github.event.pull_request.head.repo.fork is null, and !null is truthy in GitHub expressions, so the job runs. Correct.

4. Zephyr job refactoring (trunk.yml:66-224)

The matrix collapses from 5 entries to 3 (one per target), with the inlined script running both hello-executorch and mv2-ethosu samples sequentially per target. A few observations:

  • This reduces job parallelism (5 → 3 jobs) but saves runner spin-up overhead and reduces total billed minutes.
  • The test_zephyr.sh script is deleted and its logic inlined. This adds ~140 lines to the workflow but removes a layer of indirection.
  • The MV2 section correctly skips cortex-m55 with continue, matching the old behavior where cortex-m55 only appeared in the hello-executorch matrix entries.
  • Note: The PR title is "Reduce mac CI cost" but this Zephyr change affects a Linux job. Consider mentioning it in the PR description for reviewer clarity.

5. _get-changed-files.yml — Minor robustness note

The reusable workflow (line 30) fetches files via gh api ... --paginate --jq ... | tr '\n' ' '. For extremely large PRs (1000+ files), the output is a single long string in GITHUB_OUTPUT. GitHub's per-step output limit is 1MB — unlikely to be hit but worth a mental note.

6. PR description vs. actual changes

The PR description mentions changes to _unittest.yml, metal.yml, and mlx.yml, and adding nightly schedules (cron: 0 6, 0 8, 0 4). These don't appear in the current diff (only 3 files changed). Either these were dropped in a later commit or the description needs updating.

Summary

The gating logic is well-designed and correctly handles all trigger types. The contains() prefix-matching approach is a pragmatic choice that errs on the side of running jobs (safe). The Zephyr refactoring is a bonus cost reduction but might warrant a note in the description since it's unrelated to macOS.

No blocking issues found. The PR should meaningfully reduce macOS CI costs on PRs that don't touch Apple-specific code.


@metascroy metascroy marked this pull request as ready for review May 20, 2026 23:20
@metascroy metascroy merged commit 01a4554 into main May 21, 2026
182 of 183 checks passed
@metascroy metascroy deleted the reduce-mac-cost branch May 21, 2026 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants