Repository navigation
ci: skip irrelevant E2E jobs on PRs with file changes detection - #633
Conversation
…runner Signed-off-by: Chang Su <chang.s.su@oracle.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a CI bottleneck caused by irrelevant E2E tests running on self-hosted runners. It introduces path-based conditional job execution to skip GPU-intensive tests when changes are not relevant to those subsystems. Additionally, it centralizes shared E2E steps into a composite action and moves lightweight jobs off self-hosted runners, significantly improving CI efficiency and reducing resource consumption. Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a reusable GitHub Actions composite action to orchestrate configurable E2E runs, refactors the PR Rust CI workflow to a path-gated multi-stage E2E pipeline using detect-changes, and adds concurrency controls to the labeler workflow. Changes
Sequence DiagramsequenceDiagram
participant PR as Pull Request
participant Detect as detect-changes
participant E2E_A as e2e-always
participant E2E_C as e2e-chat-completions
participant E2E_G as e2e-agentic
participant RunE2E as run-e2e Action
participant Backend as Backend Setup (vLLM/TRT-LLM/SGLang)
participant Oracle as Oracle/Services
participant Tests as Test Runner
participant Results as Results Aggregator
PR->>Detect: trigger workflow
Detect->>Detect: classify changed paths (common/chat/agentic)
Detect-->>E2E_A: always run
Detect-->>E2E_C: run if chat changes
Detect-->>E2E_G: run if agentic changes
E2E_A->>RunE2E: invoke with matrix
E2E_C->>RunE2E: invoke with matrix
E2E_G->>RunE2E: invoke with matrix
RunE2E->>Backend: setup selected backend
RunE2E->>Oracle: start/prepare services
RunE2E->>Tests: run tests with env and filters
Tests-->>RunE2E: return results
RunE2E-->>Results: publish/aggregate results
Results->>PR: update status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a composite action to reduce duplication in the E2E workflows, which is a great improvement for maintainability. However, the new composite action .github/actions/run-e2e/action.yml contains critical command injection vulnerabilities. Direct substitution of user-provided inputs into shell commands allows an attacker to execute arbitrary commands on the CI runner, potentially leading to the exfiltration of sensitive secrets. It is recommended to refactor these steps to use environment variables for secure input handling. Furthermore, the action uses invalid versions of standard GitHub Actions (actions/checkout, actions/download-artifact, actions/upload-artifact), which will cause CI failures, and there is a minor typo in a script argument. None of the provided rules contradicted or applied to these comments, so all original comments have been retained.
Signed-off-by: Chang Su <chang.s.su@oracle.com>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/run-e2e/action.yml:
- Around line 65-83: The workflow uses actions/download-artifact@v8 in the steps
named "Download wheel artifact", "Download WASM test fixtures", and "Download
Python client types"; change each usage reference from
actions/download-artifact@v8 to actions/download-artifact@v7 so the steps point
to the real existing release (v7) and the workflow no longer fails due to a
non-existent v8 tag.
In @.github/workflows/pr-test-rust.yml:
- Around line 369-388: The workflow passes inputs like setup_trtllm and
setup_agentic_deps to the composite action run-e2e but those matrix keys are not
defined for the e2e-always matrix, causing linter warnings; either add explicit
matrix defaults for setup_trtllm and setup_agentic_deps (e.g., set them to
'false' in the matrix entries) or change the step to only pass those inputs when
defined using conditional expressions (wrap the input assignments for
setup_trtllm and setup_agentic_deps in an if/conditional so they are omitted
when the matrix lacks them); update the run-e2e step where setup_trtllm and
setup_agentic_deps are referenced to implement one of these fixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 508b90a3-9eb7-40e9-8e1c-f690ae11ae2a
📒 Files selected for processing (3)
.github/actions/run-e2e/action.yml.github/workflows/labeler.yml.github/workflows/pr-test-rust.yml
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afb5c032e4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-test-rust.yml:
- Around line 484-485: Add a checkout step before invoking the local composite
action used in the workflow: insert an actions/checkout@v6 step immediately
prior to the step that uses ./.github/actions/run-e2e so the repository files
are available to the composite action; ensure the new step appears in the same
job's steps array before the uses: ./.github/actions/run-e2e entry.
- Around line 439-440: The workflow is using the local composite action
reference "./.github/actions/run-e2e" without first checking out the repository;
add a step that runs the checkout action (e.g., uses: actions/checkout@v4)
immediately before the step that says "uses: ./.github/actions/run-e2e" so the
local action files are available to the runner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d3c8f26-a3f9-44a0-97f8-b516fd8d0ebe
📒 Files selected for processing (2)
.github/workflows/labeler.yml.github/workflows/pr-test-rust.yml
…and CI action filter Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d572b665f1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-test-rust.yml:
- Around line 300-310: The common change-detection filter currently omits the
workflow file, so workflow-only PRs skip e2e-chat-completions and e2e-agentic;
update the common filter (the YAML mapping under "filters: | common:") to add
the workflow path '.github/workflows/pr-test-rust.yml' to the list so changes to
that workflow trigger the full E2E suite (ensuring e2e-chat-completions and
e2e-agentic run for workflow-only PRs).
- Line 287: The detect-changes job is gated by "if: github.event_name ==
'pull_request'", causing downstream jobs that declare needs: [detect-changes]
(e.g., e2e-chat-completions, e2e-agentic) to be skipped on non-PR events; remove
that restrictive if so detect-changes always runs (or change it to a broader
condition) and ensure any references to needs.detect-changes.outputs.* in
e2e-chat-completions and e2e-agentic are still valid (or guarded) so they don't
read undefined outputs when detect-changes is skipped. Target the detect-changes
job definition and update its if condition (or remove it), and adjust the
dependent jobs' usage of needs.detect-changes.outputs to handle absence or rely
only on build-wheel if you choose to make detect-changes optional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e85fd58-bcfa-4872-bfe4-cf7422b740b6
📒 Files selected for processing (1)
.github/workflows/pr-test-rust.yml
Signed-off-by: Chang Su <chang.s.su@oracle.com>
…e to common filter Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/pr-test-rust.yml (2)
405-411:⚠️ Potential issue | 🟠 MajorJobs with
needs: detect-changesmay be skipped unintentionally on non-PR events.When
detect-changesis skipped (on push/workflow_dispatch events), GitHub Actions skips dependent jobs by default. Using!cancelled()doesn't prevent this —always()is required to evaluate the job'sifcondition regardless of dependency status.The intent is for
github.event_name != 'pull_request'to run these jobs on non-PR events, but the job is skipped before this condition is evaluated.🔧 Proposed fix
e2e-chat-completions: name: ${{ matrix.name }} needs: [build-wheel, detect-changes] if: >- - !cancelled() + always() + && !cancelled() && needs.build-wheel.result == 'success' && github.actor != 'dependabot[bot]' && (github.event_name != 'pull_request' - || needs.detect-changes.outputs.common == 'true' - || needs.detect-changes.outputs.chat-completions == 'true') + || (needs.detect-changes.result == 'success' + && (needs.detect-changes.outputs.common == 'true' + || needs.detect-changes.outputs.chat-completions == 'true')))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-test-rust.yml around lines 405 - 411, The job's if-condition can be skipped when the needs job (detect-changes) is skipped; wrap the whole boolean expression with always() so the condition is evaluated even if dependencies were skipped. Update the existing if expression (the multi-line condition starting with !cancelled() && needs.build-wheel.result == 'success' && ...) to use always(), e.g. replace the top-level expression with always() && (existing expression) so the github.event_name != 'pull_request' branch runs on non-PR events as intended.
477-483:⚠️ Potential issue | 🟠 MajorSame skipping issue applies to e2e-agentic.
Identical to
e2e-chat-completions— this job will be skipped on non-PR events before itsifcondition can be evaluated.🔧 Proposed fix
e2e-agentic: name: ${{ matrix.name }} needs: [build-wheel, detect-changes] if: >- - !cancelled() + always() + && !cancelled() && needs.build-wheel.result == 'success' && github.actor != 'dependabot[bot]' && (github.event_name != 'pull_request' - || needs.detect-changes.outputs.common == 'true' - || needs.detect-changes.outputs.agentic == 'true') + || (needs.detect-changes.result == 'success' + && (needs.detect-changes.outputs.common == 'true' + || needs.detect-changes.outputs.agentic == 'true')))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-test-rust.yml around lines 477 - 483, The e2e-agentic job's if uses needs.detect-changes.outputs (needs.detect-changes) which isn't available on non-PR events so the job is skipped before the condition runs; update the if to avoid referencing detect-changes outputs for non-PR events by restructuring it to allow non-PR runs (e.g. keep the existing guards !cancelled(), needs.build-wheel.result == 'success', github.actor != 'dependabot[bot]' and replace the PR check with: (github.event_name != 'pull_request') || (github.event_name == 'pull_request' && (needs.detect-changes.outputs.common == 'true' || needs.detect-changes.outputs.agentic == 'true')) so e2e-agentic will run on non-PR events while still gating PR runs on detect-changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/run-e2e/action.yml:
- Around line 88-91: The "Pull genai-bench image" step should validate
inputs.genai_bench_image when inputs.upload_benchmarks == 'true' to avoid
running `docker pull` with an empty string; update the step named "Pull
genai-bench image" to either add a conditional that checks
inputs.genai_bench_image is non-empty or add a pre-run validation that exits
with a clear error when inputs.upload_bench_image is 'true' and
inputs.genai_bench_image is empty (so the action fails fast with a helpful
message instead of running `docker pull` with an empty argument).
---
Duplicate comments:
In @.github/workflows/pr-test-rust.yml:
- Around line 405-411: The job's if-condition can be skipped when the needs job
(detect-changes) is skipped; wrap the whole boolean expression with always() so
the condition is evaluated even if dependencies were skipped. Update the
existing if expression (the multi-line condition starting with !cancelled() &&
needs.build-wheel.result == 'success' && ...) to use always(), e.g. replace the
top-level expression with always() && (existing expression) so the
github.event_name != 'pull_request' branch runs on non-PR events as intended.
- Around line 477-483: The e2e-agentic job's if uses
needs.detect-changes.outputs (needs.detect-changes) which isn't available on
non-PR events so the job is skipped before the condition runs; update the if to
avoid referencing detect-changes outputs for non-PR events by restructuring it
to allow non-PR runs (e.g. keep the existing guards !cancelled(),
needs.build-wheel.result == 'success', github.actor != 'dependabot[bot]' and
replace the PR check with: (github.event_name != 'pull_request') ||
(github.event_name == 'pull_request' && (needs.detect-changes.outputs.common ==
'true' || needs.detect-changes.outputs.agentic == 'true')) so e2e-agentic will
run on non-PR events while still gating PR runs on detect-changes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8efd28b-fedb-448d-b4b3-5797f307fb0d
📒 Files selected for processing (2)
.github/actions/run-e2e/action.yml.github/workflows/pr-test-rust.yml
| upload_benchmarks: 'false' | ||
| parallel_opts: "--workers 1 --tests-per-worker 4" # Thread-based parallelism | ||
| ignore_opts: "" | ||
| - name: vllm-pd |
There was a problem hiding this comment.
agentic related changes may also not require vllm-pd tests and benchmark tests?
There was a problem hiding this comment.
I thought about it.
The vllm-pd test (e2e_test/router/test_pd_mmlu.py) exercises PD (Prefill-Decode) disaggregated routing. The relevant source code is spread across:
- model_gateway/src/routers/http/pd_router.rs, pd_types.rs — HTTP PD router
- model_gateway/src/routers/grpc/pd_router.rs — gRPC PD router
- model_gateway/src/routers/grpc/common/stages/request_execution.rs — shared PD execution
- model_gateway/src/routers/factory.rs — router factory
- grpc_client/** — gRPC client (used by PD gRPC mode)
- protocols/** — worker/generate protos
- e2e_test/router/** — the test files themselves
But since PD routing lives entirely within model_gateway/**, protocols/**, grpc_client/**, and bindings/** — all of which are already in the common filter — any change to PD source code already triggers all conditional jobs via common.
There was a problem hiding this comment.
Updated in 5a03752. Right now vllm-pd is a matrix in e2e-chat-completions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61f2386c95
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-test-rust.yml:
- Line 505: Add an actionlint config to declare the custom self-hosted runner
labels so actionlint stops flagging them as unknown: create an actionlint config
file named .github/actionlint.yaml and list the runner labels "k8s-runner-gpu"
and "k8s-runner-cpu" under the runners (or runner_labels) section per actionlint
schema, ensuring those labels match the ones used in the workflow (e.g.,
occurrences of k8s-runner-gpu and k8s-runner-cpu).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b5a7170-f659-4e5d-830b-00f8ffc00c48
📒 Files selected for processing (1)
.github/workflows/pr-test-rust.yml
Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d236e442d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…m agentic Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
CatherineSue
changed the title
CatherineSue
changed the title
Reverts the composite action approach in favor of inline steps so each step appears as a separate top-level entry in CI logs. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
08a0776 to
3980566
Compare
2 participants
Description
Problem
With 10+ PRs queuing for CI, the self-hosted
k8s-runner-*runners are a bottleneck. Many PRs (e.g., MCP-only, auth-only, wasm-only changes) trigger the full E2E matrix including GPU-heavy chat-completions and agentic-apis tests that are irrelevant.Solution
Use
dorny/paths-filterto detect which subsystems changed and conditionally skip unrelated GPU E2E jobs. Extract shared E2E steps into a composite action to reduce duplication.Changes
cancel-in-progressdetect-changesjob: new lightweight job onubuntu-latestusingdorny/paths-filter@v3with three filter outputs, withpull-requests: readpermissiongateway-e2einto 3 jobs:e2e-always— benchmarks, e2e, vllm-pd (always runs)e2e-chat-completions— chat-completions-sglang/vllm/trtllm (skipped unlesscommonorchat-completionspaths changed)e2e-agentic— agentic-apis (skipped unlesscommonoragenticpaths changed)workflow_dispatchalways run the full matrix viaalways() && !cancelled()escape hatch.github/actions/run-e2e: extracts shared E2E steps (backend setup, artifact download, install, test run, Oracle setup/cleanup, benchmark upload)finishjob: updated needs/failure checks for the 3 new jobs +detect-changessummarize-benchmarks:needs: [gateway-e2e]→needs: [e2e-always]'true'/'false'to avoid actionlint warningsFilter logic
Three filters determine which conditional E2E jobs run. If any path in a filter matches, that filter outputs
true.commonmodel_gateway/**,protocols/**,bindings/**,e2e_test/conftest.py,e2e_test/infra/**,e2e_test/fixtures/**,Cargo.lock,.github/actions/**,.github/workflows/pr-test-rust.yml,scripts/ci_setup_python_venv.sh,scripts/ci_install_sglang.sh,scripts/ci_build_wheel.sh,tokenizer/**,tool_parser/**e2e-chat-completions+e2e-agentic)chat-completionsreasoning_parser/**,multimodal/**,grpc_client/**,e2e_test/chat_completions/**,scripts/ci_install_vllm.sh,scripts/ci_install_trtllm.she2e-chat-completionsonlyagenticmcp/**,data_connector/**,e2e_test/responses/**,e2e_test/messages/**,scripts/ci_agentic_svc_deps.she2e-agenticonlytokenizer/**andtool_parser/**are incommonbecause they're shared by both chat-completions and agentic subsystemsci_install_vllm.sh,ci_install_trtllm.sh) are inchat-completionssince they only affect those backendse2e-alwaysruns unconditionally — no filter gatingkv_index/,wasm/,docs/) only triggere2e-alwaysSavings examples
mcp/kv_index/orwasm/Test Plan
python -c "import yaml; yaml.safe_load(open(...))"validationmcp/— verify chat-completions jobs are skipped but agentic runskv_index/— verify both chat-completions and agentic are skippedChecklist
cargo +nightly fmtpasses (no Rust changes)cargo clippy --all-targets --all-features -- -D warningspasses (no Rust changes)Summary by CodeRabbit