Skip to content

Speed up anima a bit on nvidia.#14181

Merged
comfyanonymous merged 2 commits into
masterfrom
comfyanonymous-patch-1
May 30, 2026
Merged

Speed up anima a bit on nvidia.#14181
comfyanonymous merged 2 commits into
masterfrom
comfyanonymous-patch-1

Conversation

@comfyanonymous
Copy link
Copy Markdown
Member

No description provided.

@socket-security
Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedwebsocket-client@​1.9.098100100100100
Updatedcomfy-kitchen@​0.2.9 ⏵ 0.2.10100 +1100100100100 +20
Addedpytest-asyncio@​1.4.0100100100100100

View full report

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 30, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR consolidates rotary positional embedding (RoPE) application in the Cosmos attention mechanism. A local apply_rotary_pos_emb() helper function is removed from predict2.py, and its call sites are replaced with comfy.quant_ops.ck.apply_rope_split_half(q, k, rope_emb), which applies RoPE in a split/paired manner. The comfy-kitchen dependency is bumped from 0.2.9 to 0.2.10 to provide this external function.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title refers to speeding up 'anima' on nvidia, but the actual changes optimize rotary position embedding application and update a dependency version—no 'anima' optimization is evident in the changeset. Revise the title to accurately reflect the main changes, such as 'Optimize rotary position embeddings in Cosmos attention' or 'Switch to split-half RoPE application for performance'.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess relevance to the changeset. Add a description explaining the performance improvements from using split-half RoPE application and the rationale for the comfy-kitchen version bump.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

❤️ Share

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

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

Inline comments:
In `@comfy/ldm/cosmos/predict2.py`:
- Line 167: The call to comfy.quant_ops.ck.apply_rope_split_half(q, k, rope_emb)
can raise AttributeError if comfy.quant_ops hasn't been imported yet; fix it by
adding an explicit import for the module (e.g., add "import comfy.quant_ops" or
"from comfy import quant_ops") near the top of comfy/ldm/cosmos/predict2.py so
that comfy.quant_ops and its attribute ck are initialized before calling
apply_rope_split_half; ensure the import is placed before any use of
comfy.quant_ops.ck or apply_rope_split_half.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3ee48357-c223-4d7f-867a-e1decde205dc

📥 Commits

Reviewing files that changed from the base of the PR and between 6e1ef23 and 88c6e47.

📒 Files selected for processing (2)
  • comfy/ldm/cosmos/predict2.py
  • requirements.txt

if self.is_selfattn and rope_emb is not None: # only apply to self-attention!
q = apply_rotary_pos_emb(q, rope_emb)
k = apply_rotary_pos_emb(k, rope_emb)
q, k = comfy.quant_ops.ck.apply_rope_split_half(q, k, rope_emb)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Expect either an explicit import in this file or a package-level export of comfy.quant_ops."
echo
sed -n '1,20p' comfy/ldm/cosmos/predict2.py
echo
echo "Direct imports related to comfy.quant_ops in this file:"
rg -n --no-heading '^\s*(from\s+comfy\.quant_ops\s+import|import\s+comfy\.quant_ops(?:\.ck)?)' comfy/ldm/cosmos/predict2.py || true
echo
echo "Package-level references to quant_ops in comfy __init__ files:"
fd -t f '__init__\.py$' comfy | xargs -r rg -n --no-heading '\bquant_ops\b' || true
echo
echo "Definitions/usages of apply_rope_split_half in the repo:"
rg -n --no-heading '\bapply_rope_split_half\b' || true

Repository: Comfy-Org/ComfyUI

Length of output: 1033


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Locate comfy/quant_ops (file vs dir):"
ls -la comfy | rg -n 'quant_ops' || true
test -f comfy/quant_ops.py && echo "Found: comfy/quant_ops.py"
test -d comfy/quant_ops && echo "Found: comfy/quant_ops/ (dir)"

echo
echo "Inspect comfy/quant_ops.py for ck / apply_rope_split_half:"

if test -f comfy/quant_ops.py; then
  rg -n --no-heading '\bck\b' comfy/quant_ops.py || true
  rg -n --no-heading '\bapply_rope_split_half\b' comfy/quant_ops.py || true

  # Print the most relevant sections around any mention of ck or apply_rope_split_half
  for pat in 'apply_rope_split_half' '\bck\b'; do
    echo "---- occurrences for pattern: $pat ----"
    rg -n --no-heading -C 3 "$pat" comfy/quant_ops.py || true
  done
else
  echo "comfy/quant_ops.py not found; listing comfy/quant_ops directory if it exists."
  if test -d comfy/quant_ops; then
    ls -la comfy/quant_ops
    rg -n --no-heading '\bapply_rope_split_half\b' comfy/quant_ops || true
    rg -n --no-heading '\bck\b' comfy/quant_ops || true
  fi
fi

echo
echo "Also search for module path comfy.quant_ops.ck (files):"
fd -t f 'ck' comfy | head -n 50 || true
fd -t f 'quant_ops' comfy | head -n 50 || true

echo
echo "Repo-wide search for apply_rope_split_half (case-sensitive, exact identifier):"
rg -n --no-heading '\bapply_rope_split_half\b' . || true

Repository: Comfy-Org/ComfyUI

Length of output: 4069


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "predict2.py direct imports of comfy.quant_ops / comfy.quant_ops.ck:"
rg -n '^\s*(from\s+comfy\.quant_ops\s+import|import\s+comfy\.quant_ops(\.ck)?)' comfy/ldm/cosmos/predict2.py || true
echo

echo "comfy/quant_ops.py: presence of ck from comfy_kitchen:"
rg -n --no-heading 'import\s+comfy_kitchen\s+as\s+ck|_CK_AVAILABLE' comfy/quant_ops.py || true
echo

echo "Repo-wide imports of comfy.quant_ops:"
rg -n --no-heading '^\s*(import\s+comfy\.quant_ops|from\s+comfy\s+import\s+quant_ops)' comfy --glob='*.py' || true
echo

echo "References to quant_ops in comfy __init__.py files:"
fd -t f '__init__\.py$' comfy | xargs -r rg -n --no-heading '\bquant_ops\b' || true

Repository: Comfy-Org/ComfyUI

Length of output: 493


Explicitly import comfy.quant_ops (or fail fast) before comfy.quant_ops.ck.*.

comfy/quant_ops.py defines ck only after import comfy_kitchen as ck, so comfy.quant_ops.ck.apply_rope_split_half(...) still depends on comfy.quant_ops being imported/available on the comfy package at runtime. Some code paths import comfy.quant_ops already (e.g., comfy/model_management.py), but comfy/ldm/cosmos/predict2.py has no direct import to make this guarantee. Add an explicit import comfy.quant_ops (or from comfy import quant_ops) near the top of comfy/ldm/cosmos/predict2.py to avoid AttributeError on clean/alternative import orders.

🤖 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/ldm/cosmos/predict2.py` at line 167, The call to
comfy.quant_ops.ck.apply_rope_split_half(q, k, rope_emb) can raise
AttributeError if comfy.quant_ops hasn't been imported yet; fix it by adding an
explicit import for the module (e.g., add "import comfy.quant_ops" or "from
comfy import quant_ops") near the top of comfy/ldm/cosmos/predict2.py so that
comfy.quant_ops and its attribute ck are initialized before calling
apply_rope_split_half; ensure the import is placed before any use of
comfy.quant_ops.ck or apply_rope_split_half.

@comfyanonymous comfyanonymous merged commit 0b04660 into master May 30, 2026
25 of 36 checks passed
@comfyanonymous comfyanonymous deleted the comfyanonymous-patch-1 branch May 30, 2026 05:47
@m8rr
Copy link
Copy Markdown

m8rr commented May 30, 2026

https://github.com/pamparamm/ComfyUI-ppm/
This PR broke PPM, but I guess it's up to the custom nodes to handle it.
The execution time dropped from 5.6 seconds to 5 seconds.

[WARNING] Traceback (most recent call last):
  File "D:\AI\ComfyUI_windows_portable\ComfyUI\nodes.py", line 2212, in load_custom_node
    module_spec.loader.exec_module(module)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^
  File "<frozen importlib._bootstrap_external>", line 1023, in exec_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "D:\AI\ComfyUI_windows_portable\ComfyUI\custom_nodes\ComfyUI-ppm\__init__.py", line 6, in <module>
    from .src.nodes_ppm import (
    ...<12 lines>...
    )
  File "D:\AI\ComfyUI_windows_portable\ComfyUI\custom_nodes\ComfyUI-ppm\src\nodes_ppm\attention_couple_ppm.py", line 18, in <module>
    from ..dit_patches.cosmos_attention import patch_cosmos_attention
  File "D:\AI\ComfyUI_windows_portable\ComfyUI\custom_nodes\ComfyUI-ppm\src\dit_patches\cosmos_attention.py", line 8, in <module>
    from comfy.ldm.cosmos.predict2 import MiniTrainDIT, apply_rotary_pos_emb
ImportError: cannot import name 'apply_rotary_pos_emb' from 'comfy.ldm.cosmos.predict2' (D:\AI\ComfyUI_windows_portable\ComfyUI\comfy\ldm\cosmos\predict2.py)

[WARNING] Cannot import D:\AI\ComfyUI_windows_portable\ComfyUI\custom_nodes\ComfyUI-ppm module for custom nodes: cannot import name 'apply_rotary_pos_emb' from 'comfy.ldm.cosmos.predict2' (D:\AI\ComfyUI_windows_portable\ComfyUI\comfy\ldm\cosmos\predict2.py)

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