Add the feedback mechanism for the retry component #459
Conversation
Qodana for JVM381 new problems were found
@@ Code coverage @@
+ 67% total lines covered
8732 lines analyzed, 5932 lines covered
# Calculated according to the filters of your coverage tool☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
| */ | ||
| public sealed interface ConditionResult { | ||
| /** | ||
| * Indicates whether the current instance of `ConditionResult` represents a successful state. |
There was a problem hiding this comment.
nit: When mentioning symbols, please use square brackets instead of backticks: [ConditionResult]. This would create a proper reference when rendering docs, making it slightly easier to navigate.
| @@ -34,26 +94,61 @@ public data class RetrySubgraphResult<Output>( | |||
| * @param name The optional name of the subgraph. | |||
| * @param defineAction A lambda defining the action subgraph to perform within the retry subgraph. | |||
| */ | |||
| @Deprecated("Use `condition: suspend AIAgentContextBase.(Output) -> ConditionResult` instead") | |||
There was a problem hiding this comment.
Let's remove instead of deprecating it. It's fine since we are not stable yet.
| @@ -124,6 +237,42 @@ public inline fun <reified Input : Any, reified Output> AIAgentSubgraphBuilderBa | |||
| * @param strict If true, an exception is thrown if the condition is not met after the maximum retries. | |||
| * @param name An optional name for the subgraph. | |||
| * @param defineAction A lambda defining the actions within the subgraph. | |||
| */ | |||
| @Deprecated("Use `condition: suspend AIAgentContextBase.(Output) -> ConditionResult` instead") | |||
There was a problem hiding this comment.
Here as well, let's remove
| * | ||
| * @property feedback A descriptive message or information explaining the reason for the rejection. | ||
| */ | ||
| public class RejectWithFeedback(public val feedback: String): ConditionResult |
There was a problem hiding this comment.
I wonder if we need two separate Reject classes. Can we simply add nullable feedback field to the Reject class with default null value?
agents/agents-ext/src/commonMain/kotlin/ai/koog/agents/ext/agent/SubgraphWithRetry.kt
Show resolved
Hide resolved
| // Add clarification messages to the prompt | ||
| val clarificationMessages = storage.getValue(clarificationMessagesKey) | ||
| llm.writeSession { | ||
| updatePrompt { |
There was a problem hiding this comment.
So on each iteration we are adding all existing clarification messages again? I wonder if this would be an optimal approach, I feel like we would waste tokens this way. How about reworking this approach a bit? We store only the latest clarification message in the storage (not list) and append only it if it's present. All previous messages would be already present in the prompt anyway. And to implement this properly, after every iteration calrificationMessageKey in the storage has to be explicitly cleared if there's no actual clarification message after this iteration, to avoid setting message from the previous iteration twice.
antoniibelyshev
force-pushed
the
belyshev/retry-component-with-feedback
branch
from
0bf3fc3 to
58b449d
Compare
antoniibelyshev
force-pushed
the
belyshev/retry-component-with-feedback
branch
from
f5d39df to
46f09b9
Compare
antoniibelyshev
dismissed
EugeneTheDev’s stale review
I addressed the requested changes. If you have other suggestions regarding the retry component, we can discuss them, and I will make updates if needed.
Thank you for opening a pull request! Please add a brief description of the proposed change here.
Also, please tick the appropriate points in the checklist below.
KG-188
Allow to provide condition description and feedback in the retry component.
Type of the change
Checklist for all pull requests
developas the base branchAdditional steps for pull requests adding a new feature