Add slice_cond and per-model context window cond resizing#12645
Add slice_cond and per-model context window cond resizing#12645Kosinkadink merged 6 commits intoComfy-Org:masterfrom
Conversation
drozbay
requested review from
Kosinkadink,
comfyanonymous and
guill
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip Flake8 can be used to improve the quality of Python code reviews.Flake8 is a Python linter that wraps PyFlakes, pycodestyle and Ned Batchelder's McCabe script. To configure Flake8, add a '.flake8' or 'setup.cfg' file to your project root. See Flake8 Documentation for more details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy/context_windows.py`:
- Around line 224-230: Initialize self._model to None in the
IndexListContextHandler.__init__ so get_resized_cond won't raise AttributeError
when called before execute(); specifically, add self._model = None in the
constructor of IndexListContextHandler (the class that defines get_resized_cond)
and ensure any code paths that later set the model in execute() continue to
assign to the same attribute, which will allow the guard "self._model is not
None" to work correctly when calling resize_cond_for_context_window.
Test Evidence CheckIf this PR modifies behavior that requires testing, a test explanation is required. PRs lacking applicable test explanations may not be reviewed until added. Please add test explanations to ensure code quality and prevent regressions. If this PR changes user-facing behavior, visual proof (screen recording or screenshot) is required. PRs without applicable visual documentation may not be reviewed until provided. You can add it by:
|
Necessary for WanAnimate context windows workflow, which needs cond_retain_index_list = 0 to work properly with its reference input.
|
I had to update the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
comfy_extras/nodes_context_windows.py (1)
30-31: Consider addingadvanced=Truefor consistency with the commit description.The commit message mentions "Expose additional advanced inputs," but these new inputs are not marked as
advanced=True. Other similar configuration inputs in this schema (e.g.,context_length,context_overlap,context_stride) useadvanced=True. If these are intended to be advanced/power-user inputs, adding the flag would keep them consistent with the existing pattern and reduce UI clutter for casual users.If they're intentionally non-advanced for discoverability, the current implementation is fine.
✨ Optional: mark as advanced inputs
- io.String.Input("cond_retain_index_list", default="", tooltip="List of latent indices to retain in the conditioning tensors for each window, for example setting this to '0' will use the initial start image for each window."), - io.Boolean.Input("split_conds_to_windows", default=False, tooltip="Whether to split multiple conditionings (created by ConditionCombine) to each window based on region index."), + io.String.Input("cond_retain_index_list", default="", tooltip="List of latent indices to retain in the conditioning tensors for each window, for example setting this to '0' will use the initial start image for each window.", advanced=True), + io.Boolean.Input("split_conds_to_windows", default=False, tooltip="Whether to split multiple conditionings (created by ConditionCombine) to each window based on region index.", advanced=True),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_extras/nodes_context_windows.py` around lines 30 - 31, The new inputs "cond_retain_index_list" (io.String.Input) and "split_conds_to_windows" (io.Boolean.Input) should be marked as advanced to match the pattern used for other context parameters; update the Input declarations for cond_retain_index_list and split_conds_to_windows to include advanced=True so they appear in the advanced UI group like context_length/context_overlap/context_stride.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@comfy_extras/nodes_context_windows.py`:
- Around line 30-31: The new inputs "cond_retain_index_list" (io.String.Input)
and "split_conds_to_windows" (io.Boolean.Input) should be marked as advanced to
match the pattern used for other context parameters; update the Input
declarations for cond_retain_index_list and split_conds_to_windows to include
advanced=True so they appear in the advanced UI group like
context_length/context_overlap/context_stride.
|
Here is a workflow that demonstrates this functionality with WanAnimate. droz_Wan22_Animate_ContextWindows_Native_vs_Wrapper.json The source video can be found here: https://www.pexels.com/video/woman-performing-choreograph-6616345/ Reference image: Output example: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy/model_base.py`:
- Around line 1437-1442: The override resize_cond_for_context_window is dropping
the retain_index_list when delegating audio_embed (and other model-specific)
keys to comfy.context_windows.slice_cond; update the calls in
resize_cond_for_context_window (for cond_key "audio_embed" and the other
model-specific branches at the referenced locations) to pass the
retain_index_list through to comfy.context_windows.slice_cond (add
retain_index_list=retain_index_list or positional arg as appropriate) so that
cond_retain_index_list behavior is preserved and matches the superclass path.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
comfy/context_windows.pycomfy/model_base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- comfy/context_windows.py
3 participants
Adds a
slice_cond()helper incontext_windows.pyand a newresize_cond_for_context_window()method onBaseModelthat lets each model subclass declare how its conditionings should be sliced during context windowing.The context window handler calls the model method before falling back to the existing generic slicing logic, so models with non-standard temporal dimensions or offsets can handle them explicitly:
Implements overrides for WAN21_Vace, WAN21_HuMo, WAN22_Animate, and WAN22_S2V.
Also fixed a small mostly benign bug:
cond_value(self.dim)tocond_value.size(self.dim)