Refactor: Improve logic and error handling in KAGIterativePipeline - #546
Merged
xionghuaidong merged 1 commit intoJun 2, 2025
Merged
Conversation
This commit addresses several issues I identified during a detailed review of `kag/solver/pipeline/kag_iterative_pipeline.py`:
1. **Correct `max_iteration` Docstring:** The docstring for `max_iteration` in `__init__` now correctly states `(default: 5)` to match the actual default value in the code.
2. **Improved Context Management:** Tasks are now added to the `context` only *after* their successful execution. Previously, tasks were added before execution, which could lead to an inaccurate context if an error occurred. The final task is added before breaking the loop.
3. **Clearer Termination on Max Iterations:**
* I introduced a `MaxIterationsReachedError` custom exception.
* The pipeline now tracks if it was terminated after the final step.
* If the `max_iteration` limit is reached without the final step being called, the pipeline raises `MaxIterationsReachedError` instead of proceeding to generate a potentially misleading answer.
**Design Note (Further Discussion Recommended):**
A significant design concern remains regarding how `KAGIterativePipeline` processes tasks. I've observed that the pipeline currently only processes the first task in each iteration, discarding any others. This might be an underutilization of capabilities or a point of inefficiency. This behavior has not been changed but warrants further discussion and potential future refactoring if the pipeline is intended to handle more complex plans within a single iteration.
xionghuaidong
pushed a commit
that referenced
this pull request
Jun 26, 2025
) This commit addresses several issues I identified during a detailed review of `kag/solver/pipeline/kag_iterative_pipeline.py`: 1. **Correct `max_iteration` Docstring:** The docstring for `max_iteration` in `__init__` now correctly states `(default: 5)` to match the actual default value in the code. 2. **Improved Context Management:** Tasks are now added to the `context` only *after* their successful execution. Previously, tasks were added before execution, which could lead to an inaccurate context if an error occurred. The final task is added before breaking the loop. 3. **Clearer Termination on Max Iterations:** * I introduced a `MaxIterationsReachedError` custom exception. * The pipeline now tracks if it was terminated after the final step. * If the `max_iteration` limit is reached without the final step being called, the pipeline raises `MaxIterationsReachedError` instead of proceeding to generate a potentially misleading answer. **Design Note (Further Discussion Recommended):** A significant design concern remains regarding how `KAGIterativePipeline` processes tasks. I've observed that the pipeline currently only processes the first task in each iteration, discarding any others. This might be an underutilization of capabilities or a point of inefficiency. This behavior has not been changed but warrants further discussion and potential future refactoring if the pipeline is intended to handle more complex plans within a single iteration. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit addresses several issues I identified during a detailed review of
kag/solver/pipeline/kag_iterative_pipeline.py:Correct
max_iterationDocstring: The docstring formax_iterationin__init__now correctly states(default: 5)to match the actual default value in the code.Improved Context Management: Tasks are now added to the
contextonly after their successful execution. Previously, tasks were added before execution, which could lead to an inaccurate context if an error occurred. The final task is added before breaking the loop.Clearer Termination on Max Iterations:
MaxIterationsReachedErrorcustom exception.max_iterationlimit is reached without the final step being called, the pipeline raisesMaxIterationsReachedErrorinstead of proceeding to generate a potentially misleading answer.Design Note (Further Discussion Recommended):
A significant design concern remains regarding how
KAGIterativePipelineprocesses tasks. I've observed that the pipeline currently only processes the first task in each iteration, discarding any others. This might be an underutilization of capabilities or a point of inefficiency. This behavior has not been changed but warrants further discussion and potential future refactoring if the pipeline is intended to handle more complex plans within a single iteration.