Inference module to utilize CFZ CUDNN Toggle Node in ComfyUI-Zluda #1440
Conversation
deleted unneeded file
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
Summary of ChangesHello @NeuralFault, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates a new CUDNN toggle module into the StabilityMatrix inference system, specifically targeting ComfyUI-Zluda users. The primary goal is to resolve a RuntimeError that occurs when MIOpen conflicts with CUDNN during VAE decoding on certain hardware configurations, such as AMD cards. The module provides a user-facing toggle to control CUDNN enablement and benchmarking, ensuring smoother operation and preventing workflow interruptions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
I have read the CLA Document and I hereby sign the CLA |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new inference module to toggle cuDNN settings, which is a useful workaround for users on specific hardware configurations (AMD with Zluda). The implementation is straightforward and adds the necessary UI, ViewModel, and module logic.
My review focuses on a few key areas:
- Connecting UI to Logic: There's a disconnect between the UI controls in the new
CfzCudnnToggleCardand theCfzCudnnToggleModulelogic, which currently uses hardcoded values. I've suggested wiring them up to make the feature fully configurable as the UI implies. - Code Consistency: I've pointed out a minor naming inconsistency in the new ComfyUI node definition to align it with existing conventions in the codebase.
- Maintainability: I've also made a few suggestions to improve code organization by sorting attribute and type lists alphabetically.
Overall, this is a good addition. Addressing these points will improve the code's correctness and maintainability.
| using System.Linq; | ||
| using Injectio.Attributes; | ||
| using StabilityMatrix.Avalonia.Models.Inference; | ||
| using StabilityMatrix.Avalonia.Services; | ||
| using StabilityMatrix.Avalonia.ViewModels.Base; | ||
| using StabilityMatrix.Avalonia.ViewModels.Inference; | ||
| using StabilityMatrix.Core.Attributes; | ||
| using StabilityMatrix.Core.Models.Api.Comfy.Nodes; | ||
| using StabilityMatrix.Core.Models.Api.Comfy.NodeTypes; | ||
| using StabilityMatrix.Core.Models.Inference; | ||
|
|
||
| namespace StabilityMatrix.Avalonia.ViewModels.Inference.Modules; | ||
|
|
||
| [ManagedService] | ||
| [RegisterTransient<CfzCudnnToggleModule>] | ||
| public class CfzCudnnToggleModule : ModuleBase | ||
| { | ||
| /// <inheritdoc /> | ||
| public CfzCudnnToggleModule(IServiceManager<ViewModelBase> vmFactory) | ||
| : base(vmFactory) | ||
| { | ||
| Title = "Disable CUDNN (ComfyUI-Zluda)"; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Applies CUDNN Toggle node between sampler latent output and VAE decode | ||
| /// This prevents "GET was unable to find an engine" errors on AMD cards with Zluda | ||
| /// </summary> | ||
| protected override void OnApplyStep(ModuleApplyStepEventArgs e) | ||
| { | ||
| // Get the primary connection (can be latent or image) | ||
| var primary = e.Builder.Connections.Primary; | ||
| if (primary == null) | ||
| { | ||
| return; // No primary connection to process | ||
| } | ||
|
|
||
| // Check if primary is a latent (from sampler output) | ||
| if (primary.IsT0) // T0 is LatentNodeConnection | ||
| { | ||
| var latentConnection = primary.AsT0; | ||
|
|
||
| // Insert CUDNN toggle node between sampler and VAE decode | ||
| var cudnnToggleOutput = e.Nodes.AddTypedNode( | ||
| new ComfyNodeBuilder.CUDNNToggleAutoPassthrough | ||
| { | ||
| Name = e.Nodes.GetUniqueName("CUDNNToggle"), | ||
| Model = null, | ||
| Conditioning = null, | ||
| Latent = latentConnection, // Pass through the latent from sampler | ||
| enable_cudnn = false, | ||
| cudnn_benchmark = false, | ||
| } | ||
| ); | ||
|
|
||
| // Update the primary connection to use the CUDNN toggle latent output (Output3) | ||
| // This ensures VAE decode receives latent from CUDNN toggle instead of directly from sampler | ||
| e.Builder.Connections.Primary = cudnnToggleOutput.Output3; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The module currently hardcodes enable_cudnn and cudnn_benchmark to false, which doesn't use the values from CfzCudnnToggleCardViewModel. This makes the UI controls for these settings ineffective.
To fix this, you should:
- In the
CfzCudnnToggleModuleconstructor, add theCfzCudnnToggleCardViewModelas a card to the module. - Update
OnApplyStepto retrieve the view model and use its properties (EnableCudnnandCudnnBenchmark) when creating theCUDNNToggleAutoPassthroughnode. - Consider renaming the module
Titleto "CUDNN Toggle (ComfyUI-Zluda)" to better reflect that it can both enable and disable CUDNN settings. - The
using System.Linq;statement is not used and can be removed.
I've included a code suggestion that implements these changes. Note that it uses PascalCase property names (EnableCudnn, CudnnBenchmark) for the CUDNNToggleAutoPassthrough node, which corresponds to another suggestion I've made for ComfyNodeBuilder.cs.
using Injectio.Attributes;
using StabilityMatrix.Avalonia.Models.Inference;
using StabilityMatrix.Avalonia.Services;
using StabilityMatrix.Avalonia.ViewModels.Base;
using StabilityMatrix.Avalonia.ViewModels.Inference;
using StabilityMatrix.Core.Attributes;
using StabilityMatrix.Core.Models.Api.Comfy.Nodes;
using StabilityMatrix.Core.Models.Api.Comfy.NodeTypes;
using StabilityMatrix.Core.Models.Inference;
namespace StabilityMatrix.Avalonia.ViewModels.Inference.Modules;
[ManagedService]
[RegisterTransient<CfzCudnnToggleModule>]
public class CfzCudnnToggleModule : ModuleBase
{
/// <inheritdoc />
public CfzCudnnToggleModule(IServiceManager<ViewModelBase> vmFactory)
: base(vmFactory)
{
Title = "CUDNN Toggle (ComfyUI-Zluda)";
AddCards(vmFactory.Get<CfzCudnnToggleCardViewModel>());
}
private CfzCudnnToggleCardViewModel Card => GetCard<CfzCudnnToggleCardViewModel>();
/// <summary>
/// Applies CUDNN Toggle node between sampler latent output and VAE decode
/// This prevents "GET was unable to find an engine" errors on AMD cards with Zluda
/// </summary>
protected override void OnApplyStep(ModuleApplyStepEventArgs e)
{
// Get the primary connection (can be latent or image)
var primary = e.Builder.Connections.Primary;
if (primary == null)
{
return; // No primary connection to process
}
// Check if primary is a latent (from sampler output)
if (primary.IsT0) // T0 is LatentNodeConnection
{
var latentConnection = primary.AsT0;
// Insert CUDNN toggle node between sampler and VAE decode
var cudnnToggleOutput = e.Nodes.AddTypedNode(
new ComfyNodeBuilder.CUDNNToggleAutoPassthrough
{
Name = e.Nodes.GetUniqueName("CUDNNToggle"),
Model = null,
Conditioning = null,
Latent = latentConnection, // Pass through the latent from sampler
EnableCudnn = Card.EnableCudnn,
CudnnBenchmark = Card.CudnnBenchmark,
}
);
// Update the primary connection to use the CUDNN toggle latent output (Output3)
// This ensures VAE decode receives latent from CUDNN toggle instead of directly from sampler
e.Builder.Connections.Primary = cudnnToggleOutput.Output3;
}
}
}| [JsonDerivedType(typeof(RescaleCfgCardViewModel), RescaleCfgCardViewModel.ModuleKey)] | ||
| [JsonDerivedType(typeof(PlasmaNoiseCardViewModel), PlasmaNoiseCardViewModel.ModuleKey)] | ||
| [JsonDerivedType(typeof(NrsCardViewModel), NrsCardViewModel.ModuleKey)] | ||
| [JsonDerivedType(typeof(CfzCudnnToggleCardViewModel), CfzCudnnToggleCardViewModel.ModuleKey)] |
There was a problem hiding this comment.
To improve code organization and maintainability, please keep [JsonDerivedType] attributes sorted alphabetically by the type name. CfzCudnnToggleCardViewModel should be placed before ControlNetCardViewModel.
| [JsonDerivedType(typeof(RescaleCfgModule))] | ||
| [JsonDerivedType(typeof(PlasmaNoiseModule))] | ||
| [JsonDerivedType(typeof(NRSModule))] | ||
| [JsonDerivedType(typeof(CfzCudnnToggleModule))] |
There was a problem hiding this comment.
To improve code organization and maintainability, please keep [JsonDerivedType] attributes sorted alphabetically by the type name. CfzCudnnToggleModule should be placed before ControlNetModule.
| typeof(FluxHiresFixModule), | ||
| typeof(UpscalerModule), | ||
| typeof(CfzCudnnToggleModule), | ||
| typeof(SaveImageModule), | ||
| typeof(FaceDetailerModule) | ||
| typeof(FaceDetailerModule), |
There was a problem hiding this comment.
For better readability and maintainability, please keep the list of available modules sorted alphabetically.
typeof(CfzCudnnToggleModule),
typeof(FaceDetailerModule),
typeof(FluxHiresFixModule),
typeof(SaveImageModule),
typeof(UpscalerModule),| typeof(HiresFixModule), | ||
| typeof(UpscalerModule), | ||
| typeof(CfzCudnnToggleModule), | ||
| typeof(SaveImageModule), | ||
| typeof(FaceDetailerModule) | ||
| typeof(FaceDetailerModule), |
There was a problem hiding this comment.
For better readability and maintainability, please keep the list of available modules sorted alphabetically.
typeof(CfzCudnnToggleModule),
typeof(FaceDetailerModule),
typeof(HiresFixModule),
typeof(SaveImageModule),
typeof(UpscalerModule),
StabilityMatrix.Core/Models/Api/Comfy/Nodes/ComfyNodeBuilder.cs
Outdated
Show resolved
Hide resolved
|
recheck |
|
closing as needed to rebase, fix author issues and pushed to a new branch. |
3 participants
Adds a module to SM Inference's optional Steps listing. This, when toggled on, will add the CFZ Cudnn Toggle node in ComfyUI-Zluda to SM Inference's workflow disabling Cudnn before VAE Decode to prevent "RuntimeError: GET was unable to find an engine to execute this computation" when MIOpen kicks in.
Is only a simple toggle that needs to be placed above FaceDetailer (since it itself invokes a conv2d function) in Inference's parameters, but otherwise to be the last Step module at the bottom of the list.