Add AMD GPU Support #1501
Add AMD GPU Support #1501
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis change introduces AMD ROCm support for Windows systems. It adds GPU detection logic for AMD hardware, new AMD-specific package dependencies and overrides, type definitions to support AMD as a GPU option, and installation workflows for AMD ROCm SDK and PyTorch wheels. Documentation is updated to reference compiled requirements headers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Application
participant GPU as GPU Detection
participant Env as Virtual Environment
participant Install as Installation
User->>App: Initialize environment / setup
App->>GPU: validateHardware()
rect rgb(220, 240, 255)
Note over GPU: Windows GPU Detection
GPU->>GPU: Query Windows CIM data
GPU->>GPU: Parse vendor ID from PNPDeviceID
alt AMD Detected (1002)
GPU-->>App: detected: 'amd'
else NVIDIA Detected (10DE)
GPU-->>App: detected: 'nvidia'
else Fallback
GPU->>GPU: Execute nvidia-smi
GPU-->>App: detected GPU or error
end
end
App->>Env: selectDevice(detected)
alt Device is 'amd'
rect rgb(240, 220, 255)
Note over Env: AMD ROCm Setup
Env->>Install: installAmdRocmSdk()
Install->>Install: Build pip install command
Install->>Install: Execute via uv (AMD SDK packages)
Install-->>Env: SDK installed
Env->>Install: installAmdTorch()
Install->>Install: Build pip install command
Install->>Install: Execute via uv (AMD PyTorch wheels)
Install-->>Env: PyTorch installed
end
else Device is 'nvidia'
Env->>Install: installPytorch() [standard NVIDIA path]
Install-->>Env: PyTorch installed
else Device is 'cpu'
Env->>Install: installPytorch() [CPU only]
Install-->>Env: PyTorch installed
end
Env-->>App: Environment ready
App-->>User: Setup complete
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (6)**/*.{ts,mts,cts}📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
tests/**/*.{ts,mts,cts}📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
tests/unit/**/*.test.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/vitest.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/unit/**/*.{test,spec}.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (14)📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:50:25.371ZApplied to files:
📚 Learning: 2025-12-18T19:46:11.878ZApplied to files:
📚 Learning: 2025-11-25T20:49:50.649ZApplied to files:
📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:50:25.371ZApplied to files:
📚 Learning: 2025-11-25T20:49:40.925ZApplied to files:
📚 Learning: 2025-11-25T20:49:50.649ZApplied to files:
📚 Learning: 2025-11-25T20:50:25.371ZApplied to files:
📚 Learning: 2025-11-25T20:50:25.371ZApplied to files:
📚 Learning: 2025-12-18T19:46:11.878ZApplied to files:
📚 Learning: 2025-11-25T20:49:50.649ZApplied to files:
🧬 Code graph analysis (1)tests/unit/utils.test.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (4)
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 |
dosubot
bot
added
size:XL
![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
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
README.mdassets/override_amd.txtassets/requirements/amd_requirements.txtassets/requirements/windows_amd.compiledsrc/constants.tssrc/preload.tssrc/utils.tssrc/virtualEnvironment.tstests/unit/utils.test.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,mts,cts}: Use features available in TypeScript 5.x
Use ESM only (type: module). Avoid CommonJS (require,module.exports)
Target ESNext runtime APIs and syntax. Prefer top-levelawaitonly when it improves clarity
Code must compile withstrictmode andnoImplicitAnyenabled, with zero implicitany
Useunknowninstead ofany. Narrow with type guards.anyis allowed only when interfacing with untyped code and must be localized and justified
Useinterfacefor public object shapes intended for extension/implementation
Usetypefor unions, mapped/conditional types, function types, and when composition is needed
Model domain variants with a discriminant field (e.g.,kind: 'X' | 'Y'). Use exhaustiveswitchstatements
Preferas constobjects and string/number literal unions overenum
Preferreadonlyproperties andReadonlyArray<T>where appropriate. Do not mutate function parameters
PreferT[]overArray<T>for readability
PreferRecord<Key, T>for simple maps; useMap/Setwhen iteration semantics or key types require it
Avoid non-null assertions (!). Use optional chaining, nullish coalescing, and explicit guards
Usesatisfiesto enforce object literal conformance without widening values
Prefer named exports. Default exports are allowed only when a module clearly has a single primary export
Useimport type { X } from '…'for type-only imports. Keep value and type imports separated when helpful for clarity and bundling
Exported functions, classes, and public APIs must have explicit parameter and return types. Internal locals can rely on inference
Prefer arrow functions for local functions and callbacks. Use function declarations when hoisting orthisbinding is required
Modules should be pure. Avoid executing logic at import time unless necessary (e.g., polyfills)
Prefer async/await over rawthen/catchchains. Keep linear flow and usetry/catchfor failures
Eitherawaitpromises or deliberately mark them...
Files:
src/preload.tssrc/constants.tstests/unit/utils.test.tssrc/virtualEnvironment.tssrc/utils.ts
src/**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Centralize reusable domain types in
src/**where discoverable. Avoid ad-hoc inline types for shared structures
Files:
src/preload.tssrc/constants.tssrc/virtualEnvironment.tssrc/utils.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript: Do not use theanytype; useunknownwhen the type is genuinely unknown
Use JSDoc format for method documentation with common tags@paramand@return(omit@returnforvoidreturn type); use{@link }to reference symbols
Files:
src/preload.tssrc/constants.tstests/unit/utils.test.tssrc/virtualEnvironment.tssrc/utils.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESLint and Prettier for code formatting; run
yarn lintandyarn formatbefore committing
Files:
src/preload.tssrc/constants.tstests/unit/utils.test.tssrc/virtualEnvironment.tssrc/utils.ts
{src/main.ts,src/preload.ts,src/main-process/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Main process code must maintain clean separation between main process, renderer, and preload scripts; follow Electron security best practices
Files:
src/preload.ts
src/preload.ts
📄 CodeRabbit inference engine (AGENTS.md)
For preload script at
src/preload.ts, implement secure IPC bridge between main and renderer processes without exposing sensitive APIs
Files:
src/preload.ts
tests/**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Test code may relax some strictness to maximize ergonomics. Keep type escapes localized
Files:
tests/unit/utils.test.ts
tests/unit/**/*.test.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/vitest.mdc)
tests/unit/**/*.test.{js,ts}: Usevitestfor unit testing
Do not build custom testing infrastructure; use Vitest and existing helpers
Store tests in thetests/unit/directory with directory structure matching that of the tested file insrc/
Tests should be cross-platform compatible; ensure proper use ofpath.normalize,path.join, andpath.sepfor Windows, macOS, and Linux compatibility
Mocks should be cleanly written, easy to understand, and reusable where possible
Prefer the use oftest.extendover loose variables in Vitest tests
Usetestinstead ofitfor defining tests
Files:
tests/unit/utils.test.ts
tests/unit/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for creating unit tests; do not attempt to create custom testing infrastructure
Files:
tests/unit/utils.test.ts
🧠 Learnings (13)
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Tests should be cross-platform compatible; ensure proper use of `path.normalize`, `path.join`, and `path.sep` for Windows, macOS, and Linux compatibility
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Mock native dialogs when needed using app.app.evaluate() with electron.dialog overrides
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:49:50.649Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-25T20:49:50.649Z
Learning: Applies to tests/integration/**/*.{ts,js} : Use `path.join()` and `path.sep` to ensure file path handling works consistently across Windows, macOS, and Linux platforms
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Leverage TestEnvironment for state manipulation and error simulation instead of manual setup
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Add screenshots for visual regression testing in integration tests
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Check CI behavior with process.env.CI in integration tests
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Mocks should be cleanly written, easy to understand, and reusable where possible
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Use Playwright + TypeScript for Electron testing in integration tests
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Import fixtures from testExtensions.ts, not raw Playwright
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Use `vitest` for unit testing
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Do not build custom testing infrastructure; use Vitest and existing helpers
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-12-18T19:46:11.878Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T19:46:11.878Z
Learning: Applies to tests/unit/**/*.{test,spec}.ts : Use Vitest for creating unit tests; do not attempt to create custom testing infrastructure
Applied to files:
tests/unit/utils.test.ts
📚 Learning: 2025-11-25T20:49:50.649Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-25T20:49:50.649Z
Learning: Tests should be cross-platform compatible and work on Windows, macOS, and Linux
Applied to files:
tests/unit/utils.test.ts
🧬 Code graph analysis (4)
src/preload.ts (1)
src/main_types.ts (1)
GpuType(8-8)
tests/unit/utils.test.ts (1)
src/utils.ts (1)
validateHardware(240-295)
src/virtualEnvironment.ts (1)
src/constants.ts (2)
AMD_ROCM_SDK_PACKAGES(191-196)AMD_TORCH_PACKAGES(198-202)
src/utils.ts (2)
src/preload.ts (1)
GpuType(21-21)src/constants.ts (2)
NVIDIA_VENDOR_ID(123-123)AMD_VENDOR_ID(122-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-windows-debug
- GitHub Check: Agent
- GitHub Check: build-apple-debug-all / build-macos-debug
- GitHub Check: build-and-test-e2e-windows / integration-windows-test
- GitHub Check: lint-and-format (windows-latest)
🔇 Additional comments (24)
src/preload.ts (1)
21-22: LGTM - Type union correctly expanded to include AMD GPU support.The
GpuTypeunion now properly includes'amd', andTorchDeviceTypeautomatically inherits this addition.README.md (1)
199-206: LGTM - Documentation correctly updated for AMD ROCm support.The README now includes the AMD ROCm requirements compilation command with proper inputs (amd_requirements.txt, override_amd.txt) and output (windows_amd.compiled).
src/constants.ts (2)
191-202: AMD package arrays correctly defined.The
AMD_ROCM_SDK_PACKAGESandAMD_TORCH_PACKAGESarrays contain the correct URLs matching the requirements and override files.
122-123: Vendor ID constants correctly defined with case-insensitive comparison.The PCI vendor IDs are accurate:
1002for AMD and10DEfor NVIDIA. Vendor ID comparisons insrc/utils.tsare properly case-insensitive: thehasPciVendorId()function explicitly normalizes both sides to uppercase before comparison (line 145), andnormalizeVendorId()ensures all vendor IDs are normalized to uppercase prior to comparison with the constants (line 151).tests/unit/utils.test.ts (3)
1-24: Test setup is well-structured and follows Vitest patterns.The mock setup for
node:child_processandsysteminformationis clean and reusable. ThecreateChildProcesshelper appropriately creates a minimal mock object.
56-64: AMD GPU detection test correctly validates the new AMD path.The test properly mocks an AMD GPU with
vendorId: '1002'and verifies thatvalidateHardwarereturns{ isValid: true, gpu: 'amd' }.
66-94: Unsupported GPU test provides good coverage for rejection path.The test correctly simulates:
- An Intel GPU via systeminformation
- Failed
nvidia-smiexecution- WMI query returning Intel vendor ID (
VEN_8086)The error message assertion (
expect.stringContaining('NVIDIA or AMD')) aligns with the updated error messaging insrc/utils.ts.src/virtualEnvironment.ts (3)
10-10: Import correctly adds AMD package constants.
236-238: Compiled requirements selection correctly handles AMD device.The logic properly returns the appropriate compiled requirements file based on the selected device:
'cpu'→'windows_cpu''amd'→'windows_amd'- default (nvidia) →
'windows_nvidia'
609-613: AMD PyTorch installation path correctly short-circuits the standard flow.When the device is
'amd', the method properly delegates to the AMD-specific installation methods and returns early, avoiding the standard CUDA/CPU torch installation path.assets/requirements/windows_amd.compiled (2)
1-4: Auto-generated compiled requirements file is correctly structured.The file header properly documents the
uv pip compilecommand used for generation, matching the documentation in README.md.
230-243: ROCm and torch packages correctly pinned from AMD's repository.The compiled file properly includes:
- ROCm SDK packages from
repo.radeon.com- torch, torchaudio, torchvision wheels with override annotations pointing to the AMD ROCm wheels
Also applies to: 286-306
assets/requirements/amd_requirements.txt (1)
1-5: AMD ROCm SDK package entries are correctly defined.The requirements file properly lists the four ROCm SDK packages (rocm-sdk-core, rocm-sdk-devel, rocm-sdk-libraries-custom, and rocm) with consistent versioning (ROCm 7.1.1, version 0.1.dev0). URLs match those referenced in src/constants.ts and integrate properly with the build pipeline documented in README.md.
assets/override_amd.txt (1)
1-4: LGTM - AMD ROCm PyTorch wheel overrides are correctly configured and verified.All three wheel URLs are accessible and the versions are compatible with ComfyUI requirements. The compiled requirements output confirms torch 2.9.0, torchaudio 2.9.0, and torchvision 0.24.0 from the AMD ROCm repository resolve cleanly with all dependencies (numpy, pillow, etc.) for Python 3.12 on Windows x64.
src/utils.ts (10)
10-13: LGTM!The imports are correctly structured with type-only imports separated and properly using
import typeas per the coding guidelines.
131-135: LGTM!The PowerShell command is well-constructed with appropriate flags (
-NoProfile,-NonInteractive), explicit UTF-8 encoding, and JSON output for reliable parsing. The regex patterns and type alias are correctly defined.
137-146: LGTM!The function correctly extracts and compares PCI vendor IDs with case-insensitive matching and safe optional chaining.
148-152: LGTM!The normalization function properly handles edge cases and ensures consistent uppercase hex format for vendor ID comparison.
154-167: LGTM!The function implements a sensible two-tier detection strategy: first attempting reliable vendor ID matching, then falling back to string-based heuristics for edge cases. The fallback keywords cover common AMD identifiers including 'radeon'.
169-175: LGTM!The function correctly iterates through controllers and returns the first detected supported GPU type, which works well for multi-GPU systems where integrated GPUs (Intel) are skipped.
190-212: LGTM - well-structured WMI detection with robust JSON parsing.The function correctly handles PowerShell's JSON output variations (single string vs array) with proper type narrowing. The
as unknownpattern followed by type guards is the correct approach per coding guidelines.
214-228: LGTM!Clean helper functions that provide a readable abstraction over the generic
hasGpuViaWmifunction.
259-282: LGTM - comprehensive multi-path GPU detection.The cascading detection strategy (systeminformation → WMI NVIDIA → nvidia-smi → WMI AMD) provides thorough coverage with appropriate fallbacks. The prioritization of NVIDIA detection before AMD is reasonable given the ecosystem.
284-287: LGTM!The error message correctly reflects the expanded GPU support (NVIDIA or AMD on Windows).
| /** | ||
| * Detects NVIDIA GPUs on Windows using nvidia-smi. | ||
| * @return `true` if nvidia-smi executes successfully. | ||
| */ | ||
| async function hasNvidiaGpuViaSmi(): Promise<boolean> { | ||
| try { | ||
| await execAsync('nvidia-smi'); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a timeout for robustness.
The function works correctly, but unlike canExecuteShellCommand, it doesn't have a timeout. While nvidia-smi typically returns quickly, a hung process could delay GPU detection. Consider using a timeout pattern similar to canExecuteShellCommand above.
🔎 Suggested improvement with timeout
async function hasNvidiaGpuViaSmi(): Promise<boolean> {
try {
- await execAsync('nvidia-smi');
+ await execAsync('nvidia-smi', { timeout: 5000 });
return true;
} catch {
return false;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Detects NVIDIA GPUs on Windows using nvidia-smi. | |
| * @return `true` if nvidia-smi executes successfully. | |
| */ | |
| async function hasNvidiaGpuViaSmi(): Promise<boolean> { | |
| try { | |
| await execAsync('nvidia-smi'); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| /** | |
| * Detects NVIDIA GPUs on Windows using nvidia-smi. | |
| * @return `true` if nvidia-smi executes successfully. | |
| */ | |
| async function hasNvidiaGpuViaSmi(): Promise<boolean> { | |
| try { | |
| await execAsync('nvidia-smi', { timeout: 5000 }); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/utils.ts around lines 177 to 188, hasNvidiaGpuViaSmi currently awaits
execAsync('nvidia-smi') with no timeout; modify it to use the same timeout
pattern as canExecuteShellCommand by racing the execAsync call against a timeout
Promise (e.g., reject after configurable milliseconds), and return false on
timeout or error; ensure the timeout path cleans up/kills the spawned process if
possible and preserves the original boolean return semantics (true only on
successful exec before timeout, false otherwise).
| /** | ||
| * Installs AMD ROCm SDK packages on Windows. | ||
| * @param callbacks The callbacks to use for the command. | ||
| */ | ||
| private async installAmdRocmSdk(callbacks?: ProcessCallbacks): Promise<void> { | ||
| if (process.platform !== 'win32') { | ||
| throw new Error('AMD ROCm packages are currently supported only on Windows.'); | ||
| } | ||
|
|
||
| const installArgs = getPipInstallArgs({ | ||
| packages: AMD_ROCM_SDK_PACKAGES, | ||
| }); | ||
|
|
||
| log.info('Installing AMD ROCm SDK packages.'); | ||
| const { exitCode } = await this.runUvCommandAsync(installArgs, callbacks); | ||
| if (exitCode !== 0) { | ||
| throw new Error(`Failed to install AMD ROCm SDK packages: exit code ${exitCode}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Installs AMD ROCm PyTorch wheels on Windows. | ||
| * @param callbacks The callbacks to use for the command. | ||
| */ | ||
| private async installAmdTorch(callbacks?: ProcessCallbacks): Promise<void> { | ||
| if (process.platform !== 'win32') { | ||
| throw new Error('AMD ROCm packages are currently supported only on Windows.'); | ||
| } | ||
|
|
||
| const installArgs = getPipInstallArgs({ | ||
| packages: AMD_TORCH_PACKAGES, | ||
| }); | ||
|
|
||
| log.info('Installing AMD ROCm PyTorch packages.'); | ||
| const { exitCode } = await this.runUvCommandAsync(installArgs, callbacks); | ||
| if (exitCode !== 0) { | ||
| throw new Error(`Failed to install AMD ROCm PyTorch packages: exit code ${exitCode}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
AMD ROCm installation methods are well-implemented with proper platform guards.
Both installAmdRocmSdk and installAmdTorch:
- Validate Windows platform with clear error messages
- Use the shared
getPipInstallArgshelper - Log installation progress
- Throw descriptive errors on failure
Consider adding JSDoc documentation to these private methods for consistency with other methods in the class:
🔎 Suggested documentation
The methods already have minimal JSDoc - this is sufficient for private methods.
🤖 Prompt for AI Agents
In src/virtualEnvironment.ts around lines 632 to 670 the reviewer noted the two
private methods already have minimal JSDoc and no functional changes are
required; if you want consistency with the rest of the class, add fuller JSDoc
for each method by expanding the existing comment to include @param callbacks -
ProcessCallbacks | undefined and @returns Promise<void>, keeping the same brief
description, and ensure wording matches other private method comments in this
file.
There was a problem hiding this comment.
Pull request overview
This PR adds AMD GPU support for Windows, enabling ComfyUI Desktop to run on AMD Radeon GPUs using ROCm. Previously, only NVIDIA GPUs and Apple Silicon were supported on Windows and macOS respectively.
- Added AMD GPU detection logic using vendor IDs and WMI queries
- Implemented AMD-specific PyTorch installation using ROCm SDK and custom wheels
- Updated hardware validation to accept AMD GPUs alongside NVIDIA GPUs on Windows
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/preload.ts | Added 'amd' to the GpuType union type |
| src/constants.ts | Added AMD and NVIDIA vendor ID constants, and AMD ROCm package URLs |
| src/utils.ts | Enhanced Windows GPU detection to identify AMD GPUs via vendor IDs and WMI, updated validation logic and error messages |
| src/virtualEnvironment.ts | Added AMD device handling in compiled requirements selection, implemented AMD ROCm SDK and PyTorch installation methods |
| tests/unit/utils.test.ts | Updated tests to verify AMD GPU detection and adjusted unsupported GPU test to reflect new error message |
| assets/requirements/windows_amd.compiled | Added compiled requirements file for AMD ROCm on Windows |
| assets/requirements/amd_requirements.txt | Added AMD ROCm SDK package requirements |
| assets/override_amd.txt | Added AMD-specific PyTorch wheel overrides |
| README.md | Added documentation for generating AMD ROCm requirements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import path from 'node:path'; | ||
|
|
||
| import { InstallStage, TorchMirrorUrl } from './constants'; | ||
| import { AMD_ROCM_SDK_PACKAGES, AMD_TORCH_PACKAGES, InstallStage, TorchMirrorUrl } from './constants'; |
There was a problem hiding this comment.
The fixDeviceMirrorMismatch function at lines 96-102 doesn't handle the 'amd' device type. When an AMD device is selected and the mirror is set to TorchMirrorUrl.Default, the function doesn't remap it to an appropriate mirror. Since AMD installations use custom PyTorch wheels from AMD's ROCm repository, this function should either handle the 'amd' case explicitly or document why it's not needed. Consider adding: else if (device === 'amd') return undefined; or returning TorchMirrorUrl.Default, depending on the expected behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
README.mdpackage.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T19:46:11.878Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T19:46:11.878Z
Learning: Configuration files like `package.json` and `todesktop.json` must be kept in sync with version numbers and build settings
Applied to files:
package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-windows-debug
- GitHub Check: build-and-test-e2e-windows / integration-windows-test
- GitHub Check: build-apple-debug-all / build-macos-debug
- GitHub Check: lint-and-format (windows-latest)
🔇 Additional comments (2)
README.md (2)
11-11: Label correctly reflects AMD support scope.The Windows download label has been updated to accurately reflect the addition of AMD GPU support alongside NVIDIA.
190-192: Header comments in.compiledfiles contain required platform and version flags. ✓Both
assets/requirements/windows_amd.compiledandassets/requirements/windows_nvidia.compiledinclude the required--python-platform x86_64-pc-windows-msvcand--python-version 3.12flags in their header comments, along with all other necessary compilation options.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
scripts/downloadFrontend.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESLint and Prettier for code formatting; run
yarn lintandyarn formatbefore committing
Files:
scripts/downloadFrontend.js
🧬 Code graph analysis (1)
scripts/downloadFrontend.js (2)
scripts/makeComfy.js (1)
output(39-39)scripts/todesktop/beforeInstall.cjs (1)
output(38-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-windows-debug
- GitHub Check: lint-and-format (macos-latest)
- GitHub Check: lint-and-format (windows-latest)
- GitHub Check: build-and-test-e2e-windows / integration-windows-test
- GitHub Check: build-apple-debug-all / build-macos-debug
🔇 Additional comments (2)
scripts/downloadFrontend.js (2)
47-60: LGTM: Resilient command execution wrapper.The
execAndLogfunction properly wrapsexecSyncwith error handling, logs both success and failure output, and improves debuggability by capturing the failed command context before rethrowing.
62-84: LGTM: Safe extraction of exec error output.The
logExecErrorOutputhelper uses appropriate type checking and optional chaining to safely extract and log stdout/stderr fromexecSyncerrors. The defensive checks prevent crashes when properties are unavailable.
| await fs.rm(frontendDir, { recursive: true }); | ||
| } catch (error) { | ||
| console.error('Error building frontend:', error.message); | ||
| console.error('Error building frontend:', error instanceof Error ? error.message : String(error)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improved error handling with type-safe check.
The use of error instanceof Error ? error.message : String(error) is a robust pattern that handles both Error objects and other thrown values.
For consistency, consider applying this same pattern to lines 118 and 136, which currently access error.message directly.
Optional: Apply consistent error handling pattern
- console.error('Error downloading frontend:', error.message);
+ console.error('Error downloading frontend:', error instanceof Error ? error.message : String(error));And similarly for line 136:
- console.error('Error copying desktop-ui:', error.message);
+ console.error('Error copying desktop-ui:', error instanceof Error ? error.message : String(error));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In scripts/downloadFrontend.js around lines 118 and 136, direct access to
error.message is used which can throw if a non-Error is thrown; update both
locations to use the same type-safe pattern used at line 37: replace occurrences
like error.message with error instanceof Error ? error.message : String(error)
(or store that expression in a local variable and reuse it) so the logged/output
error text is safe and consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/downloadFrontend.js (1)
41-43: Good type-safe error handling.The
error instanceof Error ? error.message : String(error)pattern correctly handles unknown error types. Based on learnings, this is the recommended approach.Note: Lines 123 and 141 still use direct
error.messageaccess without type narrowing—this was flagged in a previous review.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
scripts/downloadFrontend.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESLint and Prettier for code formatting; run
yarn lintandyarn formatbefore committing
Files:
scripts/downloadFrontend.js
🧠 Learnings (4)
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to **/*.{ts,mts,cts} : Treat caught errors as `unknown` and narrow with `instanceof Error` (or custom guards) before access
Applied to files:
scripts/downloadFrontend.js
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to **/*.{ts,mts,cts} : Avoid non-null assertions (`!`). Use optional chaining, nullish coalescing, and explicit guards
Applied to files:
scripts/downloadFrontend.js
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to **/*.{ts,mts,cts} : Throw `Error` (or subclasses) with actionable messages. Avoid throwing strings
Applied to files:
scripts/downloadFrontend.js
📚 Learning: 2025-11-25T20:50:17.635Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/typescript.mdc:0-0
Timestamp: 2025-11-25T20:50:17.635Z
Learning: Applies to **/*.{ts,mts,cts} : Do not use non-null assertions or `ts-ignore`. Use `ts-expect-error` with an explanation when unavoidable
Applied to files:
scripts/downloadFrontend.js
🧬 Code graph analysis (1)
scripts/downloadFrontend.js (2)
scripts/todesktop/beforeInstall.cjs (2)
path(2-2)output(38-38)scripts/makeComfy.js (1)
output(39-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-windows-debug
- GitHub Check: build-and-test-e2e-windows / integration-windows-test
- GitHub Check: build-apple-debug-all / build-macos-debug
- GitHub Check: lint-and-format (windows-latest)
- GitHub Check: lint-and-format (macos-latest)
🔇 Additional comments (2)
scripts/downloadFrontend.js (2)
32-37: LGTM!The direct
nx buildapproach with explanatory comment is clear. The 8GB heap allocation viaNODE_OPTIONSis reasonable for large frontend builds, and the environment variable handling is correct.
67-89: LGTM!The helper correctly handles unknown error types with proper guards. The optional chaining on
stdout?.toString?.()andstderr?.toString?.()safely extracts output when available, consistent with the pattern inscripts/todesktop/beforeInstall.cjs. Based on learnings, this approach properly avoids non-null assertions.
| function execAndLog(command, cwd, env = {}) { | ||
| const output = execSync(command, { | ||
| cwd, | ||
| encoding: 'utf8', | ||
| env: { ...process.env, ...env }, | ||
| }); | ||
| console.log(output); | ||
| try { | ||
| const output = execSync(command, { | ||
| cwd, | ||
| encoding: 'utf8', | ||
| env: { ...process.env, ...env }, | ||
| }); | ||
| console.log(output); | ||
| } catch (error) { | ||
| console.error(`Command failed: ${command}`); | ||
| logExecErrorOutput(error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Solid error handling with proper logging and rethrow.
The try/catch wrapper correctly captures output on success and delegates to logExecErrorOutput on failure before rethrowing. The encoding: 'utf8' usage is consistent with other scripts.
Minor style note: the function is declared after its first use (lines 30-31). While hoisting makes this valid, consider moving the helper functions before the try block for improved readability.
🤖 Prompt for AI Agents
In scripts/downloadFrontend.js around lines 52 to 65, execAndLog is defined
after it's first invoked (used at lines 30-31); move this helper (and any
related helpers like logExecErrorOutput) above their first use so the function
declaration appears before the try block that calls it, keeping the same
implementation and behavior but improving readability.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Enable Windows AMD GPU support in hardware validation and installs.
What changed
selectedDeviceisamd, with new compiled requirements and override assets.Why