ComfyUI icon indicating copy to clipboard operation
ComfyUI copied to clipboard

Add mechanism to core to allow loading custom nodes as entry points

Open telamonian opened this issue 1 year ago • 4 comments

This code adds a mechanism by which custom node extensions can be loaded by core. A custom node will advertise functionality via an entry_point, and core will consume all such available and appropriate entry points. Among other things, this will allow custom nodes that are installed as proper Python packages (instead of just being manually dumped in the custom_nodes folder) to still function correctly.

In general, it would be nice if comfy extensions were more like proper Python packages in terms of metadata and layout. This would make it easier to use the existing Python installer tools with them, and just make the whole situation around developing new comfy-specific installer tools much easier. To that end, this PR is code that goes along with what I'm calling Comfy Enhancement Proposal 1 (CEP 1). Along the lines of a Python Enhancement Proposal (or a Jupyter Enhancement Proposal, which I'm a bit more familiar with personally), CEP 1 is a detailed proposal asking for community buy-in on a non-minor change to the ComfyUI ecosystem. CEP 1 lays out a number of justifications and details for this PR, so please read it for more information.

telamonian avatar May 30 '24 17:05 telamonian

  • it doesn't deal with errors, which will happen all the time
  • the way importlib.metadata.files is used will not interact correctly with packages actually installed from wheels declaring web/ and similar directories in typical cases
  • it makes sense to have 1 entry point over 0 or many. you are not going to get value out of having a million of these, eventually, in a setup.py or pyproject.toml
  • once you eliminate the multiple entrypoints, you can get rid of the Module already loaded. code flow issue. so the whole thing becomes a lot more succinct
  • the entrypoint should point to a function that follows a specific signature that is then called and returns the things like node class mapping declarations, if you want this to be less painful for the future. then it can be typed and documented easily. otherwise, it should point to a module, and the module should be introspected / queried for the node_class_mappings, etc. declared in it, as it is right now. having it point to a random dictionary is the most surprising: it will have the same documentation, non-typing and testing burden as the entrypoint pointing to a module, with all the inflexibility of dealing with a dict.

delay adding new module names to the guard set until after all possible entry_points are loaded

load order is really toxic overall in comfyui. i think you should adopt my approach.

doctorpangloss avatar Jun 05 '24 22:06 doctorpangloss

@doctorpangloss

  • good point, I'll add some basic error handling
  • AFAIK importlib.metadata.files should work in any case where you have a correctly structured/installed Python package. But I've only tested simple cases, so if you know of a case where it fails, can you please post some precise repro instructions?
  • my main goal in structuring the entry_points is to require as few changes as possible (ideally zero changes) to the actual code of an extension in order to migrate it over to using said entry_points. But you might be right that using a single module entry_point may be simpler. I'll test that out

telamonian avatar Jun 06 '24 23:06 telamonian

@doctorpangloss

i think you should adopt my approach.

Well, I would say that most people think that most of the time :)

But still, I liked your suggestions and have done my best to implement related changes:

  • The relevant bits of the entry_point loading code is now wrapped in a try...except
  • There's now just a single "comfyui.extension_module" entry_point that is meant to point to an extension's top-level module
    • I agree with you that the "best" entry_point would be a schematized entry function with a strictly defined signature/return value. However, my goal is to make migration as easy as possible for existing extensions. Making the module the entry_point means that no changes to actual code are required for migration.

In general I refactored things so that as much code as possible is reused between the old manual module loading path and the new entry_point module loading path. This should help ensure consistent behavior for extensions going forward, regardless of loading method

telamonian avatar Jun 21 '24 08:06 telamonian

I've done some testing of the PR code and everything appears to be working as advertised:

  • ComfyUI-KJNodes is a good example of an existing extension that has complex custom javascript and that makes use of WEB_DIRECTORY. I refactored KJNodes (see my fork here) into a proper python package with a "comfyui.extension_module" entry_point, and after installing it (using pip install .) it works perfectly, including all of its javascript

  • As a companion to this PR I've been working on the cookiecutter-comfy-extension project template for new extensions. An extension made using this template is automatically laid out as a python package that includes the expected entry_point. I made an example project here using the current version of the template, and once installed (using pip install .) said example project is loaded correctly by the PR code. The example project can optionally include a web directory, and the example .js file in said web dir also runs correctly (it just prints the app object to console)

To clarify, part of the trick here is that a Python project needs to have a MANIFEST.in file that lists all of the non-python files (in this case the .js files in web and other related files). That ensures that said non-python files get installed correctly alongside the python files. See one of the two above examples for details, or the MANIFEST.in docs here. If your extension is pure python and does not have a WEB_DIRECTORY, then you will not need a MANIFEST.in.

@comfyanonymous

I think this PR is about as done as it can be, so now would be a great time for you to take a look if you can and weigh in. In the meantime I'll start figuring out how to add a couple of relevant tests of extension loading to the CI

telamonian avatar Jun 21 '24 12:06 telamonian

This PR hasn't been updated in a few months, and has merge conflicts. What's the status/intent? Has this been discussed with comfy?

mcmonkey4eva avatar Sep 24 '24 08:09 mcmonkey4eva