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

Remove dynamic node from pending nodes before starting a dataflow #606

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

haixuanTao
Copy link
Collaborator

This PR makes a dataflow start independently from dynamic nodes.

Previously, all nodes wait for each other to be ready before starting the dataflow, this makes it possible to have synchronised messages, and avoid feeling up queues before the nodes is even ready to receive message. I think in the case of dynamic node, we don't want to wait for users to spawn the nodes to start the dataflow.

This creates the possibility to specify some nodes that can "tap" on the dataflow as well as remove the risk of having race condition if dynamic nodes needs to be specified in a specific order.

One recent example is RJ wanting to have multiple nodes within one python process and this was not possible due to nodes being pending.

…e flexibility to spawn them independently from starting the dataflow. also, dynamic nodes might create race condition when several are sequential
@phil-opp
Copy link
Collaborator

I think we should keep the option to wait for dynamic nodes before starting the dataflow. For some nodes, it might be important to receive all messages from the beginning. With the current implementation, this would not be possible anymore.

How about we add a separate config flag for that? E.g. something like synchronized_start defaulting to true. If changed to false the dataflow starts without waiting for the node.

@haixuanTao
Copy link
Collaborator Author

I think that if people want to have synchronised nodes they should use normal nodes.

The problem with adding another parameter is that it add another layer of convolution of potential race condition that I genuinely don't think is worth it.

I can feel like it's going to be a recurring failure that people block a dataflow because of a dynamic node not started, which I think is hard to debug and dynamic node are easy to forget.

FYI, my reason for this pr is being able to have unsynchronised dynamic node makes it possible to scale dataflow much more than currently possible: debugging node, plot node, tap node and so on.

@haixuanTao haixuanTao enabled auto-merge August 8, 2024 09:12
@haixuanTao haixuanTao disabled auto-merge August 8, 2024 09:12
@haixuanTao haixuanTao merged commit d4e629e into main Aug 8, 2024
38 checks passed
@haixuanTao haixuanTao deleted the make-dynamic-node-not-pending branch August 8, 2024 09:12
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