-
Notifications
You must be signed in to change notification settings - Fork 225
Implement a dora-openai-websocket server #1093
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
base: main
Are you sure you want to change the base?
Conversation
3f0d534
to
fa888e8
Compare
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.
Going through the CLI crate and opening a new connection to the coordinator seems a bit complicated, given that we already have a connection to a daemon through the node API. The daemon is already connected to the coordinator, so it could forward some start message to it. This way, we don't need to add the heavy CLI dependency and the code would work in distributed dataflows too.
So how about the following approach:
- We add a new
DaemonRequest::StartDataflow
variant- First field is
dataflow
, i.e. the path to the dataflow YAML file - The second field is
uv
(optional) - Third field is
name
(optional) - No need for coordinator addr and port fields.
- First field is
- We add a new
DoraNode::start_dataflow
method that sends aDaemonRequest::StartDataflow
request to the connected daemon - The daemon handles the
StartDataflow
request by reading the dataflow descriptor and session file and then sending a newCoordinatorRequest::StartDataflow
request to the coordinator - The coordinator handles this
CoordinatorRequest::StartDataflow
request in a similar way as theControlRequest::Start
that is sent by the CLI
dora_cli::command::Command::Start(Start { | ||
dataflow, | ||
name: Some(node_id.to_string()), | ||
coordinator_addr: IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), | ||
coordinator_port: DORA_COORDINATOR_PORT_CONTROL_DEFAULT, | ||
attach: false, | ||
detach: true, | ||
hot_reload: false, | ||
uv: true, | ||
}) | ||
.execute() |
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.
This is a synchronous call, which might block the async task. This is not recommended since it blocks a full thread of the tokio runtime.
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.
I agree. Although, this is more of a proof of concept. I think it's fine to just leave this sync call for now and rework this later.
coordinator_addr: IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), | ||
coordinator_port: DORA_COORDINATOR_PORT_CONTROL_DEFAULT, |
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.
So this only works when the node is running on the same machine than the coordinator. This seems quite limiting.
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.
Agreed I will make this configurable.
The drawback of this approach is that this adds more functionality to the dora-node-api. So this only makes sense if we think that a functionality like this is common enough and also useful for other nodes.
An alternative way to make the CLI dependency less heavy could be to make the |
So I understand where this is coming from and I agree that it is annoying to have to link the whole cli library. I think the thing is that implementing what you're mentioning is a lot of work as we would need to reimplement the node - daemon connection which I don't want to do at this moment. I would rather just keep this easy fix to just put those function public for now... |
I'm fine with making the functions public for now, but if this functionality is something that will also be needed by other nodes, I'd prefer a proper solution. The main limitation I'd like to avoid is that the coordinator currently has to run on the same machine as the node. Going through the daemon instead of the CLI would enable this functionality also in distributed settings.
I don't think that it's too much work. Let me try to draft a PR against your PR. |
afd751b
to
1a626ad
Compare
What's the status of this? How does this PR relate to #1122 ? |
1df28d1
to
42a4c61
Compare
Add HOST and PORT environment variable support with defaults: - HOST defaults to 0.0.0.0 (allow external connections) - PORT defaults to 8123 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
2c24307
to
bc38f19
Compare
This PR introduces a dora-openai-websocket that makes it possible to connect an openai client through dora in realtime.
The server is able to spawn and connect to dataflows allowing to customize the dataflow from the user client.
Getting started
See README.md at examples/openai-realtime/README.md