refactor: separate BatchTaskDispatcher from WorkflowExecutor#613
refactor: separate BatchTaskDispatcher from WorkflowExecutor#613
BatchTaskDispatcher from WorkflowExecutor#613Conversation
Summary of ChangesHello @garrett4wade, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring effort by introducing a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the WorkflowExecutor by extracting a generic BatchTaskDispatcher. This is a great improvement that enhances modularity and will enable code reuse as intended. The separation of concerns makes the WorkflowExecutor much cleaner, especially the prepare_batch method.
I've found a few issues to address:
- A potential bug in the capacity calculation within
active_submit_and_wait. - A redundant variable assignment.
- An incorrect exception type in an
exceptblock.
Overall, this is a solid refactoring. Please see my detailed comments below.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring by separating the generic BatchTaskDispatcher from the WorkflowExecutor. This change greatly improves modularity, making the task submission and processing logic more reusable. The introduction of the VersionProvider protocol to decouple StalenessManager is also a commendable design choice. The overall architecture is cleaner and more maintainable.
One point of attention is that the change to the wait method's return type is a breaking change to the public API, which should be noted. I've also identified a minor performance improvement opportunity in prepare_batch where workflow resolution can be made more efficient. Overall, this is a high-quality contribution.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
9cd836a to
f75fda0
Compare
BatchTaskDispatcher from WorkflowExecutorBatchTaskDispatcher from WorkflowExecutor
rchardx
left a comment
There was a problem hiding this comment.
In general LGTM!
I may submit a PR trying to elimite the busy loop in the current workflow implementaion.
…lusionAI#613) * separate BatchTaskDispatcher from WorkflowExecutor * Update areal/core/workflow_executor.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * update docstring * update comment --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…lusionAI#613) * separate BatchTaskDispatcher from WorkflowExecutor * Update areal/core/workflow_executor.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * update docstring * update comment --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Description
The main aim of this refactor is to enable code reuse in
RolloutController. ``BatchTaskDispatcherextracts the common logic of task submission, waiting, and interactive submission-waiting loops. We can reuse the logic but with a new task that calls remote engines inRolloutController`.There's no breaking change. Previous functionalities should not be affected.
Expected to be merged after #612
Type of Change
work as expected)
Checklist
jb build docs/gemini review)