mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 06:20:11 +00:00
[backport core/1.41] fix: configure nested subgraph definitions in dependency order (#10315)
Backport of #10314 to `core/1.41` Automatically created by backport workflow. Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
110
browser_tests/tests/subgraphNestedConfigureOrder.spec.ts
Normal file
110
browser_tests/tests/subgraphNestedConfigureOrder.spec.ts
Normal file
@@ -0,0 +1,110 @@
|
||||
import { expect } from '@playwright/test'
|
||||
|
||||
import { comfyPageFixture as test } from '../fixtures/ComfyPage'
|
||||
|
||||
test.describe('Nested subgraph configure order', { tag: ['@subgraph'] }, () => {
|
||||
const WORKFLOW = 'subgraphs/subgraph-nested-duplicate-ids'
|
||||
|
||||
test('Loads without "No link found" or "Failed to resolve legacy -1" console warnings', async ({
|
||||
comfyPage
|
||||
}) => {
|
||||
const warnings: string[] = []
|
||||
comfyPage.page.on('console', (msg) => {
|
||||
const text = msg.text()
|
||||
if (
|
||||
text.includes('No link found') ||
|
||||
text.includes('Failed to resolve legacy -1') ||
|
||||
text.includes('No inner link found')
|
||||
) {
|
||||
warnings.push(text)
|
||||
}
|
||||
})
|
||||
|
||||
await comfyPage.workflow.loadWorkflow(WORKFLOW)
|
||||
|
||||
expect(warnings).toEqual([])
|
||||
})
|
||||
|
||||
test('All three subgraph levels resolve promoted widgets', async ({
|
||||
comfyPage
|
||||
}) => {
|
||||
await comfyPage.workflow.loadWorkflow(WORKFLOW)
|
||||
await comfyPage.nextFrame()
|
||||
|
||||
const results = await comfyPage.page.evaluate(() => {
|
||||
const graph = window.app!.canvas.graph!
|
||||
const allGraphs = [graph, ...graph.subgraphs.values()]
|
||||
|
||||
return allGraphs.flatMap((g) =>
|
||||
g._nodes
|
||||
.filter(
|
||||
(n) => typeof n.isSubgraphNode === 'function' && n.isSubgraphNode()
|
||||
)
|
||||
.map((hostNode) => {
|
||||
const proxyWidgets = Array.isArray(
|
||||
hostNode.properties?.proxyWidgets
|
||||
)
|
||||
? hostNode.properties.proxyWidgets
|
||||
: []
|
||||
|
||||
const widgetEntries = proxyWidgets
|
||||
.filter(
|
||||
(e: unknown): e is [string, string] =>
|
||||
Array.isArray(e) &&
|
||||
e.length >= 2 &&
|
||||
typeof e[0] === 'string' &&
|
||||
typeof e[1] === 'string'
|
||||
)
|
||||
.map(([interiorNodeId, widgetName]: [string, string]) => {
|
||||
const sg = hostNode.isSubgraphNode() ? hostNode.subgraph : null
|
||||
const interiorNode = sg?.getNodeById(Number(interiorNodeId))
|
||||
return {
|
||||
interiorNodeId,
|
||||
widgetName,
|
||||
resolved: interiorNode !== null && interiorNode !== undefined
|
||||
}
|
||||
})
|
||||
|
||||
return {
|
||||
hostNodeId: String(hostNode.id),
|
||||
widgetEntries
|
||||
}
|
||||
})
|
||||
)
|
||||
})
|
||||
|
||||
expect(
|
||||
results.length,
|
||||
'Should have subgraph host nodes at multiple nesting levels'
|
||||
).toBeGreaterThanOrEqual(2)
|
||||
|
||||
for (const { hostNodeId, widgetEntries } of results) {
|
||||
expect(
|
||||
widgetEntries.length,
|
||||
`Host node ${hostNodeId} should have promoted widgets`
|
||||
).toBeGreaterThan(0)
|
||||
|
||||
for (const { interiorNodeId, widgetName, resolved } of widgetEntries) {
|
||||
expect(interiorNodeId).not.toBe('-1')
|
||||
expect(Number(interiorNodeId)).toBeGreaterThan(0)
|
||||
expect(widgetName).toBeTruthy()
|
||||
expect(
|
||||
resolved,
|
||||
`Widget "${widgetName}" (interior node ${interiorNodeId}) on host ${hostNodeId} should resolve`
|
||||
).toBe(true)
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
test('Prompt execution succeeds without 400 error', async ({ comfyPage }) => {
|
||||
await comfyPage.workflow.loadWorkflow(WORKFLOW)
|
||||
await comfyPage.nextFrame()
|
||||
|
||||
const responsePromise = comfyPage.page.waitForResponse('**/api/prompt')
|
||||
|
||||
await comfyPage.command.executeCommand('Comfy.QueuePrompt')
|
||||
|
||||
const response = await responsePromise
|
||||
expect(response.status()).not.toBe(400)
|
||||
})
|
||||
})
|
||||
@@ -78,7 +78,10 @@ import type {
|
||||
SerialisableReroute
|
||||
} from './types/serialisation'
|
||||
import { getAllNestedItems } from './utils/collections'
|
||||
import { deduplicateSubgraphNodeIds } from './utils/subgraphDeduplication'
|
||||
import {
|
||||
deduplicateSubgraphNodeIds,
|
||||
topologicalSortSubgraphs
|
||||
} from './subgraph/subgraphDeduplication'
|
||||
|
||||
export type {
|
||||
LGraphTriggerAction,
|
||||
@@ -2585,7 +2588,12 @@ export class LGraph
|
||||
effectiveNodesData = deduplicated?.rootNodes ?? nodesData
|
||||
|
||||
for (const subgraph of finalSubgraphs) this.createSubgraph(subgraph)
|
||||
for (const subgraph of finalSubgraphs)
|
||||
|
||||
// Configure in leaf-first order so that when a SubgraphNode is
|
||||
// configured, its referenced subgraph definition already has its
|
||||
// nodes/links/inputs populated.
|
||||
const configureOrder = topologicalSortSubgraphs(finalSubgraphs)
|
||||
for (const subgraph of configureOrder)
|
||||
this.subgraphs.get(subgraph.id)?.configure(subgraph)
|
||||
}
|
||||
|
||||
@@ -2878,6 +2886,10 @@ export class Subgraph
|
||||
}
|
||||
}
|
||||
|
||||
// Repair IO slot linkIds that reference links removed by
|
||||
// _removeDuplicateLinks during super.configure().
|
||||
this._repairIOSlotLinkIds()
|
||||
|
||||
if (widgets) {
|
||||
this.widgets.length = 0
|
||||
for (const widget of widgets) {
|
||||
@@ -2902,6 +2914,50 @@ export class Subgraph
|
||||
return r
|
||||
}
|
||||
|
||||
/**
|
||||
* Repairs SubgraphInput/Output `linkIds` that reference links removed
|
||||
* by `_removeDuplicateLinks` during `super.configure()`.
|
||||
*
|
||||
* For each stale link ID, finds the surviving link that connects to the
|
||||
* same IO node and slot index, and substitutes it.
|
||||
*/
|
||||
private _repairIOSlotLinkIds(): void {
|
||||
for (const [slotIndex, slot] of this.inputs.entries()) {
|
||||
this._repairSlotLinkIds(slot.linkIds, SUBGRAPH_INPUT_ID, slotIndex)
|
||||
}
|
||||
for (const [slotIndex, slot] of this.outputs.entries()) {
|
||||
this._repairSlotLinkIds(slot.linkIds, SUBGRAPH_OUTPUT_ID, slotIndex)
|
||||
}
|
||||
}
|
||||
|
||||
private _repairSlotLinkIds(
|
||||
linkIds: LinkId[],
|
||||
ioNodeId: number,
|
||||
slotIndex: number
|
||||
): void {
|
||||
const repaired = linkIds.map((id) =>
|
||||
this._links.has(id)
|
||||
? id
|
||||
: (this._findLinkBySlot(ioNodeId, slotIndex)?.id ?? id)
|
||||
)
|
||||
repaired.forEach((id, i) => {
|
||||
linkIds[i] = id
|
||||
})
|
||||
}
|
||||
|
||||
private _findLinkBySlot(
|
||||
nodeId: number,
|
||||
slotIndex: number
|
||||
): LLink | undefined {
|
||||
for (const link of this._links.values()) {
|
||||
if (
|
||||
(link.origin_id === nodeId && link.origin_slot === slotIndex) ||
|
||||
(link.target_id === nodeId && link.target_slot === slotIndex)
|
||||
)
|
||||
return link
|
||||
}
|
||||
}
|
||||
|
||||
override attachCanvas(canvas: LGraphCanvas): void {
|
||||
super.attachCanvas(canvas)
|
||||
canvas.subgraph = this
|
||||
|
||||
@@ -566,7 +566,12 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
||||
// Legacy -1 entries use the slot name as the widget name.
|
||||
// Find the input with that name, then trace to the connected interior widget.
|
||||
const input = this.inputs.find((i) => i.name === widgetName)
|
||||
if (!input?._widget) return undefined
|
||||
if (!input?._widget) {
|
||||
// Fallback: find via subgraph input slot connection
|
||||
const resolvedTarget = resolveSubgraphInputTarget(this, widgetName)
|
||||
if (!resolvedTarget) return undefined
|
||||
return [resolvedTarget.nodeId, resolvedTarget.widgetName]
|
||||
}
|
||||
|
||||
const widget = input._widget
|
||||
if (isPromotedWidgetView(widget)) {
|
||||
|
||||
76
src/lib/litegraph/src/subgraph/subgraphDeduplication.test.ts
Normal file
76
src/lib/litegraph/src/subgraph/subgraphDeduplication.test.ts
Normal file
@@ -0,0 +1,76 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import type { ExportedSubgraph } from '../types/serialisation'
|
||||
|
||||
import { topologicalSortSubgraphs } from './subgraphDeduplication'
|
||||
|
||||
function makeSubgraph(id: string, nodeTypes: string[] = []): ExportedSubgraph {
|
||||
return {
|
||||
id,
|
||||
name: id,
|
||||
version: 1,
|
||||
revision: 0,
|
||||
state: { lastNodeId: 0, lastLinkId: 0, lastGroupId: 0, lastRerouteId: 0 },
|
||||
nodes: nodeTypes.map((type, i) => ({
|
||||
id: i + 1,
|
||||
type,
|
||||
pos: [0, 0] as [number, number],
|
||||
size: [100, 100] as [number, number],
|
||||
flags: {},
|
||||
order: i,
|
||||
mode: 0,
|
||||
inputs: [],
|
||||
outputs: [],
|
||||
properties: {}
|
||||
})),
|
||||
inputNode: { id: -10, bounding: [0, 0, 100, 100] },
|
||||
outputNode: { id: -20, bounding: [0, 0, 100, 100] }
|
||||
} as ExportedSubgraph
|
||||
}
|
||||
|
||||
describe('topologicalSortSubgraphs', () => {
|
||||
it('returns original order when there are no dependencies', () => {
|
||||
const a = makeSubgraph('a')
|
||||
const b = makeSubgraph('b')
|
||||
const result = topologicalSortSubgraphs([a, b])
|
||||
expect(result).toEqual([a, b])
|
||||
})
|
||||
|
||||
it('sorts leaf dependencies before their parents', () => {
|
||||
const inner = makeSubgraph('inner', ['StringConcat'])
|
||||
const outer = makeSubgraph('outer', ['inner'])
|
||||
const result = topologicalSortSubgraphs([outer, inner])
|
||||
expect(result.map((s) => s.id)).toEqual(['inner', 'outer'])
|
||||
})
|
||||
|
||||
it('handles three-level nesting', () => {
|
||||
const leaf = makeSubgraph('leaf', ['StringConcat'])
|
||||
const mid = makeSubgraph('mid', ['leaf', 'StringConcat'])
|
||||
const top = makeSubgraph('top', ['mid'])
|
||||
const result = topologicalSortSubgraphs([top, mid, leaf])
|
||||
expect(result.map((s) => s.id)).toEqual(['leaf', 'mid', 'top'])
|
||||
})
|
||||
|
||||
it('handles diamond dependencies', () => {
|
||||
const shared = makeSubgraph('shared')
|
||||
const left = makeSubgraph('left', ['shared'])
|
||||
const right = makeSubgraph('right', ['shared'])
|
||||
const top = makeSubgraph('top', ['left', 'right'])
|
||||
const result = topologicalSortSubgraphs([top, left, right, shared])
|
||||
const ids = result.map((s) => s.id)
|
||||
expect(ids.indexOf('shared')).toBeLessThan(ids.indexOf('left'))
|
||||
expect(ids.indexOf('shared')).toBeLessThan(ids.indexOf('right'))
|
||||
expect(ids.indexOf('left')).toBeLessThan(ids.indexOf('top'))
|
||||
expect(ids.indexOf('right')).toBeLessThan(ids.indexOf('top'))
|
||||
})
|
||||
|
||||
it('returns original order for a single subgraph', () => {
|
||||
const only = makeSubgraph('only')
|
||||
const result = topologicalSortSubgraphs([only])
|
||||
expect(result).toEqual([only])
|
||||
})
|
||||
|
||||
it('returns original order for empty array', () => {
|
||||
expect(topologicalSortSubgraphs([])).toEqual([])
|
||||
})
|
||||
})
|
||||
@@ -140,6 +140,63 @@ function patchPromotedWidgets(
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Topologically sorts subgraph definitions so that leaf subgraphs (those
|
||||
* that no other subgraph depends on) are configured first. This ensures
|
||||
* that when a SubgraphNode is configured, the subgraph definition it
|
||||
* references already has its nodes, links, and inputs populated.
|
||||
*
|
||||
* Falls back to the original order if no reordering is needed or if the
|
||||
* dependency graph contains cycles.
|
||||
*/
|
||||
export function topologicalSortSubgraphs(
|
||||
subgraphs: ExportedSubgraph[]
|
||||
): ExportedSubgraph[] {
|
||||
const subgraphIds = new Set(subgraphs.map((sg) => sg.id))
|
||||
const byId = new Map(subgraphs.map((sg) => [sg.id, sg]))
|
||||
|
||||
// Build adjacency: dependency → set of dependents (parents that use it).
|
||||
// Edges go from leaf to parent so Kahn's emits leaves first.
|
||||
const dependents = new Map<string, Set<string>>()
|
||||
const inDegree = new Map<string, number>()
|
||||
for (const id of subgraphIds) {
|
||||
dependents.set(id, new Set())
|
||||
inDegree.set(id, 0)
|
||||
}
|
||||
|
||||
for (const sg of subgraphs) {
|
||||
for (const node of sg.nodes ?? []) {
|
||||
if (subgraphIds.has(node.type)) {
|
||||
// sg depends on node.type → edge from node.type to sg.id
|
||||
dependents.get(node.type)!.add(sg.id)
|
||||
inDegree.set(sg.id, (inDegree.get(sg.id) ?? 0) + 1)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Kahn's algorithm — leaves (in-degree 0) are emitted first.
|
||||
const queue: string[] = []
|
||||
for (const [id, degree] of inDegree) {
|
||||
if (degree === 0) queue.push(id)
|
||||
}
|
||||
|
||||
const sorted: ExportedSubgraph[] = []
|
||||
while (queue.length > 0) {
|
||||
const id = queue.shift()!
|
||||
sorted.push(byId.get(id)!)
|
||||
for (const dependent of dependents.get(id) ?? []) {
|
||||
const newDegree = (inDegree.get(dependent) ?? 1) - 1
|
||||
inDegree.set(dependent, newDegree)
|
||||
if (newDegree === 0) queue.push(dependent)
|
||||
}
|
||||
}
|
||||
|
||||
// Cycle fallback: return original order
|
||||
if (sorted.length !== subgraphs.length) return subgraphs
|
||||
|
||||
return sorted
|
||||
}
|
||||
|
||||
/** Patches proxyWidgets in root-level SubgraphNode instances. */
|
||||
function patchProxyWidgets(
|
||||
rootNodes: ISerialisedNode[],
|
||||
Reference in New Issue
Block a user