dora-rs / dora

DORA (Dataflow-Oriented Robotic Architecture) is middleware designed to streamline and simplify the creation of AI-based robotic applications. It offers low latency, composable, and distributed dataflow capabilities. Applications are modeled as directed graphs, also referred to as pipelines.
https://dora-rs.ai
Apache License 2.0
1.5k stars 79 forks source link

Configure remote working directory #545

Open Gege-Wang opened 3 months ago

Gege-Wang commented 3 months ago

In this PR:

follow #538 #534

Gege-Wang commented 3 months ago

@phil-opp since the dataflow check is needed in cli and coordinator, I prefer to skip all path exist check in multiple daemons. the path exist check should be done by per-daemon, instead of cli and coordinator(maybe?). I changed the path(in ubuntu case) and tried this PR, the CI in my workflow and EC2 is good, but it seems there are some No left space error

phil-opp commented 3 months ago

Thanks for the PR! I'm not sure whether it's a good idea to implicitly change the working directory based on the number of machines. But we could add a config option to set a working directory for each machine. Then relative paths on those machines could be allowed again.

the path exist check should be done by per-daemon, instead of cli and coordinator(maybe?).

This seems like a good approach too. The CLI could send a CheckPaths message to the coordinator on dora check, which forwards it to the correct daemon for each node. The daemon could then check whether the path exists and report back to the coordinator, which then reports back to the CLI. For dora start this could be implemented as part of the existing Start message. What do you think about this idea @haixuanTao?

haixuanTao commented 3 months ago

@phil-opp since the dataflow check is needed in cli and coordinator, I prefer to skip all path exist check in multiple daemons.

the path exist check should be done by per-daemon, instead of cli and coordinator(maybe?).

I changed the path(in ubuntu case) and tried this PR, the CI in my workflow and EC2 is good, but it seems there are some No left space error

The no left space error is probably independent of your PR.

haixuanTao commented 3 months ago

Thanks for the PR! I'm not sure whether it's a good idea to implicitly change the working directory based on the number of machines. But we could add a config option to set a working directory for each machine. Then relative paths on those machines could be allowed again.

the path exist check should be done by per-daemon, instead of cli and coordinator(maybe?).

This seems like a good approach too. The CLI could send a CheckPaths message to the coordinator on dora check, which forwards it to the correct daemon for each node. The daemon could then check whether the path exists and report back to the coordinator, which then reports back to the CLI. For dora start this could be implemented as part of the existing Start message. What do you think about this idea @haixuanTao?

Sounds good to me ☺️

haixuanTao commented 3 months ago

Would prefer to have a separate PR for working directory as we wanted to have abs path only for this PR.

Gege-Wang commented 3 months ago

I'm not sure whether it's a good idea to implicitly change the working directory based on the number of machines.

I have considered this question. since the check path and working_dir is relevant, in this pr, for multiple daemons in one dataflow, we must all use abs path, the working_dir seems like make no sense , except generator log(maybe?).

But we could add a config option to set a working directory for each machine.

yes, I have though about this, making working_dir configurable for every daemon, the implementation would conflict with the current implementation(the default /tmp).

phil-opp commented 3 months ago

Would prefer to have a separate PR for working directory as we wanted to have abs path only for this PR.

The abs path only is implemented in #538.

Gege-Wang commented 3 months ago

I left some issues about absolute path in #535 and why this PR is conflict with #538.