Update NVIDIA torch version on updates

[benceruleanlu]

┆Issue is synchronized with this Notion page by Unito

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds NVIDIA/CUDA-specific PyTorch constants and install logic: detects installed torch packages on NVIDIA devices, compares versions, warns if the system NVIDIA driver is below minimum, and installs recommended NVIDIA torch packages from a CUDA mirror when mismatches are found.

Changes

Cohort / File(s)	Summary
Constants src/constants.ts	Added NVIDIA_TORCH_VERSION (2.9.1+cu130), NVIDIA_TORCHVISION_VERSION (0.24.1+cu130), and NVIDIA_TORCH_PACKAGES array using template strings.
Installation manager src/install/installationManager.ts	Added NVIDIA driver parsing/comparison helpers (parseNvidiaDriverVersionFromSmiOutput, isNvidiaDriverBelowMinimum), async exec usage, warnIfNvidiaDriverTooOld, getNvidiaDriverVersionFromSmi*, and invoked NVIDIA torch recommendation flow during update/fresh install.
Virtual environment / PyTorch handling src/virtualEnvironment.ts	Added ensureRecommendedNvidiaTorch(callbacks?), logic to read installed torch/torchaudio/torchvision versions, compute mismatches, and install NVIDIA_TORCH_PACKAGES from CUDA mirror; integrated into requirements checks and install/upgrade flows.
Tests tests/unit/install/installationManager.test.ts	Added tests and exports for parseNvidiaDriverVersionFromSmiOutput and isNvidiaDriverBelowMinimum.

Sequence Diagram

sequenceDiagram
    participant IM as InstallationManager
    participant VE as VirtualEnvironment
    participant PIP as venv pip
    participant SYS as System (nvidia-smi / network)

    IM->>VE: ensureRecommendedNvidiaTorch()
    VE->>PIP: pip list --format=json (venv)
    PIP-->>VE: installed package versions
    VE->>VE: compare installed vs NVIDIA_TORCH_* versions
    alt mismatch detected
        IM->>SYS: run nvidia-smi (driver check)
        SYS-->>IM: driver version / error
        IM->>IM: isNvidiaDriverBelowMinimum?
        VE->>PIP: pip install NVIDIA_TORCH_PACKAGES (CUDA mirror)
        PIP->>SYS: download packages
        PIP-->>VE: install success/failure
        VE-->>IM: report result or throw
    else versions match
        VE-->>IM: no action
    end

Loading

Possibly related PRs

• Update NVIDIA PyTorch to 2.9.1 cu130 #1512: Modifies CUDA mirror URLs and NVIDIA/CUDA torch version targets—strongly related to the added NVIDIA_TORCH_* constants and install flow.
• Add AMD GPU Support #1501: Adds vendor-specific package lists and install flows in similar modules (src/constants.ts, src/virtualEnvironment.ts)—related by GPU-specific installation logic.

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This pull request adds functionality to ensure NVIDIA PyTorch installations use specific recommended versions during package updates. The implementation checks the installed PyTorch version and upgrades it to version 2.9.1+cu130 if it doesn't match the recommended version.

Key changes:

• Added version checking logic to detect PyTorch version mismatches for NVIDIA devices
• Integrated automatic PyTorch upgrade into the package update workflow
• New method to read installed torch version via Python introspection

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/constants.ts	Defines NVIDIA_TORCH_VERSION (2.9.1+cu130) and NVIDIA_TORCH_PACKAGES constants for version enforcement
src/virtualEnvironment.ts	Adds three new methods: ensureRecommendedNvidiaTorch for upgrading, getTorchVersion for version reading, and needsNvidiaTorchUpgrade for version checking logic
src/install/installationManager.ts	Integrates ensureRecommendedNvidiaTorch call into the package update flow after installing ComfyUI requirements

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The method ensureRecommendedNvidiaTorch doesn't verify that the installation succeeded by checking the torch version after installation. Consider adding a verification step after the installation completes to confirm that getTorchVersion() returns NVIDIA_TORCH_VERSION. This would help catch installation failures where the exit code is 0 but the actual version installed doesn't match expectations.

Suggested change

	}
	}
	const verifiedVersion = await this.getTorchVersion();
	if (verifiedVersion !== NVIDIA_TORCH_VERSION) {
	log.error('Recommended NVIDIA PyTorch version verification failed.', {
	expectedVersion: NVIDIA_TORCH_VERSION,
	actualVersion: verifiedVersion,
	});
	throw new Error(
	`Failed to verify recommended NVIDIA PyTorch version. Expected ${NVIDIA_TORCH_VERSION}, got ${verifiedVersion ?? 'unknown'}.`
	);
	}
	log.info('Successfully installed recommended NVIDIA PyTorch version.', {
	version: verifiedVersion,
	});

[Copilot]

When getTorchVersion() returns undefined (e.g., torch is not installed or cannot be imported), the comparison installedVersion === NVIDIA_TORCH_VERSION will be false, causing the method to attempt an installation. While this is likely the intended behavior, the log message "Installing recommended NVIDIA PyTorch packages" with { installedVersion } might be misleading when installedVersion is undefined. Consider adding a specific log message for this case, such as "PyTorch not found, installing recommended NVIDIA PyTorch packages" or checking for undefined explicitly.

Suggested change

	log.info('Installing recommended NVIDIA PyTorch packages.', { installedVersion });
	if (installedVersion === undefined) {
	log.info('PyTorch not found, installing recommended NVIDIA PyTorch packages.');
	} else {
	log.info('Installed NVIDIA PyTorch version does not match recommended version, installing recommended packages.', {
	installedVersion,
	});
	}

[Copilot]

The method needsNvidiaTorchUpgrade() is called during hasRequirements() which may be invoked frequently during validation checks. This method calls getTorchVersion() which spawns a Python process to import torch and read its version. Consider caching the torch version result for a short duration to avoid repeatedly spawning Python processes, especially if hasRequirements() might be called multiple times in quick succession.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @src/virtualEnvironment.ts:
- Around line 639-664: ensureRecommendedNvidiaTorch silently treats undefined
installedVersion the same as a mismatched version; explicitly check for
undefined from getTorchVersion() and log a clear message before installing:
inside ensureRecommendedNvidiaTorch call getTorchVersion(), if installedVersion
=== undefined log that torch was not detected and proceed with installation,
otherwise if installedVersion !== NVIDIA_TORCH_VERSION log the detected version
and that it differs from NVIDIA_TORCH_VERSION and proceed; keep existing error
handling for non-zero exitCode from runUvCommandAsync.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b8665c0 and de0b7e2.

📒 Files selected for processing (3)

• src/constants.ts
• src/install/installationManager.ts
• src/virtualEnvironment.ts

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{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-level await only when it improves clarity
Code must compile with strict mode and noImplicitAny enabled, with zero implicit any
Use unknown instead of any. Narrow with type guards. any is allowed only when interfacing with untyped code and must be localized and justified
Use interface for public object shapes intended for extension/implementation
Use type for unions, mapped/conditional types, function types, and when composition is needed
Model domain variants with a discriminant field (e.g., kind: 'X' | 'Y'). Use exhaustive switch statements
Prefer as const objects and string/number literal unions over enum
Prefer readonly properties and ReadonlyArray<T> where appropriate. Do not mutate function parameters
Prefer T[] over Array<T> for readability
Prefer Record<Key, T> for simple maps; use Map/Set when iteration semantics or key types require it
Avoid non-null assertions (!). Use optional chaining, nullish coalescing, and explicit guards
Use satisfies to enforce object literal conformance without widening values
Prefer named exports. Default exports are allowed only when a module clearly has a single primary export
Use import 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 or this binding is required
Modules should be pure. Avoid executing logic at import time unless necessary (e.g., polyfills)
Prefer async/await over raw then/catch chains. Keep linear flow and use try/catch for failures
Either await promises or deliberately mark them...

Files:

• src/install/installationManager.ts
• src/virtualEnvironment.ts
• src/constants.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/install/installationManager.ts
• src/virtualEnvironment.ts
• src/constants.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: TypeScript: Do not use the any type; use unknown when the type is genuinely unknown
Use JSDoc format for method documentation with common tags @param and @return (omit @return for void return type); use {@link } to reference symbols

Files:

• src/install/installationManager.ts
• src/virtualEnvironment.ts
• src/constants.ts

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use ESLint and Prettier for code formatting; run yarn lint and yarn format before committing

Files:

• src/install/installationManager.ts
• src/virtualEnvironment.ts
• src/constants.ts

🧬 Code graph analysis (1)

src/virtualEnvironment.ts (1)

src/constants.ts (2)

• NVIDIA_TORCH_VERSION (204-204)
• NVIDIA_TORCH_PACKAGES (205-209)

⏰ 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: 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 (macos-latest)
• GitHub Check: lint-and-format (windows-latest)

🔇 Additional comments (6)

src/install/installationManager.ts (1)

357-357: LGTM - Integration is correctly placed and error-handled.

The NVIDIA torch upgrade step is appropriately positioned after requirements installation and before validation. The existing try/catch block handles any failures gracefully.

src/virtualEnvironment.ts (4)

10-17: LGTM - Imports are correctly added.

The new constants are properly imported alongside the existing ones.

---

666-699: LGTM - Robust version detection implementation.

The method correctly captures stdout separately from stderr, uses sys.stdout.write to avoid newline issues, and gracefully handles failures by returning undefined.

---

910-913: LGTM - Correct integration into requirements check flow.

Placing the NVIDIA torch upgrade check after core/manager checks ensures it only triggers when other requirements are satisfied, preventing unnecessary upgrades during initial installs.

---

919-938: Exact string comparison is safe given the current exact version pinning strategy, but semantic version comparison would be more robust.

The constant NVIDIA_TORCH_VERSION = '2.9.1+cu130' already includes the CUDA suffix, and pip install uses torch=={NVIDIA_TORCH_VERSION} with exact version pinning. When pip installs a pinned version with ==, torch.__version__ should report the exact string installed. However, strict equality comparison (!==) is less robust than semantic version comparison—it could fail if PyTorch distributions report versions differently (e.g., if a distribution includes build metadata). Consider using the existing compareVersions() utility from src/utils.ts or at least a prefix check to handle edge cases more gracefully.

src/constants.ts (1)

204-209: LGTM - Constants follow established patterns and versions are compatible.

The NVIDIA torch constants align with the existing AMD_TORCH_PACKAGES pattern. Using template literals for torch and torchaudio while hardcoding torchvision==0.24.1+cu130 is appropriate—torchvision 0.24.1 is compatible with PyTorch 2.9.1 and follows the same versioning scheme as the AMD variant above.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In @src/install/installationManager.ts:
- Around line 411-423: The regex in getNvidiaDriverVersionFromSmiFallback is
incorrectly escaped (uses "\\s*" from a string-style escape) so it never
matches; change the pattern to a proper JS regex literal that uses \s for
whitespace, e.g. /driver version\s*:\s*([\d.]+)/i, keep the rest of the function
(execAsync call and log.debug on error) unchanged so the fallback can correctly
extract the driver version from nvidia-smi output.

In @src/virtualEnvironment.ts:
- Around line 646-686: ensureRecommendedNvidiaTorch detects mismatches via
getTorchVersionMismatches but builds PipInstallConfig without enabling upgrades,
so pip may skip reinstalling incorrect versions; fix by setting upgradePackages:
true on the PipInstallConfig you pass to getPipInstallArgs (where config
currently contains packages, indexUrl, prerelease) so the installation always
upgrades mismatched NVIDIA_TORCH_PACKAGES using the selected
torchMirror/torchMirror detection.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between de0b7e2 and d1bed18.

📒 Files selected for processing (3)

• src/constants.ts
• src/install/installationManager.ts
• src/virtualEnvironment.ts

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{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-level await only when it improves clarity
Code must compile with strict mode and noImplicitAny enabled, with zero implicit any
Use unknown instead of any. Narrow with type guards. any is allowed only when interfacing with untyped code and must be localized and justified
Use interface for public object shapes intended for extension/implementation
Use type for unions, mapped/conditional types, function types, and when composition is needed
Model domain variants with a discriminant field (e.g., kind: 'X' | 'Y'). Use exhaustive switch statements
Prefer as const objects and string/number literal unions over enum
Prefer readonly properties and ReadonlyArray<T> where appropriate. Do not mutate function parameters
Prefer T[] over Array<T> for readability
Prefer Record<Key, T> for simple maps; use Map/Set when iteration semantics or key types require it
Avoid non-null assertions (!). Use optional chaining, nullish coalescing, and explicit guards
Use satisfies to enforce object literal conformance without widening values
Prefer named exports. Default exports are allowed only when a module clearly has a single primary export
Use import 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 or this binding is required
Modules should be pure. Avoid executing logic at import time unless necessary (e.g., polyfills)
Prefer async/await over raw then/catch chains. Keep linear flow and use try/catch for failures
Either await promises or deliberately mark them...

Files:

• src/virtualEnvironment.ts
• src/install/installationManager.ts
• src/constants.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/virtualEnvironment.ts
• src/install/installationManager.ts
• src/constants.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: TypeScript: Do not use the any type; use unknown when the type is genuinely unknown
Use JSDoc format for method documentation with common tags @param and @return (omit @return for void return type); use {@link } to reference symbols

Files:

• src/virtualEnvironment.ts
• src/install/installationManager.ts
• src/constants.ts

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use ESLint and Prettier for code formatting; run yarn lint and yarn format before committing

Files:

• src/virtualEnvironment.ts
• src/install/installationManager.ts
• src/constants.ts

🧠 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: Applies to src/main.ts : For main process entry point at `src/main.ts`, implement Electron application lifecycle management and window initialization

Applied to files:

• src/install/installationManager.ts

🧬 Code graph analysis (2)

src/virtualEnvironment.ts (1)

src/constants.ts (3)

• NVIDIA_TORCH_VERSION (204-204)
• NVIDIA_TORCHVISION_VERSION (205-205)
• NVIDIA_TORCH_PACKAGES (206-210)

src/install/installationManager.ts (2)

src/main-process/comfyInstallation.ts (1)

• ComfyInstallation (18-263)

src/utils.ts (1)

• compareVersions (303-318)

⏰ 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: lint-and-format (windows-latest)
• GitHub Check: build-apple-debug-all / build-macos-debug
• GitHub Check: lint-and-format (macos-latest)
• GitHub Check: build-and-test-e2e-windows / integration-windows-test

🔇 Additional comments (12)

src/install/installationManager.ts (5)

3-4: LGTM! Proper async exec setup.

Using promisify(exec) is the correct approach for running shell commands asynchronously in Node.js.

---

362-364: LGTM! Good placement of NVIDIA-specific checks.

The warning about driver version is appropriately placed before the torch upgrade attempt, giving users a chance to understand potential compatibility issues.

---

371-392: Well-structured driver version warning flow.

The method appropriately guards for platform and device, uses a fallback approach for version detection, and presents a non-blocking warning that doesn't prevent installation. This is a good UX choice for users who may want to proceed despite the warning.

---

394-409: LGTM! Clean nvidia-smi query implementation.

The query format --query-gpu=driver_version --format=csv,noheader is the correct approach for structured output. The parsing handles cross-platform line endings appropriately.

---

25-27: The NVIDIA_DRIVER_MIN_VERSION value of '580' is correct. CUDA 13.0 officially requires a minimum driver version of 580 (specifically 580.65.06 or later according to the official CUDA Toolkit Release Notes). No changes are needed.

src/virtualEnvironment.ts (6)

10-18: LGTM! Imports are correctly updated.

The new NVIDIA constants are properly imported and will be used for version comparison and package installation.

---

54-58: LGTM! Well-designed type definitions.

The types correctly distinguish between expected versions (always defined) and installed versions (potentially undefined). Using type aliases for these record types follows the coding guidelines.

---

688-753: LGTM! Robust package version detection with comprehensive error handling.

The method handles various failure modes gracefully (non-zero exit, empty output, invalid JSON, unexpected structure). The type guard and iteration logic are well-implemented.

---

964-968: LGTM! Correct integration with existing requirements checking.

The NVIDIA torch upgrade check is appropriately placed after the core/manager requirements checks, ensuring it doesn't interfere with the existing upgrade detection flow.

---

973-1002: LGTM! Conservative approach handles edge cases safely.

Treating unreadable versions as requiring an upgrade is a safe fallback, though it may cause unnecessary reinstalls in rare cases where uv pip list fails transiently.

---

1004-1017: LGTM! Clean version comparison helper.

The strict equality comparison is appropriate since NVIDIA torch versions include the CUDA build suffix (e.g., +cu130) which must match exactly.

src/constants.ts (1)

204-210: LGTM! Constants follow established patterns.

The NVIDIA torch version constants are well-structured and consistent with the existing AMD_TORCH_PACKAGES pattern. PyTorch 2.9.1+cu130 and torchvision 0.24.1+cu130 are both available on the PyTorch wheel index with full CUDA 13.0 support. The use of template literals ensures the package versions stay synchronized with the version constants.

[coderabbitai]

Actionable comments posted: 4

🤖 Fix all issues with AI agents

In @src/virtualEnvironment.ts:
- Around line 54-58: The TorchPackageVersions type should be made immutable:
change type TorchPackageVersions = Record<TorchPackageName, string | undefined>
to a readonly form (e.g., Readonly<Record<TorchPackageName, string | undefined>>
or { readonly [K in TorchPackageName]: string | undefined }) and also mark the
package names array as readonly by changing TORCH_PACKAGE_NAMES:
TorchPackageName[] = [...] to a ReadonlyArray<TorchPackageName> (or use as
const) so both the mapping type and the array are immutable.
- Line 997: Extract the hardcoded CUDA tag '+cu130' into a shared constant and
use it everywhere: add REQUIRED_CUDA_TAG = '+cu130' to your constants module
(e.g., export const REQUIRED_CUDA_TAG) and update NVIDIA_TORCH_VERSION and
NVIDIA_TORCHVISION_VERSION to interpolate that constant; then remove the local
const requiredCudaTag in virtualEnvironment.ts and import and use
REQUIRED_CUDA_TAG instead (update any other occurrences that reference
requiredCudaTag to use the new constant).
- Around line 645-688: ensureRecommendedNvidiaTorch is missing telemetry around
the NVIDIA torch upgrade flow; add telemetry events to record attempts,
successes and failures for both the pinned install and the fallback install.
Specifically, fire a "nvidia_torch_install_started" event at the start of the
method (before runUvCommandAsync), record a "nvidia_torch_install_succeeded"
event when pinnedExitCode === 0, and if pinned fails record a
"nvidia_torch_install_pinned_failed" with pinnedExitCode before attempting
fallback; after the fallback runUvCommandAsync emit
"nvidia_torch_install_fallback_succeeded" on fallbackExitCode === 0 or
"nvidia_torch_install_fallback_failed" with both exit codes if it ultimately
throws. Use the same telemetry helper/decorator pattern used elsewhere in the
repo (the @trackEvent wrapper or telemetry client) and include contextual
properties like installedVersions, torchMirror, pinnedExitCode and
fallbackExitCode; locate changes around ensureRecommendedNvidiaTorch,
runUvCommandAsync calls, and the NVIDIA_TORCH_PACKAGES/getPipInstallArgs usage.
- Around line 749-752: The block that currently only returns undefined when
matched === 0 should instead treat any partial installation as invalid: change
the conditional to return undefined when matched < 3 (i.e., fewer than the three
packages torch, torchaudio, torchvision were found) and update the log.warn
message to reflect a partial or missing torch package installation; this ensures
callers like meetsMinimumNvidiaTorchVersions() receive either a complete set of
versions or undefined.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1bed18 and 8978759.

📒 Files selected for processing (1)

• src/virtualEnvironment.ts

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{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-level await only when it improves clarity
Code must compile with strict mode and noImplicitAny enabled, with zero implicit any
Use unknown instead of any. Narrow with type guards. any is allowed only when interfacing with untyped code and must be localized and justified
Use interface for public object shapes intended for extension/implementation
Use type for unions, mapped/conditional types, function types, and when composition is needed
Model domain variants with a discriminant field (e.g., kind: 'X' | 'Y'). Use exhaustive switch statements
Prefer as const objects and string/number literal unions over enum
Prefer readonly properties and ReadonlyArray<T> where appropriate. Do not mutate function parameters
Prefer T[] over Array<T> for readability
Prefer Record<Key, T> for simple maps; use Map/Set when iteration semantics or key types require it
Avoid non-null assertions (!). Use optional chaining, nullish coalescing, and explicit guards
Use satisfies to enforce object literal conformance without widening values
Prefer named exports. Default exports are allowed only when a module clearly has a single primary export
Use import 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 or this binding is required
Modules should be pure. Avoid executing logic at import time unless necessary (e.g., polyfills)
Prefer async/await over raw then/catch chains. Keep linear flow and use try/catch for failures
Either await promises or deliberately mark them...

Files:

• src/virtualEnvironment.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/virtualEnvironment.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: TypeScript: Do not use the any type; use unknown when the type is genuinely unknown
Use JSDoc format for method documentation with common tags @param and @return (omit @return for void return type); use {@link } to reference symbols

Files:

• src/virtualEnvironment.ts

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use ESLint and Prettier for code formatting; run yarn lint and yarn format before committing

Files:

• src/virtualEnvironment.ts

🧬 Code graph analysis (1)

src/virtualEnvironment.ts (2)

src/constants.ts (3)

• NVIDIA_TORCH_PACKAGES (206-210)
• NVIDIA_TORCH_VERSION (204-204)
• NVIDIA_TORCHVISION_VERSION (205-205)

src/utils.ts (1)

• compareVersions (303-318)

⏰ 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-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 (5)

src/virtualEnvironment.ts (5)

10-18: LGTM - Import additions are appropriate.

The new imports for NVIDIA torch handling are well-structured and follow ESM conventions.

Also applies to: 27-27

---

966-970: LGTM - NVIDIA torch check properly integrated.

The integration into hasRequirements follows the existing pattern for package upgrade detection and includes appropriate logging.

---

975-990: LGTM - Defensive version check logic.

The method appropriately handles missing version data and provides clear logging for troubleshooting.

---

1005-1007: Verify compareVersions function behavior with PyTorch CUDA-tagged versions.

The concern about compareVersions handling version strings with CUDA suffixes (e.g., '2.9.1+cu130') is valid. According to SemVer 2.0.0, the + character denotes build metadata, which should be ignored during version comparison—only major.minor.patch matters. However, not all version comparison libraries handle this consistently. Confirm that the compareVersions implementation:

1. Strips or ignores everything after the + character before comparison
2. Correctly parses versions like '2.9.1+cu130' as equivalent to '2.9.1' for comparison purposes
3. Or that input versions are pre-processed to remove CUDA tags before calling compareVersions

If the function doesn't handle this, version comparisons at lines 1005-1007 may produce incorrect results when PyTorch packages include CUDA metadata.

---

675-680: Remove this comment — the concern is based on a misunderstanding of pip's -U behavior.

The -U flag in pip install -U torch torchvision torchaudio upgrades only those three specified packages, not unrelated dependencies in the environment. The fallback intentionally uses -U because the pinned version install failed, allowing more flexible resolution as a recovery strategy. This is a reasonable and intentional pattern, not an aggressive or risky approach.

Likely an incorrect or invalid review comment.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider making TorchPackageVersions readonly for immutability.

The types and constants are well-defined. However, to align with the coding guideline to "prefer readonly properties and ReadonlyArray<T> where appropriate," consider making the Record values readonly.

♻️ Proposed refinement

-type TorchPackageVersions = Record<TorchPackageName, string | undefined>;
+type TorchPackageVersions = Readonly<Record<TorchPackageName, string | undefined>>;

📝 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.

Suggested change

	type TorchPackageName = 'torch' | 'torchaudio' | 'torchvision';
	type TorchPackageVersions = Record<TorchPackageName, string | undefined>;
	const TORCH_PACKAGE_NAMES: TorchPackageName[] = ['torch', 'torchaudio', 'torchvision'];
	type TorchPackageName = 'torch' | 'torchaudio' | 'torchvision';
	type TorchPackageVersions = Readonly<Record<TorchPackageName, string | undefined>>;
	const TORCH_PACKAGE_NAMES: TorchPackageName[] = ['torch', 'torchaudio', 'torchvision'];

🤖 Prompt for AI Agents

In @src/virtualEnvironment.ts around lines 54 - 58, The TorchPackageVersions
type should be made immutable: change type TorchPackageVersions =
Record<TorchPackageName, string | undefined> to a readonly form (e.g.,
Readonly<Record<TorchPackageName, string | undefined>> or { readonly [K in
TorchPackageName]: string | undefined }) and also mark the package names array
as readonly by changing TORCH_PACKAGE_NAMES: TorchPackageName[] = [...] to a
ReadonlyArray<TorchPackageName> (or use as const) so both the mapping type and
the array are immutable.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Add telemetry tracking for NVIDIA torch upgrade flow.

This new public method lacks telemetry events to track success/failure of NVIDIA torch upgrades. Given that other installation methods use @trackEvent decorators or explicit telemetry calls, this should follow the same pattern.

📊 Proposed telemetry additions

 async ensureRecommendedNvidiaTorch(callbacks?: ProcessCallbacks): Promise<void> {
   if (this.selectedDevice !== 'nvidia') return;

+  this.telemetry.track('install_flow:nvidia_torch_upgrade_start');
+
   const installedVersions = await this.getInstalledTorchPackageVersions();
   if (installedVersions && this.meetsMinimumNvidiaTorchVersions(installedVersions)) {
     log.info('NVIDIA PyTorch packages already satisfy minimum recommended versions.', installedVersions);
+    this.telemetry.track('install_flow:nvidia_torch_upgrade_end', { reason: 'already_satisfied' });
     return;
   }

   const torchMirror = this.torchMirror || getDefaultTorchMirror(this.selectedDevice);
   const config: PipInstallConfig = {
     packages: NVIDIA_TORCH_PACKAGES,
     indexUrl: torchMirror,
     prerelease: torchMirror.includes('nightly'),
   };

   const installArgs = getPipInstallArgs(config);
   log.info('Installing recommended NVIDIA PyTorch packages.', { installedVersions });
   const { exitCode: pinnedExitCode } = await this.runUvCommandAsync(installArgs, callbacks);

-  if (pinnedExitCode === 0) return;
+  if (pinnedExitCode === 0) {
+    this.telemetry.track('install_flow:nvidia_torch_upgrade_end', { reason: 'pinned_install_success' });
+    return;
+  }

   log.warn('Failed to install recommended NVIDIA PyTorch packages. Falling back to unpinned install.', {
     exitCode: pinnedExitCode,
   });

   const fallbackConfig: PipInstallConfig = {
     packages: ['torch', 'torchvision', 'torchaudio'],
     indexUrl: torchMirror,
     prerelease: torchMirror.includes('nightly'),
     upgradePackages: true,
   };
   const fallbackArgs = getPipInstallArgs(fallbackConfig);
   const { exitCode: fallbackExitCode } = await this.runUvCommandAsync(fallbackArgs, callbacks);
   if (fallbackExitCode !== 0) {
+    this.telemetry.track('install_flow:nvidia_torch_upgrade_error', {
+      pinnedExitCode,
+      fallbackExitCode,
+    });
     throw new Error(
       `Failed to install NVIDIA PyTorch packages (pinned exit ${pinnedExitCode}, fallback exit ${fallbackExitCode})`
     );
   }
+  this.telemetry.track('install_flow:nvidia_torch_upgrade_end', { reason: 'fallback_success' });
 }

📝 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.

Suggested change

	/**
	* Ensures NVIDIA installs use the recommended PyTorch packages.
	* @param callbacks The callbacks to use for the command.
	*/
	async ensureRecommendedNvidiaTorch(callbacks?: ProcessCallbacks): Promise<void> {
	if (this.selectedDevice !== 'nvidia') return;
	const installedVersions = await this.getInstalledTorchPackageVersions();
	if (installedVersions && this.meetsMinimumNvidiaTorchVersions(installedVersions)) {
	log.info('NVIDIA PyTorch packages already satisfy minimum recommended versions.', installedVersions);
	return;
	}
	const torchMirror = this.torchMirror || getDefaultTorchMirror(this.selectedDevice);
	const config: PipInstallConfig = {
	packages: NVIDIA_TORCH_PACKAGES,
	indexUrl: torchMirror,
	prerelease: torchMirror.includes('nightly'),
	};
	const installArgs = getPipInstallArgs(config);
	log.info('Installing recommended NVIDIA PyTorch packages.', { installedVersions });
	const { exitCode: pinnedExitCode } = await this.runUvCommandAsync(installArgs, callbacks);
	if (pinnedExitCode === 0) return;
	log.warn('Failed to install recommended NVIDIA PyTorch packages. Falling back to unpinned install.', {
	exitCode: pinnedExitCode,
	});
	const fallbackConfig: PipInstallConfig = {
	packages: ['torch', 'torchvision', 'torchaudio'],
	indexUrl: torchMirror,
	prerelease: torchMirror.includes('nightly'),
	upgradePackages: true,
	};
	const fallbackArgs = getPipInstallArgs(fallbackConfig);
	const { exitCode: fallbackExitCode } = await this.runUvCommandAsync(fallbackArgs, callbacks);
	if (fallbackExitCode !== 0) {
	throw new Error(
	`Failed to install NVIDIA PyTorch packages (pinned exit ${pinnedExitCode}, fallback exit ${fallbackExitCode})`
	);
	}
	}
	/**
	* Ensures NVIDIA installs use the recommended PyTorch packages.
	* @param callbacks The callbacks to use for the command.
	*/
	async ensureRecommendedNvidiaTorch(callbacks?: ProcessCallbacks): Promise<void> {
	if (this.selectedDevice !== 'nvidia') return;
	this.telemetry.track('install_flow:nvidia_torch_upgrade_start');
	const installedVersions = await this.getInstalledTorchPackageVersions();
	if (installedVersions && this.meetsMinimumNvidiaTorchVersions(installedVersions)) {
	log.info('NVIDIA PyTorch packages already satisfy minimum recommended versions.', installedVersions);
	this.telemetry.track('install_flow:nvidia_torch_upgrade_end', { reason: 'already_satisfied' });
	return;
	}
	const torchMirror = this.torchMirror || getDefaultTorchMirror(this.selectedDevice);
	const config: PipInstallConfig = {
	packages: NVIDIA_TORCH_PACKAGES,
	indexUrl: torchMirror,
	prerelease: torchMirror.includes('nightly'),
	};
	const installArgs = getPipInstallArgs(config);
	log.info('Installing recommended NVIDIA PyTorch packages.', { installedVersions });
	const { exitCode: pinnedExitCode } = await this.runUvCommandAsync(installArgs, callbacks);
	if (pinnedExitCode === 0) {
	this.telemetry.track('install_flow:nvidia_torch_upgrade_end', { reason: 'pinned_install_success' });
	return;
	}
	log.warn('Failed to install recommended NVIDIA PyTorch packages. Falling back to unpinned install.', {
	exitCode: pinnedExitCode,
	});
	const fallbackConfig: PipInstallConfig = {
	packages: ['torch', 'torchvision', 'torchaudio'],
	indexUrl: torchMirror,
	prerelease: torchMirror.includes('nightly'),
	upgradePackages: true,
	};
	const fallbackArgs = getPipInstallArgs(fallbackConfig);
	const { exitCode: fallbackExitCode } = await this.runUvCommandAsync(fallbackArgs, callbacks);
	if (fallbackExitCode !== 0) {
	this.telemetry.track('install_flow:nvidia_torch_upgrade_error', {
	pinnedExitCode,
	fallbackExitCode,
	});
	throw new Error(
	`Failed to install NVIDIA PyTorch packages (pinned exit ${pinnedExitCode}, fallback exit ${fallbackExitCode})`
	);
	}
	this.telemetry.track('install_flow:nvidia_torch_upgrade_end', { reason: 'fallback_success' });
	}

🤖 Prompt for AI Agents

In @src/virtualEnvironment.ts around lines 645 - 688,
ensureRecommendedNvidiaTorch is missing telemetry around the NVIDIA torch
upgrade flow; add telemetry events to record attempts, successes and failures
for both the pinned install and the fallback install. Specifically, fire a
"nvidia_torch_install_started" event at the start of the method (before
runUvCommandAsync), record a "nvidia_torch_install_succeeded" event when
pinnedExitCode === 0, and if pinned fails record a
"nvidia_torch_install_pinned_failed" with pinnedExitCode before attempting
fallback; after the fallback runUvCommandAsync emit
"nvidia_torch_install_fallback_succeeded" on fallbackExitCode === 0 or
"nvidia_torch_install_fallback_failed" with both exit codes if it ultimately
throws. Use the same telemetry helper/decorator pattern used elsewhere in the
repo (the @trackEvent wrapper or telemetry client) and include contextual
properties like installedVersions, torchMirror, pinnedExitCode and
fallbackExitCode; locate changes around ensureRecommendedNvidiaTorch,
runUvCommandAsync calls, and the NVIDIA_TORCH_PACKAGES/getPipInstallArgs usage.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's look at the getInstalledTorchPackageVersions method implementation
sed -n '690,755p' src/virtualEnvironment.ts

Repository: Comfy-Org/desktop

Length of output: 2157

---

🏁 Script executed:

#!/bin/bash
# Check the type definition for TorchPackageVersions
rg -B5 -A10 'type TorchPackageVersions' src/virtualEnvironment.ts

Repository: Comfy-Org/desktop

Length of output: 547

---

🏁 Script executed:

#!/bin/bash
# Find all usages of getInstalledTorchPackageVersions
rg -n 'getInstalledTorchPackageVersions' --type=ts

Repository: Comfy-Org/desktop

Length of output: 400

---

🏁 Script executed:

#!/bin/bash
# Look for the meetsMinimumNvidiaTorchVersions function that likely uses the result
rg -B5 -A20 'meetsMinimumNvidiaTorchVersions' src/virtualEnvironment.ts

Repository: Comfy-Org/desktop

Length of output: 2177

---

Consider returning undefined for all partial torch package installations, not just when no packages are found.

Currently, the method returns undefined only if matched === 0, but allows partial installations (1 or 2 packages found) to return an object with some undefined fields. The caller meetsMinimumNvidiaTorchVersions() then validates that all three packages (torch, torchaudio, torchvision) are present, returning false if any are missing. This pushes the partial-installation check to the caller rather than handling it at the source.

For clearer intent and consistency, consider returning undefined whenever fewer than 3 packages are found, since callers expect either undefined (packages not properly installed) or a complete set of all three versions.

🤖 Prompt for AI Agents

In @src/virtualEnvironment.ts around lines 749 - 752, The block that currently
only returns undefined when matched === 0 should instead treat any partial
installation as invalid: change the conditional to return undefined when matched
< 3 (i.e., fewer than the three packages torch, torchaudio, torchvision were
found) and update the log.warn message to reflect a partial or missing torch
package installation; this ensures callers like
meetsMinimumNvidiaTorchVersions() receive either a complete set of versions or
undefined.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Extract hardcoded CUDA tag to a constant.

The CUDA tag '+cu130' is hardcoded here but appears in multiple constants (NVIDIA_TORCH_VERSION, NVIDIA_TORCHVISION_VERSION). Extract it to a named constant for maintainability.

♻️ Proposed refactor

In src/constants.ts, add:

export const REQUIRED_CUDA_TAG = '+cu130';
export const NVIDIA_TORCH_VERSION = `2.9.1${REQUIRED_CUDA_TAG}`;
export const NVIDIA_TORCHVISION_VERSION = `0.24.1${REQUIRED_CUDA_TAG}`;

Then in this file:

+import {
+  AMD_ROCM_SDK_PACKAGES,
+  AMD_TORCH_PACKAGES,
+  InstallStage,
+  NVIDIA_TORCHVISION_VERSION,
+  NVIDIA_TORCH_PACKAGES,
+  NVIDIA_TORCH_VERSION,
+  REQUIRED_CUDA_TAG,
+  TorchMirrorUrl,
+} from './constants';

...

-    const requiredCudaTag = '+cu130';
+    const requiredCudaTag = REQUIRED_CUDA_TAG;

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In @src/virtualEnvironment.ts at line 997, Extract the hardcoded CUDA tag
'+cu130' into a shared constant and use it everywhere: add REQUIRED_CUDA_TAG =
'+cu130' to your constants module (e.g., export const REQUIRED_CUDA_TAG) and
update NVIDIA_TORCH_VERSION and NVIDIA_TORCHVISION_VERSION to interpolate that
constant; then remove the local const requiredCudaTag in virtualEnvironment.ts
and import and use REQUIRED_CUDA_TAG instead (update any other occurrences that
reference requiredCudaTag to use the new constant).

[coderabbitai]

Actionable comments posted: 5

🤖 Fix all issues with AI agents

In @src/install/installationManager.ts:
- Line 421: Add a timeout to the execAsync calls that query nvidia-smi to avoid
hangs: when calling execAsync (the lines with "const { stdout } = await
execAsync('nvidia-smi --query-gpu=driver_version --format=csv,noheader')" and
the fallback execAsync at the other site), pass a reasonable timeout option
(e.g., 5–15s) in the options argument and ensure the call is wrapped in
try/catch so timeout errors are handled deterministically (log/throw a clear
error or fall back as appropriate).
- Around line 393-414: The warning in warnIfNvidiaDriverTooOld hardcodes
"PyTorch 2.9.1 + cu130"; update the message to reference the canonical
NVIDIA_TORCH_VERSION constant instead so it stays in sync. Import
NVIDIA_TORCH_VERSION (and keep NVIDIA_DRIVER_MIN_VERSION usage) and interpolate
that constant into the message/detail text where the hardcoded string appears in
the appWindow.showMessageBox call.
- Line 26: NVIDIA_DRIVER_MIN_VERSION is declared locally but should be
centralized with the other NVIDIA constants; move the constant into the shared
constants module by adding and exporting NVIDIA_DRIVER_MIN_VERSION alongside
NVIDIA_TORCH_VERSION/NVIDIA_TORCHVISION_VERSION/NVIDIA_TORCH_PACKAGES, remove
the local const from installationManager.ts, and import the exported
NVIDIA_DRIVER_MIN_VERSION symbol where it’s used (in installationManager) so all
NVIDIA-related constants live in one place.

In @tests/unit/install/installationManager.test.ts:
- Line 312: Replace the use of the test definition helper "it.each([...])" with
"test.each([...])" in the installationManager test block (change the "it.each"
call to "test.each" and keep the provided test titles and callback unchanged) so
the suite follows Vitest's guideline to use test instead of it.
- Around line 256-309: Add edge-case tests for
parseNvidiaDriverVersionFromSmiOutput: create new it() cases that pass an empty
string and expect null/undefined or empty result, pass output missing the
"Driver Version" line and assert no version is returned, pass malformed version
strings (e.g., "Driver Version: 59a.?" or "Driver Version: 591") and assert the
parser rejects/returns null, and include case-variation inputs like "driver
version: 591.59" to confirm case-insensitivity; place these tests alongside the
existing describe('parseNvidiaDriverVersionFromSmiOutput', ...) using the same
helper function name so failures point directly to
parseNvidiaDriverVersionFromSmiOutput.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8978759 and bb650e5.

📒 Files selected for processing (2)

• src/install/installationManager.ts
• tests/unit/install/installationManager.test.ts

🧰 Additional context used

📓 Path-based instructions (7)

**/*.{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-level await only when it improves clarity
Code must compile with strict mode and noImplicitAny enabled, with zero implicit any
Use unknown instead of any. Narrow with type guards. any is allowed only when interfacing with untyped code and must be localized and justified
Use interface for public object shapes intended for extension/implementation
Use type for unions, mapped/conditional types, function types, and when composition is needed
Model domain variants with a discriminant field (e.g., kind: 'X' | 'Y'). Use exhaustive switch statements
Prefer as const objects and string/number literal unions over enum
Prefer readonly properties and ReadonlyArray<T> where appropriate. Do not mutate function parameters
Prefer T[] over Array<T> for readability
Prefer Record<Key, T> for simple maps; use Map/Set when iteration semantics or key types require it
Avoid non-null assertions (!). Use optional chaining, nullish coalescing, and explicit guards
Use satisfies to enforce object literal conformance without widening values
Prefer named exports. Default exports are allowed only when a module clearly has a single primary export
Use import 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 or this binding is required
Modules should be pure. Avoid executing logic at import time unless necessary (e.g., polyfills)
Prefer async/await over raw then/catch chains. Keep linear flow and use try/catch for failures
Either await promises or deliberately mark them...

Files:

• tests/unit/install/installationManager.test.ts
• src/install/installationManager.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/install/installationManager.test.ts

tests/unit/**/*.test.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/vitest.mdc)

tests/unit/**/*.test.{js,ts}: Use vitest for unit testing
Do not build custom testing infrastructure; use Vitest and existing helpers
Store tests in the tests/unit/ directory with directory structure matching that of the tested file in src/
Tests should be cross-platform compatible; ensure proper use of path.normalize, path.join, and path.sep for Windows, macOS, and Linux compatibility
Mocks should be cleanly written, easy to understand, and reusable where possible
Prefer the use of test.extend over loose variables in Vitest tests
Use test instead of it for defining tests

Files:

• tests/unit/install/installationManager.test.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: TypeScript: Do not use the any type; use unknown when the type is genuinely unknown
Use JSDoc format for method documentation with common tags @param and @return (omit @return for void return type); use {@link } to reference symbols

Files:

• tests/unit/install/installationManager.test.ts
• src/install/installationManager.ts

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use ESLint and Prettier for code formatting; run yarn lint and yarn format before committing

Files:

• tests/unit/install/installationManager.test.ts
• src/install/installationManager.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/install/installationManager.test.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/install/installationManager.ts

🧠 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: Applies to src/main.ts : For main process entry point at `src/main.ts`, implement Electron application lifecycle management and window initialization

Applied to files:

• src/install/installationManager.ts

🧬 Code graph analysis (2)

tests/unit/install/installationManager.test.ts (1)

src/install/installationManager.ts (2)

• parseNvidiaDriverVersionFromSmiOutput (33-36)
• isNvidiaDriverBelowMinimum (43-48)

src/install/installationManager.ts (1)

src/utils.ts (1)

• compareVersions (303-318)

⏰ 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-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 (5)

src/install/installationManager.ts (5)

3-4: LGTM: Import additions are appropriate.

The additions of exec, promisify, and compareVersions are necessary for the NVIDIA driver checking functionality and follow Node.js best practices.

Also applies to: 19-19

---

25-25: LGTM: Standard promisify pattern.

The promisification of exec is idiomatic and appropriate for async/await usage.

---

28-48: LGTM: Well-documented utility functions with appropriate types.

Both exported helpers follow TypeScript best practices:

• Explicit return types (string | undefined and boolean)
• Comprehensive JSDoc documentation with @param and @returns
• Pure functions with no side effects
• Appropriate use of optional chaining and default parameters

---

384-385: LGTM: Logical integration of NVIDIA driver checks.

The placement and order of these calls are appropriate:

1. Warning users about outdated drivers before attempting torch installation
2. Ensuring recommended NVIDIA torch packages are installed
3. Both operations are properly awaited and within error handling

---

419-444: LGTM: Robust two-tier driver version detection with appropriate error handling.

The implementation demonstrates good practices:

• Primary method uses structured CSV output for cleaner parsing
• Fallback method handles cases where query format isn't available
• Cross-platform line ending handling (/\r?\n/)
• Graceful degradation with undefined returns on errors
• Debug-level logging avoids alarming users for non-critical failures

The two-tier approach ensures maximum compatibility across different NVIDIA driver versions.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for NVIDIA-related constants in the codebase

echo "=== Searching for NVIDIA constants ==="
rg -n --type=ts 'NVIDIA.*VERSION|NVIDIA.*PACKAGE|NVIDIA.*VERSION|NVIDIA_DRIVER' -g '!*.test.ts' -C2

Repository: Comfy-Org/desktop

Length of output: 3058

---

🏁 Script executed:

#!/bin/bash
# Check if NVIDIA_DRIVER_MIN_VERSION is exported or used in other files

echo "=== Checking for NVIDIA_DRIVER_MIN_VERSION exports ==="
rg -n 'export.*NVIDIA_DRIVER_MIN_VERSION' --type=ts

echo ""
echo "=== Checking for imports of NVIDIA_DRIVER_MIN_VERSION ==="
rg -n 'import.*NVIDIA_DRIVER_MIN_VERSION|from.*installationManager' --type=ts

echo ""
echo "=== All references to NVIDIA_DRIVER_MIN_VERSION ==="
rg -n 'NVIDIA_DRIVER_MIN_VERSION' --type=ts -g '!*.test.ts'

Repository: Comfy-Org/desktop

Length of output: 913

---

Consider centralizing NVIDIA_DRIVER_MIN_VERSION with other NVIDIA constants.

Other NVIDIA-related constants (NVIDIA_TORCH_VERSION, NVIDIA_TORCHVISION_VERSION, NVIDIA_TORCH_PACKAGES) are centralized in src/constants.ts. Moving NVIDIA_DRIVER_MIN_VERSION there would improve consistency and discoverability across the codebase, even though it is currently only used within this file.

🤖 Prompt for AI Agents

In @src/install/installationManager.ts at line 26, NVIDIA_DRIVER_MIN_VERSION is
declared locally but should be centralized with the other NVIDIA constants; move
the constant into the shared constants module by adding and exporting
NVIDIA_DRIVER_MIN_VERSION alongside
NVIDIA_TORCH_VERSION/NVIDIA_TORCHVISION_VERSION/NVIDIA_TORCH_PACKAGES, remove
the local const from installationManager.ts, and import the exported
NVIDIA_DRIVER_MIN_VERSION symbol where it’s used (in installationManager) so all
NVIDIA-related constants live in one place.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the NVIDIA_TORCH_VERSION constant definition
rg -n --type=ts 'NVIDIA_TORCH_VERSION' src/constants.ts -A2

# Check how it's used elsewhere
echo "---"
rg -n --type=ts 'NVIDIA_TORCH_VERSION' -g '!*.test.ts' -C2

Repository: Comfy-Org/desktop

Length of output: 1568

---

🏁 Script executed:

#!/bin/bash
# Check if NVIDIA_TORCH_VERSION is imported in installationManager.ts
rg -n 'NVIDIA_TORCH_VERSION' src/install/installationManager.ts

# Also check the imports at the top of installationManager.ts
head -50 src/install/installationManager.ts | grep -n "import\|from"

Repository: Comfy-Org/desktop

Length of output: 1540

---

Consider referencing NVIDIA_TORCH_VERSION constant in warning message.

The warning message (line 410) contains a hardcoded PyTorch version string "PyTorch 2.9.1 + cu130" that may become outdated as versions are upgraded. The NVIDIA_TORCH_VERSION constant is already defined in src/constants.ts (as '2.9.1+cu130') and is the source of truth for NVIDIA torch versions used elsewhere in the codebase. Referencing this constant would ensure the message stays synchronized with actual package versions and reduce maintenance burden.

🤖 Prompt for AI Agents

In @src/install/installationManager.ts around lines 393 - 414, The warning in
warnIfNvidiaDriverTooOld hardcodes "PyTorch 2.9.1 + cu130"; update the message
to reference the canonical NVIDIA_TORCH_VERSION constant instead so it stays in
sync. Import NVIDIA_TORCH_VERSION (and keep NVIDIA_DRIVER_MIN_VERSION usage) and
interpolate that constant into the message/detail text where the hardcoded
string appears in the appWindow.showMessageBox call.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider adding timeout to execAsync calls for defensive programming.

While nvidia-smi is typically a fast and well-behaved command, adding a timeout parameter to the execAsync calls would prevent the installation flow from hanging indefinitely in edge cases (e.g., GPU driver issues, system instability).

⏱️ Example with timeout

-      const { stdout } = await execAsync('nvidia-smi --query-gpu=driver_version --format=csv,noheader');
+      const { stdout } = await execAsync('nvidia-smi --query-gpu=driver_version --format=csv,noheader', { timeout: 5000 });

And similarly for the fallback:

-      const { stdout } = await execAsync('nvidia-smi');
+      const { stdout } = await execAsync('nvidia-smi', { timeout: 5000 });

Also applies to: 438-438

🤖 Prompt for AI Agents

In @src/install/installationManager.ts at line 421, Add a timeout to the
execAsync calls that query nvidia-smi to avoid hangs: when calling execAsync
(the lines with "const { stdout } = await execAsync('nvidia-smi
--query-gpu=driver_version --format=csv,noheader')" and the fallback execAsync
at the other site), pass a reasonable timeout option (e.g., 5–15s) in the
options argument and ensure the call is wrapped in try/catch so timeout errors
are handled deterministically (log/throw a clear error or fall back as
appropriate).

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Add edge case tests for parsing robustness.

The test suite only covers the happy path with valid nvidia-smi output. Consider adding test cases for edge cases to ensure the parser handles various scenarios gracefully:

• Empty string input
• Output without "Driver Version" present
• Malformed version numbers (e.g., missing digits, extra characters)
• Case variations (though the regex is case-insensitive, explicit tests improve confidence)

📋 Proposed additional test cases

  describe('parseNvidiaDriverVersionFromSmiOutput', () => {
    it('parses driver version from nvidia-smi output', () => {
      const output = String.raw`
+-----------------------------------------------------------------------------------------+
| NVIDIA-SMI 591.59                 Driver Version: 591.59         CUDA Version: 13.1     |
+-----------------------------------------------------------------------------------------+
`;
      expect(parseNvidiaDriverVersionFromSmiOutput(output)).toBe('591.59');
    });
+
+   it('returns undefined when driver version is not present', () => {
+     const output = 'Some output without driver version info';
+     expect(parseNvidiaDriverVersionFromSmiOutput(output)).toBeUndefined();
+   });
+
+   it('returns undefined for empty string', () => {
+     expect(parseNvidiaDriverVersionFromSmiOutput('')).toBeUndefined();
+   });
+
+   it('parses driver version with different casing', () => {
+     const output = 'DRIVER VERSION: 591.59';
+     expect(parseNvidiaDriverVersionFromSmiOutput(output)).toBe('591.59');
+   });
  });

🤖 Prompt for AI Agents

In @tests/unit/install/installationManager.test.ts around lines 256 - 309, Add
edge-case tests for parseNvidiaDriverVersionFromSmiOutput: create new it() cases
that pass an empty string and expect null/undefined or empty result, pass output
missing the "Driver Version" line and assert no version is returned, pass
malformed version strings (e.g., "Driver Version: 59a.?" or "Driver Version:
591") and assert the parser rejects/returns null, and include case-variation
inputs like "driver version: 591.59" to confirm case-insensitivity; place these
tests alongside the existing describe('parseNvidiaDriverVersionFromSmiOutput',
...) using the same helper function name so failures point directly to
parseNvidiaDriverVersionFromSmiOutput.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use test.each instead of it.each.

Per the coding guidelines for Vitest tests: "Use test instead of it for defining tests."

🔧 Proposed fix

-  it.each([
+  test.each([
    { version: '579.0.0', expected: true, label: 'below 580' },
    { version: '580.0.0', expected: false, label: 'at 580' },
    { version: '580.0.1', expected: false, label: 'at a version of 580' },
    { version: '581.0', expected: false, label: 'above 580' },
  ])('returns $expected when $label', ({ version, expected }) => {
    expect(isNvidiaDriverBelowMinimum(version, '580')).toBe(expected);
  });

📝 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.

Suggested change

	it.each([
	test.each([
	{ version: '579.0.0', expected: true, label: 'below 580' },
	{ version: '580.0.0', expected: false, label: 'at 580' },
	{ version: '580.0.1', expected: false, label: 'at a version of 580' },
	{ version: '581.0', expected: false, label: 'above 580' },
	])('returns $expected when $label', ({ version, expected }) => {
	expect(isNvidiaDriverBelowMinimum(version, '580')).toBe(expected);
	});

🤖 Prompt for AI Agents

In @tests/unit/install/installationManager.test.ts at line 312, Replace the use
of the test definition helper "it.each([...])" with "test.each([...])" in the
installationManager test block (change the "it.each" call to "test.each" and
keep the provided test titles and callback unchanged) so the suite follows
Vitest's guideline to use test instead of it.

[benceruleanlu]

@codex review

[chatgpt-codex-connector]

To use Codex here, create a Codex account and connect to github.