Skip to content

fix: vllm e2e job uses matrix context without strategy block - #750

Closed
andrewwhitecdw wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
andrewwhitecdw:bugfix/cicd-main-vllm-e2e-job-uses-matrix-context
Closed

andrewwhitecdw wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
andrewwhitecdw:bugfix/cicd-main-vllm-e2e-job-uses-matrix-context

Conversation

@andrewwhitecdw

Copy link
Copy Markdown
Contributor

This PR addresses the following issue in .github/workflows/cicd-main.yml: vllm e2e job uses matrix context without strategy block.

Changes

  • .github/workflows/cicd-main.yml: vllm e2e job uses matrix context without strategy block.

Details

--- a/.github/workflows/cicd-main.yml
+++ b/.github/workflows/cicd-main.yml
@@ -1,19 +1,37 @@
-  cicd-e2e-tests-trt-onnx:
-    needs: [pre-flight, cicd-unit-tests-vllm]
-    runs-on: ${{ needs.pre-flight.outputs.runner_prefix }}
-    name: ${{ matrix.is_optional && 'PLEASEFIXME_' || '' }}${{ matrix.script }}
-...
-          script: L2_ONNX_TRT
-          is_optional: ${{ matrix.is_optional || false }}
-
-  cicd-e2e-tests-vllm:
-    needs: [cicd-unit-tests-vllm, pre-flight]
-...
-          script: L2_Launch_vLLM
-          is_optional: ${{ matrix.is_optional || false }}
-
-  cicd-e2e-tests-inframework:
-    needs: [pre-flight, cicd-unit-tests-vllm]
-...
-          script: L2_Launch_InFramework
-          is_optional: ${{ matrix.is_optional || false }}
+  cicd-e2e-tests-trt-onnx:
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          - script: L2_ONNX_TRT
+            is_optional: false
+    needs: [pre-flight, cicd-unit-tests-vllm]
+    runs-on: ${{ needs.pre-flight.outputs.runner_prefix }}
+    name: ${{ matrix.is_optional && 'PLEASEFIXME_' || '' }}${{ matrix.script }}
+...
+          script: ${{ matrix.script }}
+          is_optional: ${{ matrix.is_optional }}
+
+  cicd-e2e-tests-vllm:
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          - script: L2_Launch_vLLM
+            is_optional: false
+    needs: [cicd-unit-tests-vllm, pre-flight]
+...
+          script: ${{ matrix.script }}
+          is_optional: ${{ matrix.is_optional }}
+
+  cicd-e2e-tests-inframework:
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          - script: L2_Launch_InFramework
+            is_optional: false
+    needs: [pre-flight, cicd-unit-tests-vllm]
+...
+          script: ${{ matrix.script }}
+          is_optional: ${{ matrix.is_optional }}

Tests

Let me know if you want tests added for this fix or not.

Contributor guidelines

Per this repo's CONTRIBUTING.md:

  • All commits are signed off (Signed-off-by trailer, DCO).

Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
@andrewwhitecdw
andrewwhitecdw requested a review from a team as a code owner August 17, 2026 20:19
@copy-pr-bot

copy-pr-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the CI label Aug 17, 2026
@chtruong814

Copy link
Copy Markdown
Contributor

/ok to test 57c3538

@chtruong814

Copy link
Copy Markdown
Contributor

@andrewwhitecdw Thanks for the PR for cleaning this up. Could you actually combine your changes from these other PRs? It'll be faster for us to review together since it's CI related.

@chtruong814 chtruong814 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the comment about aggregating the other CI proposed changes into this PR.

@andrewwhitecdw

Copy link
Copy Markdown
Contributor Author

Consolidating these related CI fixes into a single PR: #755. Thanks for the suggestion!

andrewwhitecdw added a commit to andrewwhitecdw/Export-Deploy that referenced this pull request Aug 31, 2026
This combines three related CI fixes so they can be reviewed together:

- `.github/actions/test-template/action.yml`: corrects the `is_optional`
  description so it matches the actual behavior.
- `.github/workflows/cicd-main.yml`: adds missing `strategy`/`matrix` blocks to
  the trt-onnx, vllm, and inframework e2e jobs, and uses `${{ matrix.script }}`
  and `${{ matrix.is_optional }}` instead of hard-coded values.

Tests
- `unit_tests/github_actions/test_optional_description.py`: verifies the
  `is_optional` description no longer inverts the behavior.
- `tests/test_cicd_yaml.py`: verifies the e2e jobs define a strategy block
  before referencing matrix context.

Closes NVIDIA-NeMo#728, NVIDIA-NeMo#749, NVIDIA-NeMo#750.

Signed-off-by: Andrew White <andrewwhitecdw@users.noreply.github.com>
andrewwhitecdw added a commit to andrewwhitecdw/Export-Deploy that referenced this pull request Aug 31, 2026
This combines three related CI fixes so they can be reviewed together:

- `.github/actions/test-template/action.yml`: corrects the `is_optional`
  description so it matches the actual behavior.
- `.github/workflows/cicd-main.yml`: adds missing `strategy`/`matrix` blocks to
  the trt-onnx, vllm, and inframework e2e jobs, and uses `${{ matrix.script }}`
  and `${{ matrix.is_optional }}` instead of hard-coded values.

Tests
- `unit_tests/github_actions/test_optional_description.py`: verifies the
  `is_optional` description no longer inverts the behavior.
- `tests/test_cicd_yaml.py`: verifies the e2e jobs define a strategy block
  before referencing matrix context.

Closes NVIDIA-NeMo#728, NVIDIA-NeMo#749, NVIDIA-NeMo#750.

Signed-off-by: Andrew White <andrewwhitecdw@users.noreply.github.com>

This branch was previously deployed

3 inactive deployments
public — 57c35388 Deployed Aug 24, 2026 by copy-pr-bot[bot] via release / finalize / publish-docs #290
nemo-ci — 57c35388 Deployed Aug 23, 2026 by copy-pr-bot[bot] via L2_Launch_vLLM #3025
test — 57c35388 Deployed Aug 23, 2026 by copy-pr-bot[bot] via cicd-wait-in-queue #3025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants