-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: object_info (v2) #1
Conversation
Questions
ContextHow we are working around with ComfyDeploy with object_info in existing ComfyUI.
FeedbackBy having Suggestion
OverallOverall liking the direction, Some custom nodes do not yet follow proper folder_path systems, if this migration can force custom nodes dev to upgrade. |
@BennyKok Thanks for the feedback! Questions
|
"CheckpointLoader": { | ||
"input": { | ||
"required": { | ||
"ckpt_name": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a decision I am not very sure about. Whether we go one step further and flatten the structure here:
From
"combo input": ["COMBO", { options: [1, 2, 3], default: 2}]
to
"combo input": {"type": "COMBO", "options": [1, 2, 3], "default": 2}
This shall further reduce the complexity parsing the API response, at a cost of an extra named field.
The frontend is already doing this conversion here:
https://github.com/Comfy-Org/ComfyUI_frontend/blob/527561d1488c5ab522b8d9ef14a4b82c97d1f550/src/stores/nodeDefStore.ts#L96-L122
Maybe we can support this feature based on this modification. |
Here's my 2c to this rfc:
|
I propose we handle the lazy initialization of combos at the widget level rather than with any server-side caching. The approach would be:
This gives us:
The main tradeoff is that, by default, combo values will become stale (current behavior). If real-time file system sync is needed for specific widgets, dependencies can be added or Example implementation: // widgets.ts
COMBO(node, inputName, inputData: InputSpec, app, widgetName, options = {}) {
// ...
if (options.isLazy) {
const widgetStore = useWidgetStore()
// Create memoization cache key that can be shared with widgets using same (structurally) getter
// E.g., two widgets on distinct nodes that both need the list of loras.
const widgetKey = `${inputName}/${JSON.stringify(options)}`
// Register the getter with the store
widgetStore.registerGetter(widgetKey, options.getter)
const origOptions = res.widget.options
res.widget.options = new Proxy(origOptions, {
get(target, prop) {
if (prop !== 'values') return target[prop]
// widgetStore.getValues will call the getter associated with the widget's key.
// If any dependencies have changed since last access, the value will be re-evaluated.
return widgetStore.getValues(widgetKey)
}
})
// This can be called via `app.graph.nodes` reference
res.widget.forceUpdate = () => {
// Value is deleted which forces re-evaluation on next access
widgetStore.clearCache(widgetKey)
node.graph.setDirtyCanvas(true)
}
}
FILE_COMBO(node: LGraphNode, inputName: string, inputData: InputSpec) {
const {
folder_path,
filter_content_type,
filter_extension,
include_directories
} = inputData[1]
const getter = async () => {
const res = await api.getFilepaths({
folder_path,
filter_content_type,
filter_extension,
include_directories
})
return res.files
}
return ComfyWidgets.COMBO(node, inputName, inputData, app, inputName, {
isLazy: true,
getter
})
}, Any thoughts or feedback is appreciated. |
LGTM. Some extra random thoughts regarding the implementation:
|
Thanks for the reply.
I agree. We should make the getter configurable through the python input data and construct the js function inside of The spec should be made so it is generic enough to be extended beyond internal file lists. For example, instead of this: class LoadLatent:
@classmethod
def INPUT_TYPES(s):
return {
"required": {
"latent": (
"FILE_COMBO",
{"folder_path": "input", "filter_extension": [".latent"]},
)
},
} Something like this: class LoadLatent:
@classmethod
def INPUT_TYPES(s):
return {
"required": {
"latent": (
"COMBO",
{
"getter": {
"type": "file_list",
"folder_path": "input",
"filter_extension": [".latent"],
}
},
)
},
} Or this: class LoadLatent:
@classmethod
def INPUT_TYPES(s):
return {
"required": {
"latent": (
"COMBO",
{
"getter": {
"type": "route",
"route": "/internal/files?folder_path=input&extension=.latent",
"backoff": 500,
"TTL": 10_000 # default is infinity
}
},
)
},
} WDYT? |
Thoughts:
|
In this proposal, However, if an alternative approach, such as a client-side TTL, is used, those changes might become relevant. |
The main effect is that part of the frontend startup time will be deferred until nodes are added to the graph. One consequence of this is that graph execution must be disabled until widgets finish initializing. For example:
|
Yes, the lazy widgets can use a global cache with routes as keys. The cache can be populated with the promise first then the resolved value afterwards. On access, if the value is uninitialized or stale, the fetch is started in non-blocking manner. If the cache contains a promise, it does not repeat. The cache can also track errors and timestamps to allow for TTL field and backoff.
Since the requests are managed in a centralized store, it should be possible to batch. |
The lazy widgets and |
Think this can merge now since it's actively being worked on |
This RFC proposes three key improvements to the ComfyUI API
/object_info
endpoint: