Skip to content
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

New Node Import System #1679

Merged
merged 54 commits into from
Apr 5, 2023
Merged

Conversation

joeyballentine
Copy link
Member

@joeyballentine joeyballentine commented Mar 26, 2023

This is based on #1430.

Summary of what I've done on top of that:

  • Renamed "subcategories" to "node groups" to be more clear about them and also to prepare for a future in which we use sub-menus
  • Move metadata things into the decorator. Now the constructor is only used for input & output definitions and things like saying if a node has side effects. This is nice because it now gives a clear separation between what is metadata and what is functionality.
  • Scan packages directory for imports rather than manually importing it and ignoring that it isn't used
  • Path-like routing. I'm not 100% sold on this system and will probably go back to how it was before, but basically, now categories and node groups have their variables defined in the __init__.py files and are generically named so they can be moved around freely to re-categorize them without needing code changes. This means in order to reorder node groups or categories, you need to name the folders in the alphabetical order you want to use them in. Since that's kinda weird, I'll probably go back to the other system where categories and node groups are defined one level up and then imported. in each node. Though, one potential idea to get around that could be just using the __init__.py file in each node group folder to import the node group and then export it again as node_group. That would have the best of both systems.
  • I decided to go with my idea from above. Nodes can still be freely moved around and automatically get put into the ode group they're placed into, and ordering node groups is manually done in code. This means that node groups can't be reordered based on where they're placed (at least not as easily), but I think that's a good compromise.

Anyway, this is still heavily WIP as I've only ported over the image category, but I want to gett an opinion before working on it more.

Checklist:

  • image
  • image_adjustment
  • image_channel
  • image_dimension
  • image_filter
  • image_utility
  • material_textures
  • utility
  • ncnn
  • onnx
  • pytorch

Other things:

  • afterwards, fix the ordering of the node groups to match how they were (except for whatever changes we want to make)
  • make sure the nodes in each sub-group get ordered alphabetically.

To be accomplished in separate PRs after this one:

  • Defining dependencies per-package on the backend side
  • Changing how the backend sends the schema to the frontend, by using the category and group hierarchy (package > category > node group > node) rather than just a list of all nodes

@RunDevelopment
Copy link
Member

Renamed "subcategories" to "node groups"

👍

Move metadata things into the decorator. Now the constructor is only used for input & output definitions and things like saying if a node has side effects. This is nice because it now gives a clear separation between what is metadata and what is functionality.

Input/output declarations, side effects, etc. are also metadata. They describe the behavior of the node. Literally everything except the run function is metadata. (This is why we do NodeClassName().run(*args) when calling nodes from other nodes. Nothing except the run function affects functionality.)


One question I would like to raise is whether nodes should be classes.
By their very nature, classes are stateful. But nodes are expected to be stateless. As I see it, a node is effectively just a function with some metadata. (Metadata isn't state because it's not specific to a class instance, it's essentially a static constant.)

Conceptually, nodes are just function + metadata, so why would we need a class?

Using functions for nodes would also simplify the API. Example:

@register(
    schema_id="chainner:image:load",
    group=node_group,
    name="Load Image",
    description="Load image from specified file.",
    icon="BsFillImageFill",
    inputs=[
        ImageFileInput(primary_input=True),
    ],
    outputs=[
        LargeImageOutput(),
        DirectoryOutput("Image Directory", of_input=0),
        FileNameOutput("Image Name", of_input=0),
    ],
)
def load_image(path: str) -> Tuple[np.ndarray, str, str]:
    ...

Of course, internally in the registry, we still need (data)classes to hold the function + metadata pair.

@joeyballentine
Copy link
Member Author

I actually completely agree with that. Especially since I've been getting a warning not to use decorators with classes 😆

This would basically make them the equivalent of function components in react 👍

Side thought: i still would absolutely love if there was a way to type validate the "props" of the run function based on the inputs metadata. I doubt that's possible, but i wish it was.

@RunDevelopment
Copy link
Member

RunDevelopment commented Mar 27, 2023

Side thought: i still would absolutely love if there was a way to type validate the "props" of the run function based on the inputs metadata. I doubt that's possible, but i wish it was.

Python's type system doesn't have powerful-enough generics and literal types. It would be possible in TypeScript, not python.

We could do runtime checks, though. The __annotations__ prop gives us all typed args + the return type. So we could somewhat verify input and outputs types. The only issue is that it doesn't contain untyped args. At the very least, we could verify the return type.

>>> def foo(a: int, b: str) -> float:
...     return 0
>>> foo.__annotations__
{'a': <class 'int'>, 'b': <class 'str'>, 'return': <class 'float'>}

>>> def bar(a, b: str):
...     pass
>>> bar.__annotations__
{'b': <class 'str'>}

@joeyballentine
Copy link
Member Author

joeyballentine commented Mar 27, 2023

Theoretically, could it be done through a vscode extension or something? So not through python directly

EDIT: Or maybe even as a CI step?

@joeyballentine joeyballentine force-pushed the import-system-proof-of-concept-2 branch from 13e8ccb to 8d891ae Compare March 28, 2023 05:32
backend/src/api.py Outdated Show resolved Hide resolved
backend/src/api.py Outdated Show resolved Hide resolved
@joeyballentine
Copy link
Member Author

joeyballentine commented Mar 29, 2023

I have no idea why the CI is failing. As in, I don't know why the dictionary size is changing during iteration. I don't get that same issue locally

EDIT: wrapped it in list to force it to make a copy. Looks like that fixed it

@joeyballentine joeyballentine marked this pull request as ready for review April 1, 2023 17:16
@joeyballentine joeyballentine changed the title (WIP) New import system proof of concept v2 New import system proof of concept v2 Apr 1, 2023
@joeyballentine joeyballentine changed the title New import system proof of concept v2 New Node Import System Apr 1, 2023
backend/src/chain/chain.py Outdated Show resolved Hide resolved
backend/src/chain/chain.py Outdated Show resolved Hide resolved
backend/src/run.py Outdated Show resolved Hide resolved
backend/src/run.py Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member

Nodes within node groups are not sorted by name. This is a pretty minor frontend thing, so I think we can also fix that after merging this.

image

@RunDevelopment
Copy link
Member

There are still a couple of minor things that we should consider changing (node function names, name enforcement, implementation details of registry, node order), but none are pressing enough to warrant holding this PR back. We can make follow-up PRs for those.

@joeyballentine joeyballentine merged commit 3effb7a into main Apr 5, 2023
@joeyballentine joeyballentine deleted the import-system-proof-of-concept-2 branch April 5, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants