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

feat(hydro_cli): added basic wrapper for hydro deploy Maelstrom integration #936

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RyanAlameddine
Copy link
Contributor

No description provided.

@MingweiSamuel
Copy link
Member

See https://www.conventionalcommits.org/en/v1.0.0/ for the commit format and run cargo clippy --all-targets for lints

@MingweiSamuel
Copy link
Member

also recommend rebasing on the latest main

@shadaj shadaj self-requested a review November 5, 2023 22:38
Copy link
Member

@shadaj shadaj left a 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.

source_port_names: &[String],
child_stdin: &mut ChildStdin,
) -> Result<()> {
let localhost = HashMap::from([("TcpPort", "127.0.0.1")]);
Copy link
Member

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.

Copy link
Member

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.

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)]
Copy link
Member

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

// Send the body string to the target port
let body_string = serde_json::to_string(body)?;
#[cfg(debug_assertions)]
println!("Sending line {}", body_string);
Copy link
Member

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

// {"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();
Copy link
Member

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.

#[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";
Copy link
Member

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.

// 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"}]"#;
Copy link
Member

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.

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.

3 participants