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

(Re)allow registering new node packages #51

Closed
liamhuber opened this issue Oct 25, 2023 · 1 comment · Fixed by #53
Closed

(Re)allow registering new node packages #51

liamhuber opened this issue Oct 25, 2023 · 1 comment · Fixed by #53
Assignees

Comments

@liamhuber
Copy link
Member

For good measure I should make an issue for this. Node package registration, i.e. wf.create.register("my_nodes", *my_nodes) got turned off because the implementation was not wise and didn't play well with executors (like, at all). There's nothing wrong with the idea, of course. Joerg also has a nice modification for registering modules over on #33. I also promised somewhere to create an alias so you can just wf.register. It shouldn't be hard, it just needs to be done. Now that Node.pull is working it is my top priority.

@liamhuber liamhuber self-assigned this Oct 25, 2023
@liamhuber liamhuber mentioned this issue Oct 26, 2023
5 tasks
@liamhuber
Copy link
Member Author

This wound up being way more complicated than I anticipated.

Since we want node_function to be both a valid input argument for Function(Node) s.t. users can instantiate new nodes at runtime from the parent class and to be a static method of the class when the classes are created with a decorator (s.t. super users can compose stuff with these functional pieces inside other nodes), we get in a bit of a pickle with pickle. cloudpickle nicely captures the actual code at all times just fine, but when you (de)serialize a node created with a decorator, the copy of that class on the parent python process somehow gets cls.node_function.__globals__ modified!

Now, because it's capturing the actual code OK we keep some elements of __globals__ -- e.g. for the standard Scatter node, we still see plt in the list since the function body directly uses matplotlib. Unfortunately, the type hint annotations are somehow getting lost, e.g. Optional and np in the case of Scatter.

I still don't grok why the node_function.__globals__ are getting modified in this way on (de)serialization, but on a basic level you can look at typing.get_type_hints and see how losing these causes that method to bork inside, e.g., Function._build_input_channels.

At first I tried just naively including node_function.__globals__ (or some slightly trimmed down version) in Function.__get/setstate__, but this caused things to hang indefinitely for reasons I don't understand.

Currently my attack in #53 is to just directly assign the type hints as attributes of the dynamically created class, since all the necessary globals are available at the time the decorator is actually called, and then they'll get shipped off with the class itself just like the node_function does. So far this seems to work fine.

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 a pull request may close this issue.

1 participant