feat: add cudaCapabilities option to NixOS module

[jamesbrink]

Summary

Adds an optional cudaCapabilities setting to the NixOS module that allows users to specify CUDA compute capabilities for their GPUs.

Changes

• Added services.comfyui.cudaCapabilities option (default: null)
• When set, applies to nixpkgs.config.cudaCapabilities globally
• Documented common CUDA capability values with link to NVIDIA docs
• Updated README with usage example
• Fixed platform constraints for CUDA packages (x86_64-linux only)
• Expanded AGENTS.md with comprehensive development guidelines

Implementation Notes

• Default null preserves existing behavior (backwards compatible)
• Setting affects all CUDA packages in the system configuration (documented)
• Pre-built PyTorch wheels already support all architectures, but this allows optimization for packages that honor the setting

Testing

• nix flake check ✅
• nix flake check --all-systems ✅
• nixfmt --check ✅

Closes #19

[claude]

Code Review: cudaCapabilities Option

Thank you for this PR! Overall, this is a well-implemented feature that addresses issue #19. The code quality is good and the documentation is thorough. Here's my detailed review:

✅ Strengths

1. Excellent Documentation: The cudaCapabilities option includes comprehensive inline documentation with common GPU architecture examples and a link to NVIDIA's official documentation. This is very helpful for users.

2. Backwards Compatible: Using null as the default preserves existing behavior perfectly.

3. Clear Transparency: The README and module documentation clearly state that this setting affects the global nixpkgs.config, which is important for users to understand.

4. Proper Platform Constraints: The fix to restrict CUDA packages to x86_64-linux only (instead of all Linux) is correct and improves the code.

5. Comprehensive Testing: The PR description shows all relevant checks were run successfully.

6. AGENTS.md Improvements: The expansion of development guidelines is valuable and well-structured, making it easier for contributors to understand expectations.

🔍 Observations & Considerations

1. Module Implementation (nix/modules/comfyui.nix:242-244)

The implementation is straightforward and correct:

```nix
config = lib.mkIf cfg.enable {
nixpkgs.config = lib.mkIf (cfg.cudaCapabilities != null) {
cudaCapabilities = cfg.cudaCapabilities;
};

...

}
```

Note: This modifies the global nixpkgs configuration. While this is documented, it means:

• It affects all CUDA packages in the system, not just ComfyUI
• It's set inside the service module, which some might find unexpected

Alternative consideration (not a blocking issue): In some NixOS modules, global config changes like this are done at the top-level configuration instead of inside a service module. However, since this is well-documented and the user explicitly opts in, the current approach is acceptable.

2. Impact on Pre-built Wheels

From the code comments and documentation, ComfyUI's CUDA build uses pre-built PyTorch wheels that already support all GPU architectures (Pascal through Hopper). This means:

• Setting cudaCapabilities won't affect ComfyUI's main PyTorch dependency
• It will only affect other nixpkgs-built CUDA packages in the system
• The performance benefit for ComfyUI specifically may be minimal

Suggestion: Consider adding a brief note in the module documentation mentioning that the pre-built PyTorch wheels already support all architectures, so this setting primarily benefits other CUDA packages. For example:

```nix
description = ''
Optional list of CUDA compute capabilities to use for builds that honor
`nixpkgs.config.cudaCapabilities`. When set, this updates the global
nixpkgs configuration, so it affects other CUDA packages too.

Note: ComfyUI uses pre-built PyTorch wheels that already support all GPU
architectures, so this setting primarily optimizes other system CUDA packages.

Example: [ "8.9" ] for Ada Lovelace (RTX 40xx) GPUs.

...

```

3. Platform Constraints Fix (flake.nix:154, 228)

The change from pkgs.stdenv.isLinux to pkgs.stdenv.isLinux && pkgs.stdenv.isx86_64 is excellent and more accurate. CUDA support on ARM Linux is not standard, so this is the right fix.

Minor note: isx86_64 is the correct attribute (not isx86-64 or isAmd64), which you've used correctly.

🐛 Potential Issues

None identified. The code is well-structured and I don't see any bugs or security issues.

🎯 Recommendations

1. Optional Enhancement: Add the clarification note mentioned above about pre-built wheels.

2. Consider Testing Scenario: If you haven't already, you might want to verify that setting this option doesn't cause unexpected behavior when combined with custom nodes that might compile CUDA code.

3. Documentation Consistency: The README table (line 323) could benefit from a slightly longer description, e.g., "Optional CUDA compute capability list (affects all system CUDA packages)" to match the detail level of other options.

