fix: Windows Vulkan runtime dependencies - #1046
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Windows Vulkan DLL dependency collection and verification for native runtime packages, uses Windows-specific dynamic library loading, validates Vulkan GPU facts, and runs dependency tests in Windows GPU CI. ChangesWindows Vulkan runtime support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PackageScript
participant DependencyResolver
participant RuntimeArtifact
participant ArtifactVerifier
PackageScript->>DependencyResolver: collect DLL dependencies
DependencyResolver->>RuntimeArtifact: copy resolved DLLs
PackageScript->>RuntimeArtifact: write manifest with staged libraries
ArtifactVerifier->>DependencyResolver: verify artifact dependencies
DependencyResolver-->>ArtifactVerifier: return validation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/pr_builds.yml (1)
126-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or wire this duplicate
windows_gpufilter.
changes.outputs.windows_gpuis sourced fromsteps.compute.outputs.windows_gpu_build_required(Line 38), notsteps.filter.outputs.windows_gpu; these entries are currently dead configuration and can drift from.github/actions/compute-changes/action.yml.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr_builds.yml around lines 126 - 129, Remove the duplicate windows_gpu-related path entries from the filter configuration in the workflow, since changes.outputs.windows_gpu uses compute.outputs.windows_gpu_build_required rather than filter.outputs.windows_gpu. Keep the authoritative change detection aligned with the compute-changes action and remove only the dead configuration entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/compute-changes/action.yml:
- Line 291: The Windows GPU routing filters omit changes to the routing action
itself. Update WINDOWS_GPU_INPUTS in .github/actions/compute-changes/action.yml
at lines 291-291 to include that action path, and update both the windows_gpu
and rust filters in .github/workflows/ci.yml at lines 143-146 so the job
condition triggers Windows GPU validation for routing-action changes.
---
Nitpick comments:
In @.github/workflows/pr_builds.yml:
- Around line 126-129: Remove the duplicate windows_gpu-related path entries
from the filter configuration in the workflow, since changes.outputs.windows_gpu
uses compute.outputs.windows_gpu_build_required rather than
filter.outputs.windows_gpu. Keep the authoritative change detection aligned with
the compute-changes action and remove only the dead configuration entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 977af892-437a-4097-aafd-675fe9594d89
📒 Files selected for processing (10)
.github/actions/compute-changes/action.yml.github/workflows/ci.yml.github/workflows/pr_builds.ymlcrates/mesh-llm-system/src/hardware/skippy_devices.rscrates/skippy-ffi/src/dynamic_library.rscrates/skippy-ffi/src/lib.rsscripts/package-native-runtime.shscripts/tests/test_windows_native_runtime_deps.pyscripts/verify-native-runtime-package.shscripts/windows-native-runtime-deps.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/tests/test_windows_native_runtime_deps.py`:
- Around line 109-114: Update the verifier invocation in the test flow to
resolve bash with shutil.which("bash") before calling subprocess.run. Fail
clearly when no executable path is found, and pass the resolved bash path
instead of the literal command name while preserving the existing VERIFY_SCRIPT
and artifact arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ef7b84d-af82-462d-aacd-2939af9e52d1
📒 Files selected for processing (1)
scripts/tests/test_windows_native_runtime_deps.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
992d475 to
2e64d15
Compare
ndizazzo
changed the title
2e64d15 to
5817676
Compare
…ngram * origin/main: Add GLM DSA correctness trace reporting (#1033) Add GLM DSA package contract validation (#1032) Add direct quant layer package tooling (#1031) Lock split topology placement (#1050) fix nightly stability Qwen thinking ui(data-mode): migrate legacy harness storage to v2 (#1040) fix: Windows Vulkan runtime dependencies (#1046) test: validate MI300 runtime selection and placement (#1045) fix: support ROCm RDNA APU kernels safely (#1044) fix: ROCm GPU enumeration (#1039) Document public packaging installation channels Fix Windows autoupdate import (#1030) Update llama.cpp upstream pin
2 participants
Summary
Root cause
The Windows Vulkan artifact manifest listed llama.cpp runtime DLLs but omitted transitive MinGW dependencies imported by ggml-vulkan.dll. Windows also loaded the primary DLL without searching its installed runtime directory for sibling dependencies, so startup failed before Vulkan device enumeration and the system reported zero GPUs.
Validation
Stacked on #1039 as requested.
Closes #1022
Summary by CodeRabbit