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

Configure remote working directory #545

Closed

Conversation

Gege-Wang
Copy link
Collaborator

@Gege-Wang Gege-Wang commented Jun 10, 2024

In this PR:

  • In the case of multiple daemons:
    • the dataflow.yml
nodes:
  - id: rust-node
    _unstable_deploy:
      machine: A
      local: true
    custom:
      build: cargo build -p multiple-daemons-example-node
      source: ../../target/debug/multiple-daemons-example-node
      inputs:
        tick: dora/timer/millis/10
      outputs:
        - random
  - id: runtime-node
    _unstable_deploy:
      machine: B
      local: false
      working_dir: /home/ubuntu/dora/examples/multiple-daemons
    operators:
      - id: rust-operator
        build: cargo build -p multiple-daemons-example-operator
        shared-library: ../../target/debug/multiple_daemons_example_operator
        inputs:
          tick: dora/timer/millis/100
          random: rust-node/random
        outputs:
          - status
  - id: rust-sink
    _unstable_deploy:
      machine: A
      local: true
    custom:
      build: cargo build -p multiple-daemons-example-sink
      source: ../../target/debug/multiple-daemons-example-sink
      inputs:
        message: runtime-node/rust-operator/status
  • add two items for every node. local (whether they are the same as cli) and working_dir (the working_dir of the daemon for this dataflow.
    • the local(default true) and working_dir(no default value) are all option. So if the dataflow is not distributed, you can write dataflow.yml just like now.
  • when you use absolute path
    • if you specify the working_dir, we will use this working_dir
    • if you not specify, we will change the working_dir to the home_directory(such as in linux,/home/miyamo). I do this because if you don't specify the working_dir but use absolute path, the dataflow can still run.
      .
  • when you use relative path
    • if the node are local, we can use the dataflow.yml working_dir from cli, just like now.
    • if the node is not local, we must specify working_dir for it(I also check this in check_dataflow), otherwise dora will throw an error.

follow #538 #534

@Gege-Wang Gege-Wang force-pushed the only-allow-absolute-path-on-remote branch from 41658d2 to 9dc07b1 Compare June 11, 2024 00:45
@Gege-Wang
Copy link
Collaborator Author

Gege-Wang commented Jun 11, 2024

@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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Collaborator

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

@Gege-Wang
Copy link
Collaborator Author

Gege-Wang commented Jun 11, 2024

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
Copy link
Collaborator

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.

Copy link
Collaborator

@haixuanTao haixuanTao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall :)

binaries/coordinator/src/run/mod.rs Outdated Show resolved Hide resolved
libraries/core/src/descriptor/mod.rs Show resolved Hide resolved
libraries/core/src/descriptor/validate.rs Outdated Show resolved Hide resolved
examples/multiple-daemons/dataflow.yml Outdated Show resolved Hide resolved
@Gege-Wang
Copy link
Collaborator Author

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

@Gege-Wang Gege-Wang changed the title Only allow absolute path on multiple daemons Configure remote working directory Jun 24, 2024
@haixuanTao
Copy link
Collaborator

Closed in favor of #658

@haixuanTao haixuanTao closed this Oct 7, 2024
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.

4 participants