-
Notifications
You must be signed in to change notification settings - Fork 276
Updated per action retry using annotation and properties #1223
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
base: main
Are you sure you want to change the base?
Conversation
| "embabel.agent.platform.action-qos.default.idempotent=true", | ||
| "embabel.agent.platform.action-qos.default.backoff-multiplier=1", | ||
| "embabel.agent.platform.action-qos.default.backoff-millis=1", | ||
| "embabel.agent.platform.action-qos.agents.agent.method.max-attempts=6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, again, and thanks for the PR!
Do you think the agent retry configuration (e.g., embabel.agent.platform.action-qos.agents.agent.method.max-attempts=6) would be more suitable in this form?
For example, I’ve defined my agents like this:
@Agent(
name = "Subscription availability verification agent",
description = "Explains the availability of any subscription for a user's subscription state"
)
However, there are no validations for the @Agent.name parameter. This configuration approach would force me to rewrite my agent names to ensure safe and correct usage of Spring properties.
While unification is generally beneficial, I’m concerned that such agent names might be vulnerable at requested prompts, potentially compromising the quality of generative results from the LLM. Autonomy could use @Agent.name for the ranking operation, if user message was passed in a free form, not programmatically via the AgentInvocation class.
Maybe it would be enough to code the agent retry configuration at the annotation level? What are your thoughts on this?
| val toolGroups: Array<String> = [], | ||
| val toolGroupRequirements: Array<ToolGroup> = [], | ||
| val trigger: KClass<*> = Unit::class, | ||
| val retry: Array<Retry> = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify why Array<Retry> was used instead of a single Retry? Since the Action annotation applies per action, I’m not sure I understand the reasoning behind this choice.
The same thing to Retry.maxAttempts[] and others, it goes as several nested arrays Action.Retry[].maxAttempts[], but at DefaultActionQosProvider you always take the first.
By the way, I’m just curious in which cases I could use this in my project 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @drewlakee - thanks for looking at this!
I made it an array because I found that they can't be null in Kotlin, and I didn't want to match over the default value because I'm overriding per parameter, so the empty array would signify the value not being set. However it does seem a bit confusing. I suppose we can set it to the default value in ActionQos instead to make it more clear.
However, if the user provides the same as the default, and then provides a property also, we won't know that the user provided it in the action. You know what I mean? There would be no way to know if we match over the default value. So I was thinking it's probably better to do it like this? Unless there's a more idiomatic approach in Kotlin for this?
Thank you,
Hayden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haydenrear thanks for explanation, I just didn't think about it in that way. Considering this, looks like a good workaround for compile-time with optional parameters. Also, I looked into how Kotlin designs its annotations, and it looks like it's the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi again! I’m back with a fresh proposal about a (potentially) more consistent action retry configuration for agents in the framework :)
Don’t you think that the @Retry configuration looks a bit messy? It causes the RetryTemplate to leak its parameters into the Embabel's high-end @Action, including all the verbose details. I’ve been thinking about this and trying to challenge it in various forms, so maybe my idea could help give more space to think about.
Currently, we have a default behaviour for Action retries. Perhaps we could reuse it as a «compromise» by introducing some kind of enum‑like configuration: @Action(retryPolicy = ActionRetryPolicy.DEFAULT). This approach could benefit the encapsulation of the RetryTemplate implementation. Considering your thoughts about the confusing property matching — whether defaults come from the annotation level or from Spring’s default properties — perhaps we could introduce something like @Action(retryPolicy = ActionRetryPolicy.NONE)?
In this model, ActionRetryPolicy would only define framework‑preconfigured profiles. There could also be something like ActionRetryPolicy.FIRE_ONCE. However, if we want to override it more precisely, we could introduce a custom retry policy, such as @Action(retryPolicy = "subscription.agent"). This would force the framework to scan Spring properties like <subscription.agent>.action.retry.policy.max-attempts and others. Otherwise, it would partially adopt the default profile parameters for the retry policy — in this case, from ActionRetryPolicy.DEFAULT.
If we would like to override action retry policies per agent, we could introduce something like @Agent(actionRetryPolicy = "my.agent.name"). This would function in the same way as Spring’s property scanning for per‑action configurations.
To sum up:
ActionRetryPolicy(e.g., NONE, DEFAULT, FIRE_ONCE, etc.) — provides framework‑preconfigured retry profiles for actions.@Action.retryPolicy— offers the same functionality per action, with support for Spring properties.@Agent.actionRetryPolicy— provides the same capability per agent.- Default retry policy per action & per agent at spring properties (the values from
ActionQos)
Just wanted to share my thoughts, maybe it would be useful in some way 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @drewlakee
Do you think we should support any Spel expression? Should it only support ${property.value} only? In that case should we drop the spring idiom? Or keep the property placeholder syntax? Or should I just strip the property placeholder manually?
Thanks!
Hayden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haydenrear Honestly, I don’t have that much extensive experience with SpEL, so I can’t tell whether this is the most idiomatic spring approach for such configuration. However, after reviewing your latest commit, I believe you’ve nailed the idea perfectly 👍 I pulled the PR and experimented with the tests — the solution feels more declarative and Spring‑friendly to me. Thank you!
Considering SpEL docs, maybe at some point it would come in handy for such project needs
@Action(actionRetryPolicyExpression = "condition ? 'one' : 'two'")
fun perform() { }
@Agent(actionRetryPolicyExpression = "\${project.custom.action.retry.policy:until.success.action.retry.policy}")At least for now, it can resolve the configuration via spring properties in a convenient way — pretty useful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @drewlakee, great ideas!
2378579 to
169b932
Compare
…ith nested for action names embabelgh-1221 Signed-off-by: hayden.rear <hayden.rear@gmail.com>
169b932 to
0a10dca
Compare
Added annotations for retry for qos provider, and also properties, with nested for action names #1221 - related to #1215