📊 Code Quality Assessment

• Style: ✅ Follows Nix style conventions, properly formatted
• Documentation: ✅ Excellent inline documentation and examples
• Testing: ✅ All checks passed
• Security: ✅ No security concerns
• Performance: ✅ Correctly scoped to build-time optimization
• Maintainability: ✅ Clean, simple implementation

🎬 Verdict

LGTM (Looks Good To Me) with minor optional suggestions above. This is a solid feature addition that gives users control over CUDA compute capabilities while maintaining good defaults and clear documentation.

The code is ready to merge. The optional enhancement suggestions are nice-to-haves that could be addressed in this PR or a follow-up, but they don't block merging.

Great work! 🚀

[claude]

Code Review: cudaCapabilities Option

Thanks for this PR! This is a well-implemented feature that addresses issue #19. Here's my detailed review:

✅ Strengths

1. Excellent Documentation: The cudaCapabilities option has comprehensive inline documentation with:

   • Clear examples of common GPU compute capabilities
   • Link to official NVIDIA documentation
   • Important warning about global configuration impact
   • Proper default behavior (null = no change)

2. Backwards Compatible: Using null as the default preserves existing behavior perfectly

3. Proper Platform Constraints: The fix in commit 83bd0ad correctly separates:

   • dockerImage: Available on all Linux platforms (amd64 + arm64)
   • cuda and dockerImageCuda: Restricted to x86_64-linux only

4. Comprehensive AGENTS.md Update: The expanded development guidelines are excellent and will help future contributors

5. Testing: All checks passed (nix flake check on all systems)

🔍 Technical Correctness

The implementation correctly uses lib.mkIf (cfg.cudaCapabilities \!= null) to conditionally set the global nixpkgs config. This is the right approach for an optional feature.

Important Note: Per nixpkgs CUDA documentation, cudaCapabilities only affects packages that honor this setting during compilation. Since this flake uses pre-built PyTorch wheels (as documented in CLAUDE.md), this option won't affect PyTorch itself, but could optimize other CUDA packages in the user's system.

⚠️ Considerations & Suggestions

1. Documentation Clarity

Consider adding a note in the README or module documentation that clarifies:

• Pre-built PyTorch wheels already support all architectures (Pascal through Hopper)
• This setting primarily benefits other system-level CUDA packages
• Setting this affects the entire nixpkgs configuration, not just ComfyUI

Example addition to the module description:

description = ''
  Optional list of CUDA compute capabilities to use for builds that honor
  `nixpkgs.config.cudaCapabilities`. When set, this updates the global
  nixpkgs configuration, so it affects other CUDA packages too.
  
  Note: ComfyUI's pre-built PyTorch wheels already support all GPU
  architectures. This setting is primarily useful for optimizing other
  CUDA-enabled packages in your system configuration.
  
  ...
'';

2. Type Safety

The type is listOf lib.types.str which is correct, but you could add validation to ensure values match the expected format (e.g., "X.Y"). However, this might be over-engineering since nixpkgs will validate it downstream.

3. Alternative Approach (Optional)

For users who want fine-grained control, you might consider documenting that they can override the entire nixpkgs.config in their NixOS configuration instead of using this option. But the current approach is simpler and covers the common use case well.

📝 Minor Suggestions

1. README.md: The table entry is good, but consider adding an example in the configuration snippet showing when to use it:

services.comfyui = {
  enable = true;
  cuda = true;
  # Optional: Optimize CUDA compilation for your specific GPU
  # (affects system-wide CUDA packages, not the pre-built PyTorch wheels)
  cudaCapabilities = [ "8.9" ];  # RTX 40xx series
  ...
};

2. AGENTS.md: The improvements are excellent! No changes needed here.

🎯 Overall Assessment

This is a high-quality PR that:

• Solves the user's problem (issue Setting stdenv? #19)
• Maintains backwards compatibility
• Includes excellent documentation
• Follows the project's conventions
• Passes all checks

The implementation is clean and follows Nix best practices. The only suggestion is to make the documentation slightly more explicit about what this setting does and doesn't affect, given that PyTorch uses pre-built wheels.

Recommendation

Approve with minor documentation enhancement suggestion - The code is ready to merge as-is, but would benefit from the documentation clarification mentioned above.

---

Sources:

• CUDA Documentation | nixpkgs
• cudaCapabilities Issue | nixpkgs
• CUDA - NixOS Wiki

[claude]

Code Review

I've reviewed PR #20 and here's my detailed feedback:

Overall Assessment

