Skip to content

Conversation

dseditor
Copy link

@dseditor dseditor commented Jul 9, 2025

Added support for MagCache. It has been tested locally link (custom server) with ComfyUI. MagCache is commonly used in video models such as FramePack and Wan, and it also supports models like Flux and Flux Kontext. This support includes updates to nodes and adjustments to detailed parameters.

https://github.com/Zehong-Ma/ComfyUI-MagCache

@Acly
Copy link
Owner

Acly commented Jul 12, 2025

Looks interesting.

I have two requests:

  1. Show only one button (enable/disable) OR one slider (strength). I don't think all the parameters should be exposed, unless there is a good reason to tweak them for certain scenarios. Default values for the models can be passed to the node.
  2. There should be no need for PerformanceSettings in workflow.py? The parameter is already part of CheckpointInput. Please handle MagCache in the existing load_checkpoint_with_lora function (similar to WaveSpeed), don't introduce a new one.

# Apply MagCache as a model patch if enabled
if checkpoint.magcache_enabled:
if arch in [Arch.flux, Arch.flux_k]:
print(f"Applying MagCache patch for {arch}")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove all the prints


model_type = "flux_kontext" if arch is Arch.flux_k else "flux"

try:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to wrap in try/except, this should not fail (or do so loudly if the code is broken)

else ""
)

has_wave_speed = hasattr(client.features, 'wave_speed') and client.features.wave_speed
Copy link
Owner

@Acly Acly Jul 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the hasattr is not needed

(see also below)

@@ -64,7 +63,12 @@ class CheckpointInput:
self_attention_guidance: bool = False
dynamic_caching: bool = False
tiled_vae: bool = False

magcache_enabled: bool = False
magcache_thresh: float = 0.24
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the parameters also from here and settings.py.

They can be dug up from git if ever needed, but I'd rather not have them in the code without a good reason.

And if more configurability is desired at some point, a single "strength" value or some preset would be a more user friendly way to control it anyway.

@dseditor
Copy link
Author

dseditor commented Jul 13, 2025

Thanks for Acly's detailed review. I'll try to address these issues. I think we might keep a threshold setting to accommodate models that will use MagCache in the future, but clean up unnecessary code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants