Prevent conditioning fill from indexing empty regional destinations - #16477
perfloop-agent wants to merge 2 commits into
Conversation
|
✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged. |
|
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 configurationConfiguration used: Repository: Comfy-Org/ComfyUI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | 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
Comment @coderabbitai help to get the list of available commands.
|
I have read and agree to the Contributor License Agreement |
|
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 |
apply_empty_x_to_equal_areanow 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.pyandruff 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).