Add output socket to save nodes#13866
Conversation
alexisrolland
requested review from
Kosinkadink,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates save and preview nodes to declare and return their primary output data alongside UI payloads. Audio nodes (SaveAudio, SaveAudioMP3, SaveAudioOpus, PreviewAudio) now expose an "audio" output and return the input audio with UI payloads. Image nodes (SaveAnimatedWEBP, SaveAnimatedPNG, SaveImageAdvanced, SaveSVGNode) return images/svg alongside UI payloads and remove legacy aliases. SaveWEBM/SaveVideo declare explicit outputs and return their input media with UI payloads. Core nodes SaveLatent and SaveImage now declare return types/names and return the original data in result tuples alongside UI payloads. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Feedback from @comfyanonymous
|
alexisrolland
changed the title
|
Bringing back this PR as per our discussion with @guill. This would be a more straightforward way to do pass through. |
…exis/add_output_save_nodes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
comfy_extras/nodes_images.py (2)
197-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the legacy
save_images/save_svgaliases for now.This refactor removes the old entrypoint aliases from these save nodes, but the rest of
comfy_extras/nodes_images.pystill keeps the same compatibility pattern (crop = execute,repeat = execute, etc.). Dropping the aliases here makes these nodes a breaking interface change for older/custom integrations that still resolvesave_imagesorsave_svg.Suggested compatibility shim
class SaveAnimatedWEBP(IO.ComfyNode): @@ def execute(cls, images, fps, filename_prefix, lossless, quality, method, num_frames=0) -> IO.NodeOutput: return IO.NodeOutput( images, ui=UI.ImageSaveHelper.get_save_animated_webp_ui( images=images, filename_prefix=filename_prefix, cls=cls, fps=fps, lossless=lossless, quality=quality, method=cls.COMPRESS_METHODS.get(method) ) ) + + save_images = execute # TODO: remove @@ class SaveAnimatedPNG(IO.ComfyNode): @@ def execute(cls, images, fps, compress_level, filename_prefix="ComfyUI") -> IO.NodeOutput: return IO.NodeOutput( images, ui=UI.ImageSaveHelper.get_save_animated_png_ui( images=images, filename_prefix=filename_prefix, cls=cls, fps=fps, compress_level=compress_level, ) ) + + save_images = execute # TODO: remove @@ class SaveSVGNode(IO.ComfyNode): @@ def execute(cls, svg: IO.SVG.Type, filename_prefix="svg/ComfyUI") -> IO.NodeOutput: @@ return IO.NodeOutput(svg, ui={"images": results}) + + save_svg = execute # TODO: removeAs per coding guidelines,
comfy_extras/**: Community-contributed extra nodes. Focus on: ... No breaking changes to existing node interfaces.Also applies to: 236-266, 497-566
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_extras/nodes_images.py` around lines 197 - 233, The SaveAnimatedWEBP node removed legacy entrypoint aliases; to preserve backward compatibility add legacy names that point to the existing execute implementation (e.g., append at class scope: save_images = execute and save_svg = execute where appropriate) so older integrations can call SaveAnimatedWEBP.save_images(...) or SaveAnimatedWEBP.save_svg(...) and get the same behavior; locate the class SaveAnimatedWEBP and add these alias assignments (and mirror the same pattern for other affected classes/sections mentioned) rather than removing or renaming the execute entrypoint.
520-566:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRewind the SVG streams before passing
svgdownstream.
svg_bytes.read()leaves each buffer at EOF. That was harmless when this node returned only UI metadata, but with the new passthrough output the downstream node can now receive empty SVG streams unless it manually seeks back to0.Suggested fix
for batch_number, svg_bytes in enumerate(svg.data): filename_with_batch_num = filename.replace("%batch_num%", str(batch_number)) file = f"{filename_with_batch_num}_{counter:05}_.svg" # Read SVG content svg_bytes.seek(0) svg_content = svg_bytes.read().decode('utf-8') + svg_bytes.seek(0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_extras/nodes_images.py` around lines 520 - 566, The SVG byte streams are left at EOF after calling svg_bytes.read(), so downstream nodes receive empty streams; in execute (method execute of the node) rewind each buffer in svg.data back to position 0 after reading/modifying (e.g., call svg_bytes.seek(0)) so that the returned svg (NodeOutput(svg, ...)) contains seeked-to-start streams; ensure this is done for every svg_bytes in the loop (or after the loop) so passthrough consumers get readable streams.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@comfy_extras/nodes_images.py`:
- Around line 197-233: The SaveAnimatedWEBP node removed legacy entrypoint
aliases; to preserve backward compatibility add legacy names that point to the
existing execute implementation (e.g., append at class scope: save_images =
execute and save_svg = execute where appropriate) so older integrations can call
SaveAnimatedWEBP.save_images(...) or SaveAnimatedWEBP.save_svg(...) and get the
same behavior; locate the class SaveAnimatedWEBP and add these alias assignments
(and mirror the same pattern for other affected classes/sections mentioned)
rather than removing or renaming the execute entrypoint.
- Around line 520-566: The SVG byte streams are left at EOF after calling
svg_bytes.read(), so downstream nodes receive empty streams; in execute (method
execute of the node) rewind each buffer in svg.data back to position 0 after
reading/modifying (e.g., call svg_bytes.seek(0)) so that the returned svg
(NodeOutput(svg, ...)) contains seeked-to-start streams; ensure this is done for
every svg_bytes in the loop (or after the loop) so passthrough consumers get
readable streams.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da1535e8-33ff-4571-8dd3-e483a659411e
📒 Files selected for processing (2)
comfy_extras/nodes_images.pycomfy_extras/nodes_video.py
1 participant
Add output socket to various save nodes to facilitate pipelining.