-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat(hydro_cli): added basic wrapper for hydro deploy Maelstrom integration #936
base: main
Are you sure you want to change the base?
Conversation
See https://www.conventionalcommits.org/en/v1.0.0/ for the commit format and run |
also recommend rebasing on the latest main |
75c3102
to
43c5713
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.
First pass looks pretty good! Main areas for polish are around serialization/deserialization, those should be using proper Rust types under the hood rather than passing strings around.
hydro_cli_maelstrom/src/main.rs
Outdated
source_port_names: &[String], | ||
child_stdin: &mut ChildStdin, | ||
) -> Result<()> { | ||
let localhost = HashMap::from([("TcpPort", "127.0.0.1")]); |
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.
To improve the type safety here, we should use the original enum and serialize it with serde.
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.
Also, would be great to use Unix sockets when available.
hydro_cli_maelstrom/src/main.rs
Outdated
let connection_defs_str = serde_json::to_string(&connection_defs)?; | ||
let formatted_defns = format!("start: {connection_defs_str}\n"); | ||
child_stdin.write_all(formatted_defns.as_bytes()).await?; | ||
#[cfg(debug_assertions)] |
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.
eventually, we'll want to drop this altogether
hydro_cli_maelstrom/src/main.rs
Outdated
// Send the body string to the target port | ||
let body_string = serde_json::to_string(body)?; | ||
#[cfg(debug_assertions)] | ||
println!("Sending line {}", body_string); |
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.
same as above wrt removing eventually
hydro_cli_maelstrom/src/main.rs
Outdated
// {"src":"n1","dest":"c1","body":{"echo":"hello world!","msg_id":0,"in_reply_to":1,"type":"echo_ok"}} | ||
|
||
// parse line to string | ||
let raw_line: String = bincode::deserialize(&line).unwrap(); |
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 bit funky; we should probably have a special MaelstromResponse
type that's available to both this helper and the actual Hydroflow program. That way instead of using stringly-typed data we can have proper data types.
hydro_cli_maelstrom/src/main.rs
Outdated
#[tokio::main] | ||
async fn main() -> Result<()> { | ||
// let path = r"C:\Users\rhala\Code\hydroflow\target\debug\examples\echo"; | ||
let path = r"/mnt/c/Users/rhala/Code/hydroflow/target/debug/examples/maelstrom_unique_id"; |
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.
Should be read from command line args or an environment variable.
hydro_cli_maelstrom/src/main.rs
Outdated
// let port_input_str = r#"[{"maelstrom_type":"echo","port_name":"echo_in","direction":"Source"},{"maelstrom_type":"echo_ok","port_name":"echo_out","direction":"Sink"}]"#; | ||
|
||
// unique id gen setup | ||
let port_input_str = r#"[{"maelstrom_type":"generate","port_name":"gen_in","direction":"Source"},{"maelstrom_type":"generate_ok","port_name":"ok_out","direction":"Sink"}]"#; |
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.
We should declare Rust datatypes for each of these messages and use serde_json
to serialize them.
f84c542
to
9f9f757
Compare
9f9f757
to
3451d6b
Compare
e9a87eb
to
6039078
Compare
4f84634
to
0e64791
Compare
fb8b3b3
to
a2ec110
Compare
No description provided.