[codex] Simplify flmexec new API usage#457
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the flmexec service and client to utilize the new flame-rs macro-based entrypoint and invocation API, significantly simplifying the codebase. Key changes include replacing the FlameService trait implementation with a #[flame::entrypoint] function, moving ScriptRuntime to the script module, and removing several unused dependencies such as tonic and serde_json. In the client, the TaskInformer logic was replaced with direct task invocation. Feedback indicates that the use of try_join_all for waiting on tasks reduces resilience compared to the previous implementation, as a single failure will stop all output; switching to join_all is suggested to handle individual task results independently.
| let outputs = try_join_all(handles).await?; | ||
|
|
||
| for (task_id, output) in task_ids.into_iter().zip(outputs) { | ||
| println!( | ||
| "Task {:<10}: {:?}", | ||
| task_id, | ||
| output.map(|output| output.data).unwrap_or_default() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Using try_join_all to wait for task completion causes the entire operation to fail and exit if any single task encounters an error. This results in the loss of output for all other tasks, including those that may have completed successfully. The previous implementation using TaskInformer was more resilient, as it printed results for each task as they finished regardless of other task failures. Consider using futures::future::join_all to wait for all tasks and handle individual successes and failures independently.
let outputs = futures::future::join_all(handles).await;
for (task_id, output) in task_ids.into_iter().zip(outputs) {
match output {
Ok(output) => {
println!(
"Task {:<10}: {:?}",
task_id,
output.map(|output| output.data).unwrap_or_default()
);
}
Err(e) => {
println!("Task {:<10}: Failed with error: {e}", task_id);
}
}
}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Summary
Verification