Skip to content

Prevent conditioning fill from indexing empty regional destinations - #16477

Open
perfloop-agent wants to merge 2 commits into
Comfy-Org:masterfrom
perfloop:perfloop-pr-open-9rmq62k04e
Open

perfloop-agent wants to merge 2 commits into
Comfy-Org:masterfrom
perfloop:perfloop-pr-open-9rmq62k04e

Conversation

@perfloop-agent

Copy link
Copy Markdown

apply_empty_x_to_equal_area now returns when the destination conditioning family has no global entry. This prevents all-regional negative conditioning from reaching modulo-by-zero while leaving the existing global GLIGEN and control fill loop unchanged.

The regression tests call the helper directly. They cover both GLIGEN and control with two regional destinations and verify that area, metadata, and object identity remain unchanged. They also retain the three-source/two-destination C/B mapping and verify that an existing global destination is not overwritten.

Verified with pytest -q tests-unit/comfy_test/samplers_conditioning_test.py and ruff check --no-cache .. The retained graph replay used a synthetic model to exercise prompt execution, KSampler, Euler, and GLIGEN paths; it was not real-checkpoint image generation. A writable full-suite run exited zero, while an isolated replay excluded two frontend tests that separately failed on proxy DNS.

Assertion repaired: comparison violated it and final source satisfied it: During CFGGuider conditioning preprocessing, a conditioning family with global GLIGEN or control source payloads and only regional destination entries must complete without adding a global destination, preserve every regional destination's area and metadata identity, and retain the existing global-destination fill mapping and non-overwrite behavior.

Checks: 2 passed.


Generated by Perfloop. Human sponsor: Tomás Senart. Measurements and checks. Reproduce this comparison (tenant access required).

@github-actions

github-actions Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged.
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: Comfy-Org/ComfyUI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 29722a10-3fda-42b7-b835-2f1e6fdc6e8b

📥 Commits

Reviewing files that changed from the base of the PR and between 95539f5 and e0d4ea7.

📒 Files selected for processing (2)
  • comfy/samplers.py
  • tests-unit/comfy_test/samplers_conditioning_test.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📜 Recent review details
⚠️ CI failures not shown inline (2)

GitHub Actions: CLA Assistant / 0_cla-assistant.txt: Prevent conditioning fill from indexing empty regional destinations

Conclusion: failure

View job details

##[group]Run contributor-assistant/github-action@ca4a40a7d1004f18d9960b404b97e5f30a505a08
 with:
   lock-pullrequest-aftermerge: false
   path-to-document: https://github.com/Comfy-Org/comfy-cla/blob/main/comfyui_icla.md
   remote-organization-name: comfy-org
   remote-repository-name: comfy-cla
   path-to-signatures: signatures/cla.json
   branch: main
   allowlist: action@github.com,actions-user,ampagent,claude,comfy-pr-bot,GitHub Action,github-actions,github-actions[bot],Glary Bot,Glary-Bot,*[bot]
   custom-notsigned-prcomment: 🎉 Thank you for your contribution, we really appreciate it! 🎉
Like many open source projects, we require contributors to sign our [Contributor License Agreement (CLA)](https://github.com/Comfy-Org/comfy-cla/blob/main/comfyui_icla.md). A CLA makes the ownership of contributions explicit, so contributors and the project share a clear understanding of how the code can be used. By signing, you:
- Confirm that you own your contribution.
- Keep the right to reuse your own code.
- Grant us a copyright license to include and share it within our projects.
CLAs are standard practice across major open source projects including those under the Apache Software Foundation and the Linux Foundation. Ours is based on the Apache Software Foundation's CLA. Most importantly, it would enable us to relicense the project under a more permissive license in the future, giving the project and its community greater flexibility.
✍ **To sign, please post a new comment on this PR with exactly the following text:** ✍
   custom-pr-sign-comment: I have read and agree to the Contributor License Agreement
   custom-allsigned-prcomment: ✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged.
   use-dco-flag: false
   suggest-recheck: true
 env:
   GITHUB_***REDACTED_SECRET_ASSIGNMENT***
   PERSONAL_ACCESS_***REDACTED_SECRET_ASSIGNMENT***
 ##[endgroup]
 CLA Assistant GitHub Action bot has started the process
 (node:2338) [DEP0040] DeprecationWar...

GitHub Actions: CLA Assistant / cla-assistant: Prevent conditioning fill from indexing empty regional destinations

Conclusion: failure

View job details

##[group]Run contributor-assistant/github-action@ca4a40a7d1004f18d9960b404b97e5f30a505a08
 with:
   lock-pullrequest-aftermerge: false
   path-to-document: https://github.com/Comfy-Org/comfy-cla/blob/main/comfyui_icla.md
   remote-organization-name: comfy-org
   remote-repository-name: comfy-cla
   path-to-signatures: signatures/cla.json
   branch: main
   allowlist: action@github.com,actions-user,ampagent,claude,comfy-pr-bot,GitHub Action,github-actions,github-actions[bot],Glary Bot,Glary-Bot,*[bot]
   custom-notsigned-prcomment: 🎉 Thank you for your contribution, we really appreciate it! 🎉
Like many open source projects, we require contributors to sign our [Contributor License Agreement (CLA)](https://github.com/Comfy-Org/comfy-cla/blob/main/comfyui_icla.md). A CLA makes the ownership of contributions explicit, so contributors and the project share a clear understanding of how the code can be used. By signing, you:
- Confirm that you own your contribution.
- Keep the right to reuse your own code.
- Grant us a copyright license to include and share it within our projects.
CLAs are standard practice across major open source projects including those under the Apache Software Foundation and the Linux Foundation. Ours is based on the Apache Software Foundation's CLA. Most importantly, it would enable us to relicense the project under a more permissive license in the future, giving the project and its community greater flexibility.
✍ **To sign, please post a new comment on this PR with exactly the following text:** ✍
   custom-pr-sign-comment: I have read and agree to the Contributor License Agreement
   custom-allsigned-prcomment: ✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged.
   use-dco-flag: false
   suggest-recheck: true
 env:
   GITHUB_***REDACTED_SECRET_ASSIGNMENT***
   PERSONAL_ACCESS_***REDACTED_SECRET_ASSIGNMENT***
 ##[endgroup]
 CLA Assistant GitHub Action bot has started the process
 (node:2338) [DEP0040] DeprecationWar...
🧰 Additional context used
📓 Path-based instructions (3)
Core ML/diffusion engine.

⚙️ CodeRabbit configuration file

Files:

  • comfy/samplers.py
IMPORTANT: Only comment on issues directly introduced by this PR's code changes.

⚙️ CodeRabbit configuration file

Files:

  • comfy/samplers.py
  • tests-unit/comfy_test/samplers_conditioning_test.py
Documentation and README edits should be concise, factual, and tied to the changed behavior.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • comfy/samplers.py
  • tests-unit/comfy_test/samplers_conditioning_test.py
🔇 Additional comments (2)
comfy/samplers.py (1)

922-922: LGTM!

tests-unit/comfy_test/samplers_conditioning_test.py (1)

12-79: LGTM!


📝 Walkthrough

Walkthrough

apply_empty_x_to_equal_area now returns when no unconditioned entries remain without the named control or gligen key. This prevents indexing an empty list. New parametrized tests cover regional entries, round-robin global filling, and preservation of existing global payloads.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to e0d4e

The sampler now safely handles regional conditioning without indexing an empty source list, while preserving existing conditioning entries. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main fix: preventing conditioning fill from indexing empty regional destinations.
Description check ✅ Passed The description accurately explains the code change, regression coverage, validation steps, and intended conditioning behavior.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Warning

⚠️ This pull request has been flagged as potential spam (promotional) by CodeRabbit slop detection and should be reviewed carefully.


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

@perfloop-agent

Copy link
Copy Markdown
Author

I have read and agree to the Contributor License Agreement

comfy-legal added a commit to Comfy-Org/comfy-cla that referenced this pull request Sep 22, 2026
@tsenart

tsenart commented Sep 22, 2026

Copy link
Copy Markdown

Hi, I'm Tomás, I build Perfloop. I approved this contribution and will handle review feedback here.

We've had performance PRs merged in ClickHouse, Prometheus, Ruff/ty, parquet-go, RoaringBitmap, and more. This one comes from the same loop, pointed at correctness. In practice, a graph with ConditioningSetArea on the negative and GLIGENTextBoxApply on the positive stops at KSampler with the modulo by zero. The workflows here only need an approve-and-run.

This branch has not been deployed

No deployments
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