Fix Connection of Primitive nodes to Subgraph node #5024
Conversation
|
AustinMroz
force-pushed
the
austin/subgraph-primitive-widgets
branch
from
f687e1c to
5c7cf93
Compare
🎭 Playwright Test Results✅ Tests completed successfully! ⏰ Completed at: 09/11/2025, 01:50:56 PM UTC 📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
dosubot
bot
added
the
size:XS
Medium hackyness, but this saves ~100 lines.
AustinMroz
force-pushed
the
austin/subgraph-primitive-widgets
branch
from
5c7cf93 to
55063c1
Compare
dosubot
bot
added
size:S
|
Do you know about this github feature Austin https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests? |
|
I do! I'm quite a fan of it and have made use of it in most of the PRs I open. In this case, I didn't have a full fix when I opened the PR as draft and forgot to edit it when I returned to the issue. I try to err on the side of caution with declaring an issue fixed when a PR is only a partial solution. |
|
Anything blocking on this PR? |
|
All the feedback was nonblocking nits. With webfiltered's beautiful PR (and a forgotten warning comment), it's solidly GTM. |
* Fix connection of primitives to subgraphNodes * Fix loading and nested subgraphs with primitives Medium hackyness, but this saves ~100 lines. * Use improved type check * Remove requirement for type assertion * Add warning comment --------- Co-authored-by: filtered <[email protected]>
#5024 added support for connecting primitive nodes to subgraph inputs. To accomplish this, it pulls WidgetLocator information from the node owning the widget. This `node` property does not exist on all IBaseWidget. `toConcrete` was used to instead have a BaseWidget which is guaranteed to have a node property. The issue that was missed, is that a widget which lacks this information (such as most implemented by custom nodes) sets the node value to the argument which was passed. Here that is the reference to the subgraph node. Sometimes, this `#setWidget` call is made multiple times, and when this occurs, the `input.widget` has itself set as the protoyep, throwing an error. This is resolved by instead taking an additional input which is unambiguous. For reference, this is a near minimal workflow using comfy_mtb that replicates the issue [cyclic.json](https://github.com/user-attachments/files/22412187/cyclic.json) Special thanks to @melMass for assistance discovering this issue. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5637-Fix-cyclic-prototype-errors-with-subgraphNodes-2726d73d365081fea356f5197e4c2b42) by [Unito](https://www.unito.io)
Had a clever idea which greatly simplifies the logic of nesting here. This patch is now entirely usable, and, in my opinion, worth releasing, but I've noticed a little spottyness with testing of swapped connections to partially matching types (FLOAT to FLOAT with different max/min)
Resolves #5000
┆Issue is synchronized with this Notion page by Unito