[CI] Promote NVIDIA DirectX target out of experimental#1187
Conversation
Both AMD and NVIDIA DirectX configurations have been stable and have higher pass rates than the existing Tier 1 Intel target. Promote them to Tier 1 so they run on every PR. Qualcomm and the Vulkan IHV configurations remain experimental and continue to require the 'test-all' label. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
alsepkow
force-pushed
the
promote-dx-targets
branch
from
af63cf9 to
cab55cf
Compare
Match the tier change in docs/CI.md and pr-matrix.yaml so the README status table reflects that these targets now run on every PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thank you Alex! |
| matrix: | ||
| SKU: [windows-intel] | ||
| TestTarget: [check-hlsl-d3d12, check-hlsl-vk, check-hlsl-clang-d3d12, check-hlsl-clang-vk] | ||
| include: |
There was a problem hiding this comment.
nit:
You might consider something like this:
matrix:
SKU: [windows-intel, windows-amd, windows-nvidia]
TestTarget: [check-hlsl-d3d12, check-hlsl-vk, check-hlsl-clang-d3d12, check-hlsl-clang-vk]
exclude:
- { SKU: windows-amd, TestTarget: check-hlsl-vk }
- { SKU: windows-amd, TestTarget: check-hlsl-clang-vk }
- { SKU: windows-nvidia, TestTarget: check-hlsl-vk }
- { SKU: windows-nvidia, TestTarget: check-hlsl-clang-vk }
so that as future test targets get promoted, we can just remove exclusions, rather than needing to add specific inclusions. Something about this seems cleaner to me.
There was a problem hiding this comment.
This does seem cleaner to me assuming the expected state would eventually be that we don't have exclusions.
Per Bob's review feedback, switch from listing AMD/NVIDIA D3D12 combinations via 'include' to a cross-product with 'exclude' for the AMD/NVIDIA Vulkan combinations. As future targets get promoted out of experimental, we can simply remove exclusions rather than adding inclusions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Apply the same cross-product + exclude pattern to the experimental Exec-Tests-Extra job for consistency. As targets are promoted out of experimental, exclusions can be added here in lockstep with their removal from the Tier 1 Exec-Tests-Windows job. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This reverts commit 1eec3eb. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
AMD's check-hlsl-d3d12 job uncovered AMD UMD crashes (0xC0000005 in amdxc64.dll) on a small set of CBuffer/matrix tests when running with the default RelWithDebInfo build used by pr-matrix.yaml. The scheduled Debug-build job sees fewer of these failures, so the AMD pass is not yet stable enough for Tier 1. Move AMD's D3D12 entries back into Exec-Tests-Extra (gated on the test-all label) and revert the AMD rows in docs/CI.md and README.md. NVIDIA DirectX remains promoted to Tier 1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| matrix: | ||
| SKU: [windows-nvidia, windows-amd, windows-qc] | ||
| TestTarget: [check-hlsl-d3d12, check-hlsl-vk, check-hlsl-clang-d3d12, check-hlsl-clang-vk] | ||
| include: |
There was a problem hiding this comment.
Why has this been expanded out, rather than using the matrix and exclusions approach used above? Presumably it'd be preferable to use the matrix/exclude pattern?
There was a problem hiding this comment.
Came up in discussion with @bob80905 that was that the includes option makes for cleaner looking diffs if we're doing partial promotions. And that partial promotions have typically been the case.
If that weren't the case than sticking with excludes makes more sense. I'm indifferent. Do we have a pattern we typically follow?
There was a problem hiding this comment.
Previously we seemed to use the matrix and have excludes. The matrix + excludes IMO specifies intent much more clearly. I need to reconstruct the matrix to understand what's going on with the explicit include list.
damyanp
left a comment
damyanp
left a comment
There was a problem hiding this comment.
I don't think that the PR description matches what the PR actually does?
alsepkow
changed the title
Convert Exec-Tests-Extra from an explicit `include:` list to the same cross-product + `exclude:` shape used by Exec-Tests-Windows. The set of combinations that run is unchanged (AMD x all 4 TestTargets, NVIDIA x Vulkan only, QC x all 4 TestTargets); only the YAML shape changes. Per review feedback, this keeps the two matrices internally consistent and matches the cross-product shape both jobs used prior to this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bogner
left a comment
bogner
left a comment
There was a problem hiding this comment.
LGTM. For the commit message/PR description I don't think the recent pass rates, the fact that we were considering AMD as well here, or the list of what the mechanical changes are are useful. IMO we just need the blurb at the beginning to justify this change and provide sufficient historical context:
NVIDIA's DirectX configuration has been stable and has a higher pass rate
than the existing Tier 1 Intel target. Promote it to Tier 1 so it runs on
every PR. AMD, Qualcomm, and the Vulkan IHV configurations remain
experimental and continue to require the test-all label.
5 participants
NVIDIA's DirectX configuration has been stable and has a higher pass rate
than the existing Tier 1 Intel target. Promote it to Tier 1 so it runs on
every PR. AMD, Qualcomm, and the Vulkan IHV configurations remain
experimental and continue to require the
test-alllabel.Recent pass rates:
AMD was originally part of this promotion but is being excluded for now
while we debug some intermittent bugs that were noticed.
Changes
docs/CI.md: PromoteWindows NVIDIA GPU DirectXfrom Experimental toTier 1.
README.md: Move the NVIDIA DirectX row from Experimental Targets toTier 1 Targets in the status table.
.github/workflows/pr-matrix.yaml: Addwindows-nvidiato the existingExec-Tests-Windowsmatrix (excluding the still-experimental Vulkancombinations).
Exec-Tests-Extra(gated ontest-all) continues tocover AMD D3D12, AMD/NVIDIA Vulkan, and all QC targets.
Assisted by Claude Opus 4.7