feat: introduce GitHub Actions workflow for PR testing#2326
feat: introduce GitHub Actions workflow for PR testing#2326yzh119 merged 24 commits intoflashinfer-ai:mainfrom
Conversation
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
📝 WalkthroughWalkthroughAdds a new GitHub Actions workflow Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger as GitHub Trigger
participant Setup as Setup Job
participant AOT as AOT Build Jobs
participant GPU_A10 as GPU JIT A10G Jobs
participant GPU_T4 as GPU JIT T4 Job
participant Summary as CI Summary Job
Trigger->>Setup: start workflow (push/PR/dispatch)
Setup->>Setup: read `ci/docker-tags.yml`, compute outputs (`docker_tag`, `skip_build`)
alt AOT not skipped
Setup->>AOT: start matrix (x86_64/aarch64 × CUDA versions)
AOT-->>Setup: return results/artifacts
end
alt GPU not skipped
Setup->>GPU_A10: dispatch 5 shards (A10G, SM86)
Setup->>GPU_T4: dispatch 1 shard (T4, SM75)
GPU_A10-->>Setup: shard results
GPU_T4-->>Setup: result
end
Setup->>Summary: provide outputs/status
AOT->>Summary: report status
GPU_A10->>Summary: report status
GPU_T4->>Summary: report status
Summary->>Summary: aggregate results, write GitHub Step Summary, set final workflow status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/pr-test.yml:
- Around line 64-65: The TODO indicates .github/ was temporarily removed from
SKIP_PATTERNS; update the SKIP_PATTERNS declaration: either restore the
^\.github/ pattern into the SKIP_PATTERNS value (so that files under .github are
skipped) or remove the TODO comment and add a short inline comment documenting
the chosen final policy for .github/* (state whether CI should include or
exclude it), ensuring you modify the SKIP_PATTERNS variable and the accompanying
comment consistently.
- Around line 227-235: The summary currently treats only "failure" as a failing
job; update the final conditional that checks needs.aot-build-import.result,
needs.gpu-tests-a10g.result, and needs.gpu-tests-t4.result so any result not
equal to "success" (e.g., "cancelled" or "skipped") is considered a failure
unless an explicit skip flag is set; change the checks from == "failure" to !=
"success" and add exceptions for the explicit skip markers (e.g., skip_aot,
skip_gpu, skip_build or equivalent inputs) so that skipped jobs caused by those
flags do not cause the summary to fail.
| # TODO (yongwww): Add back ^\.github/ before merging to main | ||
| SKIP_PATTERNS="README.md|^docs/|^docker/|^licenses/|^LICENSE$|^NOTICE$|^version\.txt$" |
There was a problem hiding this comment.
Resolve the TODO before merge.
Line 64 notes that .github/ should be re-added to SKIP_PATTERNS. Please either restore it now (if intended) or remove the TODO and document the final policy to avoid forgetting.
🤖 Prompt for AI Agents
In @.github/workflows/pr-test.yml around lines 64 - 65, The TODO indicates
.github/ was temporarily removed from SKIP_PATTERNS; update the SKIP_PATTERNS
declaration: either restore the ^\.github/ pattern into the SKIP_PATTERNS value
(so that files under .github are skipped) or remove the TODO comment and add a
short inline comment documenting the chosen final policy for .github/* (state
whether CI should include or exclude it), ensuring you modify the SKIP_PATTERNS
variable and the accompanying comment consistently.
There was a problem hiding this comment.
let's keep the todo here, because we are testing the workflow. We can remove it in follow-up prs
There was a problem hiding this comment.
@yongwww, sounds good! That makes sense while you're testing the workflow.
Would you like me to create a follow-up issue to track removing the TODO and finalizing the SKIP_PATTERNS policy for .github/ files? This way it won't be forgotten once testing is complete.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/pr-test.yml:
- Around line 48-53: The "Get Docker Tag" step (id: get-tag) currently extracts
TAG from ci/docker-tags.yml but doesn't validate results; update the step to
first check that ci/docker-tags.yml exists and is readable, then run the
grep/cut pipeline to set TAG, and after extraction verify TAG is non-empty and
matches the expected pattern; if the file is missing or TAG is empty/invalid,
print a clear error message (including the file/path and pattern) and exit
non‑zero so the workflow fails fast instead of propagating an empty TAG to
downstream jobs.
♻️ Duplicate comments (1)
.github/workflows/pr-test.yml (1)
64-65: Acknowledged: TODO retained for workflow testing.Per prior discussion, the TODO is intentionally kept while testing the workflow. Consider tracking this in an issue so it's not forgotten post-testing.
🧹 Nitpick comments (1)
.github/workflows/pr-test.yml (1)
88-92: Custom runner labels are expected for self-hosted runners.The actionlint warnings about
cpu,X64,ARM64labels are false positives. These are custom labels for your self-hosted runner fleet. Consider adding anactionlint.yamlconfig to suppress these warnings:# actionlint.yaml self-hosted-runner: labels: - cpu - gpu - sm75 - sm86 - X64 - ARM64
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/pr-test.yml:
- Around line 58-63: Replace the unreliable git diff that uses origin/${{
github.base_ref }}...HEAD when computing CHANGED; update the CHANGED computation
to use the PR event SHAs (github.event.pull_request.base.sha and
github.event.pull_request.head.sha) for the diff range so the workflow uses the
pull_request base and head commit SHAs instead of relying on origin refs, or
alternatively explicitly fetch the base branch before diffing if you need to
compare against the up-to-date base branch; adjust the CHANGED assignment (the
git diff invocation) accordingly to reference these PR SHAs.
♻️ Duplicate comments (1)
.github/workflows/pr-test.yml (1)
48-53: Validate Docker tag extraction to avoid empty image tags.
If the pattern misses or the file is missing,TAGbecomes empty and later Docker pulls fail with unclear errors—fail fast with a guard.🛠️ Suggested hardening
- name: Get Docker Tag id: get-tag run: | - TAG=$(grep 'flashinfer/flashinfer-ci-cu129:' ci/docker-tags.yml | cut -d':' -f2 | tr -d ' ') + if [ ! -f ci/docker-tags.yml ]; then + echo "::error::Missing ci/docker-tags.yml" + exit 1 + fi + TAG=$(grep 'flashinfer/flashinfer-ci-cu129:' ci/docker-tags.yml | cut -d':' -f2 | tr -d ' ') + if [ -z "$TAG" ]; then + echo "::error::Failed to extract Docker tag from ci/docker-tags.yml" + exit 1 + fi echo "tag=$TAG" >> $GITHUB_OUTPUT echo "Docker tag: $TAG"
🧹 Nitpick comments (1)
.github/workflows/pr-test.yml (1)
7-11: Verify self‑hosted runner isolation for PRs.
pull_requestexecutes untrusted code on self‑hosted runners; ensure runners are ephemeral/isolated and have no secrets or broad network access, or restrict PR runs to trusted contributors.Also applies to: 88-92
| HEAD_SHA="${{ github.event.pull_request.head.sha }}" | ||
| CHANGED=$(git diff --name-only "$BASE_SHA...$HEAD_SHA") | ||
| # TODO (yongwww): Add back ^\.github/ before merging to main | ||
| SKIP_PATTERNS="README.md|^docs/|^docker/|^licenses/|^LICENSE$|^NOTICE$|^version\.txt$" |
There was a problem hiding this comment.
We should use a script file to determine skip patterns (under scripts/).
📌 Description
#2355
🔍 Related Issues
🚀 Pull Request Checklist
Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).Co-Authored-By: Kevin Vincent kvincent@nvidia.com
Reviewer Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.