-
Notifications
You must be signed in to change notification settings - Fork 102
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
Enable downloading remote dataflow #682
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,6 @@ | |
}; | ||
use eyre::{ContextCompat, WrapErr}; | ||
use std::{ | ||
env::consts::EXE_EXTENSION, | ||
path::{Path, PathBuf}, | ||
process::Stdio, | ||
sync::Arc, | ||
|
@@ -101,13 +100,10 @@ | |
source => { | ||
let resolved_path = if source_is_url(source) { | ||
// try to download the shared library | ||
let target_path = Path::new("build") | ||
.join(node_id.to_string()) | ||
.with_extension(EXE_EXTENSION); | ||
download_file(source, &target_path) | ||
let target_dir = Path::new("build"); | ||
download_file(source, &target_dir) | ||
.await | ||
.wrap_err("failed to download custom node")?; | ||
target_path.clone() | ||
.wrap_err("failed to download custom node")? | ||
Comment on lines
-104
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you clarify the reason for this breaking change? By renaming the downloaded files we avoided collisions. For example, consider: nodes:
- id: camera
path: example.com/camera/node
inputs:
tick: dora/timer/millis/20
outputs:
- image
- id: plot
path: example.com/plot/node
inputs:
image: camera/image With the new logic, the second download overwrites the first, as both nodes are stored as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand you're reasoning. But, I really don't think we should rename file on the fly as it makes it very hard to keep track of which file correspond to what. I think that if people name everything "node.py" and colludes with names, I don't think that changing every name is the right solution. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would very much prefer people use build for caching, in the likes of something like: - id: plot
build: wget -O plot.py example.com/plot/node
path: node.py
inputs:
image: camera/image There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The thing is that the two
For the above example, you don't have the source for one of the nodes at all (because it's overwritten).
Sure, this is a good alternative. But if we support pointing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, so I guess, what we could do is fail on pre-existing files. That would make the most sense to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other alternative would be renaming, either always or only on conflict. If we don't want to do that, I agree that failing is better than silently overwriting the files of other nodes. |
||
} else { | ||
resolve_path(source, working_dir).wrap_err_with(|| { | ||
format!("failed to resolve node source `{}`", source) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,26 +27,21 @@ | |
use tracing::{field, span}; | ||
|
||
pub fn run( | ||
node_id: &NodeId, | ||
operator_id: &OperatorId, | ||
_node_id: &NodeId, | ||
_operator_id: &OperatorId, | ||
source: &str, | ||
events_tx: Sender<OperatorEvent>, | ||
incoming_events: flume::Receiver<Event>, | ||
init_done: oneshot::Sender<Result<()>>, | ||
) -> eyre::Result<()> { | ||
let path = if source_is_url(source) { | ||
let target_path = adjust_shared_library_path( | ||
&Path::new("build") | ||
.join(node_id.to_string()) | ||
.join(operator_id.to_string()), | ||
)?; | ||
let target_path = &Path::new("build"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason for this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand, but I honestly find it super hard to keep track about all the rules that we had rather than having simple link. I would rather prefer user implement their own url resolver for cross platform than choosing a url definition that people might need to remember. this might resolve some issues as you might also want to deal with architecture (x86 or arm) and I don't think we should expect people to build library for all platform. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let say I want to use the current status quo for something like: -> Knowing that none of this is going to be properly documented in the near future... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Being cross platform could be as simple as: - id: plot
build: }
wget -O plot.zip example.com/plot/$OSTYPE/node/plot.zip
unzip plot.zip
path: libplot.so
inputs:
image: camera/image There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fully agree that the status quo solution was hacky and not ideal. Of course, things get simpler if we simply ignore the platform differences. However, if we want dora to be cross-platform, we should provide some way to use this feature for cross-platform dataflows.
The issue is that the file would need to be named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we decided to remove operator support in the near future, so we might not have to deal with this anymore. However, the same things also applies to executables, which need to have an Would you be fine with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in windows you can run an exe without the See: https://stackoverflow.com/questions/44441265/calling-an-exe-without-the-extension-but-with-i FYI, i hope that most binary that we ship for dora can be packaged in a way that is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's useful for local paths, but unfortunately not for URLs. We need different binaries on Windows and Linux, so downloading the same file would not help. Also, the downloaded file would probably need an However, even if we added a path:
x86_64-unknown-linux-gnu: https://example.com/path/x86/linux/node
x86_64-pc-windows-msvc: https://example.com/path/x86/win/node.exe (Of course, this is not a high priority right now.) |
||
// try to download the shared library | ||
let rt = tokio::runtime::Builder::new_current_thread() | ||
.enable_all() | ||
.build()?; | ||
rt.block_on(download_file(source, &target_path)) | ||
.wrap_err("failed to download shared library operator")?; | ||
target_path | ||
.wrap_err("failed to download shared library operator")? | ||
} else { | ||
adjust_shared_library_path(Path::new(source))? | ||
}; | ||
|
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.