fix(inspector): check only the negative the legacy parse read for a zero-out
Review I1/M2/M3/M5 of the zero-out legacy-fill guard. The guard decided over every node with a `negative` input, so a graph whose read sampler encodes "blurry" lost that legacy-filled negative when another branch zeroed its own (fewer than the legacy pages: below the parity floor). - comfy-metadata.js gains `legacyNegativeSource`, which returns the graph and sampler `negative` link that `extractGenerationParams` reads from. It reuses the parser's own sampler pick (`selectSampler`, factored out of `fromPromptGraph` without changing any reading). The workspace checks only that link: a direct zero-out shows the zeroed empty negative, a zero-out further up the path leaves it unfilled, and anything else keeps the legacy fill. - Workflow-only files go through the parser's own workflow conversion, so bypassed (mode 4) and muted (mode 2) zero-outs and unconnected ones behave as the legacy reading does. A subgraph instance whose definition runs a zero-out (directly or nested) leaves the negative unfilled. - One bounded walk with an index pointer replaces a walk per negative input with queue.shift(). - An ambiguous fact that the legacy parse knows (a grid's per-branch model) is still filled, documented: the legacy pages show it and this page never shows less. Tests: the reversed mixed graph shows "blurry"; the mixed graph whose read sampler is zeroed, the zero-out-only grid and a direct workflow zero-out show the zeroed empty negative; a combined zero-out and a zero-out subgraph instance stay unfilled; bypassed and unconnected workflow zero-outs keep the legacy reading. An API-graph Reroute in front of a zero-out gives the legacy parse no negative, so the server answer stays (that expectation changed from zeroed).
This commit is contained in:
Vito Sansevero
@@ -30,7 +30,7 @@ test("facts guider providers are guiders in the semantics table", () => {
|
||||
});
|
||||
|
||||
test("the facts tables are not part of the legacy page contract", () => {
|
||||
assert.deepStrictEqual(Object.keys(metadata).filter(key => !key.startsWith("_")).sort(), ["PLACEHOLDERS", "extractGenerationFacts", "extractGenerationParams"]);
|
||||
assert.deepStrictEqual(Object.keys(metadata).filter(key => !key.startsWith("_")).sort(), ["PLACEHOLDERS", "extractGenerationFacts", "extractGenerationParams", "legacyNegativeSource"]);
|
||||
assert.ok(Object.isFrozen(metadata._factsTables));
|
||||
});
|
||||
|
||||
|
||||
@@ -573,44 +573,99 @@ test('a zeroed negative is never filled with the positive text from the legacy p
|
||||
const record = undecided(zeroedGridGraph());
|
||||
assert.deepEqual(record.prompts.positive, { status: 'resolved', text: 'a woman holding a coffee cup', source: 'graph-inferred' });
|
||||
assert.deepEqual(record.prompts.negative, { status: 'resolved', text: '', source: 'zeroed-conditioning' });
|
||||
// A Reroute between the sampler and the zero-out is still a zeroed negative.
|
||||
const rerouted = zeroedGridGraph(2);
|
||||
// Behind an API-graph Reroute the legacy parse reads no negative, so there is nothing
|
||||
// to correct: the server's answer stays.
|
||||
const rerouted = zeroedGridGraph(1);
|
||||
rerouted[7] = { class_type: 'Reroute', inputs: { value: ['5', 0] } };
|
||||
rerouted[20].inputs.negative = ['7', 0];
|
||||
assert.deepEqual(undecided(rerouted).prompts.negative, { status: 'resolved', text: '', source: 'zeroed-conditioning' });
|
||||
assert.equal(parser.legacyNegativeSource({ prompt: rerouted }), null);
|
||||
assert.deepEqual(undecided(rerouted).prompts.negative, { status: 'ambiguous', text: null, source: null });
|
||||
});
|
||||
|
||||
test('a negative whose zero-out path cannot be decided stays as the server left it (#249)', t => {
|
||||
globalThis.ComfyMetadata = require('../../web/js/comfy-metadata.js');
|
||||
test('the zero-out check follows the sampler the legacy parse read (#249)', t => {
|
||||
const parser = require('../../web/js/comfy-metadata.js');
|
||||
globalThis.ComfyMetadata = parser;
|
||||
t.after(() => { delete globalThis.ComfyMetadata; });
|
||||
const ambiguous = { status: 'ambiguous', text: null, source: null };
|
||||
// One branch zeroes its negative, another encodes a text: which one the image used is open.
|
||||
const zeroed = { status: 'resolved', text: '', source: 'zeroed-conditioning' };
|
||||
// The legacy parse reads the first sampler (20): its negative is the zero-out.
|
||||
const mixed = zeroedGridGraph(2);
|
||||
mixed[8] = { class_type: 'CLIPTextEncode', inputs: { text: 'blurry', clip: ['2', 0] } };
|
||||
mixed[21].inputs.negative = ['8', 0];
|
||||
assert.deepEqual(undecided(mixed).prompts.negative, ambiguous);
|
||||
assert.deepEqual(undecided(mixed).prompts.negative, zeroed);
|
||||
// Reversed: the sampler it reads encodes "blurry", which it showed before and still does.
|
||||
const reversed = zeroedGridGraph(2);
|
||||
reversed[8] = { class_type: 'CLIPTextEncode', inputs: { text: 'blurry', clip: ['2', 0] } };
|
||||
reversed[20].inputs.negative = ['8', 0];
|
||||
assert.equal(parser.extractGenerationParams({ prompt: reversed }).negativePrompt, 'blurry');
|
||||
assert.deepEqual(undecided(reversed).prompts.negative, { status: 'resolved', text: 'blurry', source: 'graph-inferred' });
|
||||
// The positive fills from the legacy parse in both cases.
|
||||
assert.equal(undecided(mixed).prompts.positive.text, 'a woman holding a coffee cup');
|
||||
assert.equal(undecided(reversed).prompts.positive.text, 'a woman holding a coffee cup');
|
||||
});
|
||||
|
||||
test('a zero-out mixed into the read negative leaves it as the server left it (#249)', t => {
|
||||
globalThis.ComfyMetadata = require('../../web/js/comfy-metadata.js');
|
||||
t.after(() => { delete globalThis.ComfyMetadata; });
|
||||
// A zero-out combined with another conditioning is not an empty negative.
|
||||
const combined = zeroedGridGraph(1);
|
||||
combined[8] = { class_type: 'CLIPTextEncode', inputs: { text: 'blurry', clip: ['2', 0] } };
|
||||
combined[9] = { class_type: 'ConditioningCombine', inputs: { conditioning_1: ['5', 0], conditioning_2: ['8', 0] } };
|
||||
combined[20].inputs.negative = ['9', 0];
|
||||
assert.deepEqual(undecided(combined).prompts.negative, ambiguous);
|
||||
// The positive still fills from the legacy parse in both cases.
|
||||
assert.equal(undecided(mixed).prompts.positive.text, 'a woman holding a coffee cup');
|
||||
// A workflow-only file holding a zero-out never fills the negative either.
|
||||
assert.deepEqual(undecided(combined).prompts.negative, { status: 'ambiguous', text: null, source: null });
|
||||
});
|
||||
|
||||
function workflowOnly(workflow) {
|
||||
const entry = image(), data = envelope(entry);
|
||||
data.prompts = { positive: { status: 'unresolved', text: null, source: null }, negative: { status: 'unresolved', text: null, source: null } };
|
||||
const workflow = { nodes: [
|
||||
{ id: 4, type: 'CLIPTextEncode', inputs: [], outputs: [{ links: [1, 2] }], widgets_values: ['a woman holding a coffee cup'] },
|
||||
{ id: 5, type: 'ConditioningZeroOut', inputs: [{ name: 'conditioning', link: 1 }], outputs: [{ links: [3] }] },
|
||||
{ id: 1, type: 'UNETLoader', inputs: [], outputs: [{ links: [4] }], widgets_values: ['base.safetensors', 'default'] },
|
||||
{ id: 3, type: 'KSampler', inputs: [{ name: 'model', link: 4 }, { name: 'positive', link: 2 }, { name: 'negative', link: 3 }], widgets_values: [42, 'fixed', 8, 1, 'euler', 'simple', 1] },
|
||||
], links: [[1, 4, 0, 5, 0, 'CONDITIONING'], [2, 4, 0, 3, 1, 'CONDITIONING'], [3, 5, 0, 3, 2, 'CONDITIONING'], [4, 1, 0, 3, 0, 'MODEL']] };
|
||||
assert.equal(globalThis.ComfyMetadata.extractGenerationParams({ workflow }).negativePrompt, 'a woman holding a coffee cup', 'the legacy parse would fill it');
|
||||
Object.assign(data.metadata, { raw: { workflow: JSON.stringify(workflow) }, record_formats: { workflow: 'reserialized-json' },
|
||||
prompt_graph: { status: 'absent', value: null }, workflow_graph: { status: 'parsed', value: workflow } });
|
||||
const record = contract('toInspectionRecord')(data, entry, extractGenerationFacts);
|
||||
assert.deepEqual(record.prompts.negative, { status: 'unresolved', text: null, source: null });
|
||||
return contract('toInspectionRecord')(data, entry, extractGenerationFacts);
|
||||
}
|
||||
|
||||
// A UI workflow: encode 4 -> zero-out 5 -> sampler 3's negative, encode 4 -> positive.
|
||||
function zeroedWorkflow({ zeroMode = 0, extraNodes = [], extraLinks = [], negativeLink = 3 } = {}) {
|
||||
return { nodes: [
|
||||
{ id: 4, type: 'CLIPTextEncode', inputs: [], outputs: [{ links: [1, 2] }], widgets_values: ['a woman holding a coffee cup'] },
|
||||
{ id: 5, type: 'ConditioningZeroOut', mode: zeroMode, inputs: [{ name: 'conditioning', type: 'CONDITIONING', link: 1 }], outputs: [{ links: [3] }] },
|
||||
{ id: 1, type: 'UNETLoader', inputs: [], outputs: [{ links: [4] }], widgets_values: ['base.safetensors', 'default'] },
|
||||
{ id: 3, type: 'KSampler', inputs: [{ name: 'model', link: 4 }, { name: 'positive', link: 2 }, { name: 'negative', link: negativeLink }], widgets_values: [42, 'fixed', 8, 1, 'euler', 'simple', 1] },
|
||||
...extraNodes,
|
||||
], links: [[1, 4, 0, 5, 0, 'CONDITIONING'], [2, 4, 0, 3, 1, 'CONDITIONING'], [3, 5, 0, 3, 2, 'CONDITIONING'], [4, 1, 0, 3, 0, 'MODEL'], ...extraLinks] };
|
||||
}
|
||||
|
||||
test('workflow-only files follow the legacy reading of their zero-out (#249)', t => {
|
||||
const parser = require('../../web/js/comfy-metadata.js');
|
||||
globalThis.ComfyMetadata = parser;
|
||||
t.after(() => { delete globalThis.ComfyMetadata; });
|
||||
// A connected zero-out on the read sampler's negative is the zeroed empty negative.
|
||||
const direct = zeroedWorkflow();
|
||||
assert.equal(parser.extractGenerationParams({ workflow: direct }).negativePrompt, 'a woman holding a coffee cup', 'the legacy parse would fill it');
|
||||
assert.deepEqual(workflowOnly(direct).prompts.negative, { status: 'resolved', text: '', source: 'zeroed-conditioning' });
|
||||
// A bypassed zero-out (mode 4) passes its conditioning through: the legacy reading is right.
|
||||
const bypassed = workflowOnly(zeroedWorkflow({ zeroMode: 4 }));
|
||||
assert.deepEqual(bypassed.prompts.negative, { status: 'resolved', text: 'a woman holding a coffee cup', source: 'graph-inferred' });
|
||||
// An unconnected zero-out beside a text negative changes nothing.
|
||||
const unconnected = workflowOnly(zeroedWorkflow({ negativeLink: 6,
|
||||
extraNodes: [{ id: 6, type: 'CLIPTextEncode', inputs: [], outputs: [{ links: [6] }], widgets_values: ['blurry'] }],
|
||||
extraLinks: [[6, 6, 0, 3, 2, 'CONDITIONING']] }));
|
||||
assert.deepEqual(unconnected.prompts.negative, { status: 'resolved', text: 'blurry', source: 'graph-inferred' });
|
||||
});
|
||||
|
||||
test('a negative read through a subgraph holding a zero-out is not filled (#249)', t => {
|
||||
const parser = require('../../web/js/comfy-metadata.js');
|
||||
globalThis.ComfyMetadata = parser;
|
||||
t.after(() => { delete globalThis.ComfyMetadata; });
|
||||
// The sampler's negative comes from a subgraph instance whose definition zeroes it.
|
||||
const definition = 'aaaaaaaa-0000-4000-8000-000000000001';
|
||||
const workflow = zeroedWorkflow({ negativeLink: 7,
|
||||
extraNodes: [{ id: 9, type: definition, inputs: [{ name: 'conditioning', type: 'CONDITIONING', link: 8 }], outputs: [{ links: [7] }] }],
|
||||
extraLinks: [[7, 9, 0, 3, 2, 'CONDITIONING'], [8, 4, 0, 9, 0, 'CONDITIONING']] });
|
||||
workflow.definitions = { subgraphs: [{ id: definition, nodes: [{ id: 1, type: 'ConditioningZeroOut', inputs: [], outputs: [] }], links: [] }] };
|
||||
assert.equal(parser.extractGenerationParams({ workflow }).negativePrompt, 'a woman holding a coffee cup', 'the legacy parse would fill it');
|
||||
assert.deepEqual(workflowOnly(workflow).prompts.negative, { status: 'unresolved', text: null, source: null });
|
||||
// The same instance of a definition without a zero-out keeps the legacy fill.
|
||||
workflow.definitions.subgraphs[0].nodes[0].type = 'Reroute';
|
||||
assert.equal(workflowOnly(workflow).prompts.negative.text, 'a woman holding a coffee cup');
|
||||
});
|
||||
|
||||
test('callers without the browser global pass the legacy parser explicitly', () => {
|
||||
|
||||
@@ -212,12 +212,17 @@
|
||||
return undefined;
|
||||
}
|
||||
|
||||
/** The sampler the legacy parse reads: a custom sampler's guider, else the first sampler. */
|
||||
function selectSampler(ctx, nodes) {
|
||||
const custom = nodes.find(isCustomSampler);
|
||||
const [, guider] = custom ? enter(ctx, custom.inputs.guider, new Set()) : [];
|
||||
return { custom, sampler: isSampler(guider) ? guider : nodes.find(isSampler) };
|
||||
}
|
||||
|
||||
function fromPromptGraph(graph, workflow) {
|
||||
const ctx = createContext(graph, workflow);
|
||||
const nodes = Object.values(graph);
|
||||
const custom = nodes.find(isCustomSampler);
|
||||
const [, guider] = custom ? enter(ctx, custom.inputs.guider, new Set()) : [];
|
||||
const sampler = isSampler(guider) ? guider : nodes.find(isSampler);
|
||||
const { custom, sampler } = selectSampler(ctx, nodes);
|
||||
if (!sampler) {
|
||||
const loose = nodes.find((n) => n && n.inputs && /ksampler/i.test(n.class_type || ""));
|
||||
const inputs = (loose && loose.inputs) || {};
|
||||
@@ -402,6 +407,26 @@
|
||||
return merged;
|
||||
}
|
||||
|
||||
/**
|
||||
* Where `extractGenerationParams` reads its negative from (#249): the graph it used
|
||||
* (the API graph, else the workflow converted the same way) and the read sampler's
|
||||
* `negative` link, or null when it found no negative. It changes no reading.
|
||||
*/
|
||||
function legacyNegativeSource(comfyData) {
|
||||
const data = comfyData || {};
|
||||
const sources = [];
|
||||
if (data.prompt && typeof data.prompt === "object") sources.push([data.prompt, data.workflow]);
|
||||
if (data.workflow) sources.push([workflowToGraph(data.workflow), null]);
|
||||
for (const [graph, workflow] of sources) {
|
||||
const ctx = createContext(graph, workflow);
|
||||
const { sampler } = selectSampler(ctx, Object.values(graph));
|
||||
if (sampler && hasValue(traceConditioning(ctx, sampler.inputs.negative, "negative"))) {
|
||||
return { graph, link: sampler.inputs.negative };
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
const FACT_FIELDS = ["seed", "steps", "cfg", "sampler", "scheduler", "model"];
|
||||
const WORKFLOW_PASSTHROUGH = Symbol("inspection workflow passthrough");
|
||||
// These tables must equal inspection-semantics.json: SAMPLERS its `samplers` section
|
||||
@@ -761,7 +786,7 @@
|
||||
return factSet(metadataStatus === "available" || metadataStatus === "absent" ? "not-recorded" : "unresolved");
|
||||
}
|
||||
|
||||
const api = { extractGenerationParams, extractGenerationFacts, PLACEHOLDERS };
|
||||
const api = { extractGenerationParams, extractGenerationFacts, legacyNegativeSource, PLACEHOLDERS };
|
||||
// For the semantics sync test only; not part of the legacy page contract.
|
||||
Object.defineProperty(api, "_factsTables", {
|
||||
value: Object.freeze({
|
||||
|
||||
@@ -175,45 +175,52 @@ function comfyMetadata() {
|
||||
}
|
||||
|
||||
// #249: the legacy parse follows ConditioningZeroOut back to its input, so it reads the
|
||||
// positive text as the negative. The workspace shows the server's zeroed state instead
|
||||
// when every sampler's negative is a zero-out, and never fills the negative from the
|
||||
// legacy parse when a zero-out sits anywhere else in a negative path.
|
||||
// positive text as the negative. Only the negative it actually read is checked: a direct
|
||||
// zero-out shows the server's zeroed state; a zero-out elsewhere on
|
||||
// that path, or a subgraph instance whose definition holds one, leaves the negative
|
||||
// unfilled. Any other negative keeps the legacy fill, so this page never shows less.
|
||||
const ZERO_OUT = 'ConditioningZeroOut';
|
||||
const ZEROED_NEGATIVE = Object.freeze({ status: 'resolved', text: '', source: 'zeroed-conditioning' });
|
||||
const MAX_NEGATIVE_VISITS = 2000;
|
||||
const MODE_MUTED = 2, MODE_BYPASSED = 4;
|
||||
const isLink = value => Array.isArray(value) && value.length === 2 && ['string', 'number'].includes(typeof value[0]);
|
||||
|
||||
/** 'zeroed', 'undecided' (do not fill), or null (no zero-out: the legacy fill applies). */
|
||||
function zeroOutNegative(graph, workflow) {
|
||||
if (!graph) return Array.isArray(workflow?.nodes) && workflow.nodes.some(node => node?.type === ZERO_OUT) ? 'undecided' : null;
|
||||
const nodes = Object.values(graph).filter(object);
|
||||
if (!nodes.some(node => node.class_type === ZERO_OUT)) return null;
|
||||
const kinds = nodes.filter(node => object(node.inputs) && isLink(node.inputs.negative)).map(node => negativeKind(graph, node.inputs.negative));
|
||||
if (kinds.length && kinds.every(kind => kind === 'zeroed')) return 'zeroed';
|
||||
return kinds.every(kind => kind === 'plain') && kinds.length ? null : 'undecided';
|
||||
/** Subgraph definition IDs that run a zero-out, directly or through a nested instance. */
|
||||
function zeroOutSubgraphs(workflow) {
|
||||
const definitions = Array.isArray(workflow?.definitions?.subgraphs) ? workflow.definitions.subgraphs.filter(object) : [];
|
||||
const active = item => Array.isArray(item.nodes) ? item.nodes.filter(node => object(node) && ![MODE_MUTED, MODE_BYPASSED].includes(node.mode)) : [];
|
||||
const found = new Set();
|
||||
for (let changed = true; changed;) {
|
||||
changed = false;
|
||||
for (const item of definitions) {
|
||||
if (found.has(item.id) || !active(item).some(node => node.type === ZERO_OUT || found.has(node.type))) continue;
|
||||
found.add(item.id); changed = true;
|
||||
}
|
||||
}
|
||||
return found;
|
||||
}
|
||||
|
||||
function negativeKind(graph, link) {
|
||||
let node = graph[String(link[0])];
|
||||
for (let hops = 0; object(node) && node.class_type === 'Reroute' && hops < MAX_NEGATIVE_VISITS; hops++) {
|
||||
const next = object(node.inputs) ? node.inputs.value : undefined;
|
||||
node = isLink(next) ? graph[String(next[0])] : undefined;
|
||||
}
|
||||
/** 'zeroed', 'undecided' (do not fill), or null (the legacy fill applies). */
|
||||
function zeroOutNegative(source, workflow) {
|
||||
if (!source || !object(source.graph) || !isLink(source.link)) return null;
|
||||
const { graph } = source, subgraphs = zeroOutSubgraphs(workflow);
|
||||
const zeroing = node => node.class_type === ZERO_OUT || subgraphs.has(node.class_type);
|
||||
if (!Object.values(graph).some(node => object(node) && zeroing(node))) return null;
|
||||
const node = graph[String(source.link[0])];
|
||||
if (!object(node)) return 'undecided';
|
||||
if (node.class_type === ZERO_OUT) return 'zeroed';
|
||||
// Any zero-out further up (combined, averaged, ...) leaves the negative undecided.
|
||||
// One bounded walk upstream; an index pointer keeps it linear in the nodes visited.
|
||||
const queue = [node], seen = new Set();
|
||||
while (queue.length) {
|
||||
const current = queue.shift();
|
||||
if (current.class_type === ZERO_OUT) return 'undecided';
|
||||
if (seen.size >= MAX_NEGATIVE_VISITS) return 'undecided';
|
||||
for (let index = 0; index < queue.length; index++) {
|
||||
const current = queue[index];
|
||||
if (zeroing(current) || seen.size >= MAX_NEGATIVE_VISITS) return 'undecided';
|
||||
for (const value of object(current.inputs) ? Object.values(current.inputs) : []) {
|
||||
const id = isLink(value) ? String(value[0]) : null;
|
||||
if (id === null || seen.has(id) || !object(graph[id])) continue;
|
||||
seen.add(id); queue.push(graph[id]);
|
||||
}
|
||||
}
|
||||
return 'plain';
|
||||
return null;
|
||||
}
|
||||
|
||||
function withWorkflowParams(record, parser = comfyMetadata()) {
|
||||
@@ -226,7 +233,9 @@ function withWorkflowParams(record, parser = comfyMetadata()) {
|
||||
const found = key => typeof params[key] === 'string' || typeof params[key] === 'number' ?
|
||||
String(params[key]) !== '' && params[key] !== parser.PLACEHOLDERS?.[key] : false;
|
||||
const prompts = { ...record.prompts };
|
||||
const zeroed = prompts.negative?.status === 'resolved' ? null : zeroOutNegative(graph, workflow);
|
||||
const read = prompts.negative?.status === 'resolved' || typeof parser.legacyNegativeSource !== 'function' ? null :
|
||||
parser.legacyNegativeSource({ prompt: graph, workflow });
|
||||
const zeroed = zeroOutNegative(read, workflow);
|
||||
for (const [role, key] of [['positive', 'positivePrompt'], ['negative', 'negativePrompt']]) {
|
||||
const current = prompts[role];
|
||||
if (current?.status === 'resolved') continue;
|
||||
@@ -240,6 +249,8 @@ function withWorkflowParams(record, parser = comfyMetadata()) {
|
||||
record.prompts = prompts;
|
||||
for (const [field, key] of LEGACY_FACTS) {
|
||||
// Only open answers are filled: a known value or a definite "not recorded" stays.
|
||||
// An ambiguous field (e.g. a compare grid's per-branch model) is filled too, on
|
||||
// purpose: the legacy pages show this value, and this page never shows less.
|
||||
if (!found(key) || !['unresolved', 'ambiguous'].includes(record.generation[field]?.status)) continue;
|
||||
record.generation[field] = { status: 'known', value: String(params[key]), source: graph ? 'api-graph' : 'workflow', evidence: 'configuration', reason: null, sampler_id: null };
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user