✅ Well-executed implementation - This PR adds valuable functionality with thoughtful design decisions and comprehensive documentation.

---

Strengths

1. Backwards Compatibility

• ✅ Default value of null preserves existing behavior perfectly
• ✅ No breaking changes for existing users

2. Clear Documentation

• ✅ Extensive inline comments explaining common CUDA capability values (6.1, 7.0, 7.5, 8.0, 8.6, 8.9, 9.0)
• ✅ Important caveat clearly stated: pre-built PyTorch wheels already support all architectures
• ✅ Links to NVIDIA documentation for reference
• ✅ README updated with usage example and table entry

3. Platform Constraints

• ✅ Properly constrains CUDA packages to x86_64-linux only in flake.nix:157
• ✅ Overlay correctly throws error for unsupported platforms (flake.nix:230-233)

4. Clean Implementation

• ✅ Follows existing patterns in the module
• ✅ Uses proper Nix types (lib.types.nullOr (lib.types.listOf lib.types.str))
• ✅ Configuration properly scoped with lib.mkIf guard (nix/modules/comfyui.nix:246-248)

5. AGENTS.md Improvements

• ✅ Comprehensive expansion of development guidelines
• ✅ Detailed command reference for linting, formatting, and type checking
• ✅ Well-structured coding conventions section
• ✅ Helpful for both AI agents and human contributors

---

Considerations & Questions

1. Global Side Effects ⚠️
The implementation sets nixpkgs.config.cudaCapabilities globally (module line 246-248), which affects all CUDA packages in the system configuration, not just ComfyUI.

Trade-off:

• ✅ Pro: Simple, minimal code, follows Nix conventions
• ⚠️ Con: May surprise users who expect it to only affect ComfyUI
• ✅ Mitigation: This is clearly documented in both the option description and README

Alternative approach (not necessarily better):

# Could create a package override instead
package = lib.mkOption {
  default = if cfg.cuda then 
    (pkgs.comfy-ui-cuda.override { 
      cudaCapabilities = cfg.cudaCapabilities; 
    })
  else pkgs.comfy-ui;
};

Verdict: Current approach is acceptable given the clear documentation. The global setting is actually useful for users with CUDA packages beyond ComfyUI.

2. CUDA Capabilities Value Validation
Currently accepts any string in the list. Consider if validation would help catch typos:

# Optional enhancement
cudaCapabilities = lib.mkOption {
  type = lib.types.nullOr (lib.types.listOf (lib.types.strMatching "[0-9]+\\.[0-9]+"));
  # ...
};

Verdict: Not critical since Nix will fail at build time with invalid values, and flexibility may be needed for future CUDA versions.

3. Interaction with Pre-built PyTorch Wheels
The documentation correctly notes that pre-built PyTorch wheels already support all architectures. This option is most useful for:

• System-wide CUDA package optimization
• Packages built from source that honor cudaCapabilities

Question: Does ComfyUI use any other CUDA packages beyond PyTorch that would benefit from this setting? If not, consider emphasizing this is primarily for "system-wide CUDA optimization" rather than ComfyUI-specific.

---

Nit: Code Quality

Minor suggestion for README.md:334-336
The note about global effects could be slightly more prominent:

+**Important:** `cudaCapabilities` maps to `nixpkgs.config.cudaCapabilities`, so setting it will
-`cudaCapabilities` maps to `nixpkgs.config.cudaCapabilities`, so setting it will

---

Security Considerations

✅ No security concerns identified. The option only affects build-time optimization.

---

Test Coverage

✅ The PR description indicates all checks passed:

• nix flake check ✅
• nix flake check --all-systems ✅
• nixfmt --check ✅

---

Recommendations

Before merge:

1. ✅ All checks passing (confirmed)
2. ✅ Documentation is clear and comprehensive
3. ⚠️ Optional: Consider adding a usage example in CLAUDE.md since it's a new NixOS module option

Post-merge:

1. Consider documenting in release notes that this affects global CUDA configuration
2. Monitor for user feedback on whether package-level override would be preferred

---

Verdict

✅ Approve with minor suggestions

This is a solid PR that:

• Solves a real user need (issue Setting stdenv? #19)
• Maintains backwards compatibility
• Includes excellent documentation
• Follows project conventions
• Has appropriate platform constraints

The global nixpkgs.config side effect is acceptable given:

1. It's clearly documented
2. It follows standard Nix patterns
3. It's actually useful for users with multiple CUDA packages

Suggested merge strategy: Squash or rebase to keep commit history clean, given the comprehensive nature of the changes across multiple files.