Configure exit policy of AgentValidationManager using a property - #1801
Configure exit policy of AgentValidationManager using a property#1801deleSerna wants to merge 7 commits into
Conversation
|
@deleSerna please confirm this PR is for Embabel 1.5.0 release vs 1.0.1 |
igordayen
left a comment
There was a problem hiding this comment.
@deleSerna - thank you for your contribution, left few comments. Also: how does this check correlate with generic Agent validation method. thanks
igordayen
left a comment
There was a problem hiding this comment.
@deleSerna - could you please consider aligning with:
src/main/kotlin/com/embabel/agent/spi/validation
thanks
@azanux - FYI
|
@deleSerna - build failed; expected failure? |
|
@igordayen Could you clarify what do you meant by this? Move the check to |
https://github.com/embabel/embabel-agent/wiki/Working-with-Multiple-Releases |
I will check. Since both tests around the same criteria, may be VoidAchievesGoalJavaTest need an update . I will check it tomorrow. |
@igordayen In my case, should I just need to target to 1.0.x and would whoever merging the PR will take care of committing that to 1.5.0 and main? |
@deleSerna it us actually developer responsibility to commit to both branches, if required. 1.5.0 - is actually "main" branch, thanks |
Yes, ideally validation to be aligned with existing package structure and good reason why it can not be incorporated into agent structure validation. Thank you. |
Understood, why it's failed. I have wrongly interpreted that only plan type with |
We don't fail on them at the moment. On the other hand, IMO, there is no point in a validation failure that would not result in actual failure as people simply won't notice such warnings. I can create a separate ticket for this and then decide in which branch that should go. |
|
@deleSerna, may I suggest creating an AchievableGoalValidator in a package with existing Agent Validators and employing it? |
There was a problem hiding this comment.
(!method.returnType.isInterface || !requireInterfaceDeserializationAnnotations || hasRequiredJsonDeserializeAnnotationOnInterfaceReturnType(
@igordayen Do you know why are we checking this for a method to considered as Action so that I can document accordingly.
igordayen
left a comment
There was a problem hiding this comment.
@deleSerna - more comments added. more leaning to configurable policy. complex conditions require proper documentation, i would suggest.
PR is having conflicts too.
Thank you
| requireInterfaceDeserializationAnnotations : Boolean, | ||
| ): Boolean { | ||
| return method.isAnnotationPresent(Action::class.java) && | ||
| (agentClass.declaredMethods.contains(method) || isMethodFromSupertype(method, agentClass)) && |
There was a problem hiding this comment.
not easy to read without comments
| /** | ||
| * Returns true, if the given method is declared in its super type. | ||
| */ | ||
| fun isMethodFromSupertype( |
There was a problem hiding this comment.
i think spring has a utility for this
There was a problem hiding this comment.
I have not seen a direct equivalent of this in org.springframework.util.ClassUtils. Do you know which one are you referring here?
|
For multi-agent deployment, single-agent failure should not block server bootstrapping, my thinking |
But there are other validation errors already for which already fail. eg: here . Do you think this violation is different from those ones? |
==> Could you please document the behavior in English, in your own words? Thanks. Related to goals dup too, looping @tuannx |
Therefore, IMO, we could also return null and fail here as this validation error is similar to the cases I mentioned above. |
Please compile full documentation on known agent validators behavior for consistency. Thanks |
|
@deleSerna - please see conflicts. And- what is more important - per analysis, deployments should not be rejected. Thanks |
@igordayen Sure, I could fix the conflicts. As there are multiple PRs around the similar topic (#1833 ), I am a bit confused how to proceed further here. Could you please clarify which one among the below path should follow?
|
|
@deleSerna - please see the in-depth discussion here: Thank you. |
No that is not true. For some structure violation, we are still failing. See AgentMetadataReader.kt#L218 I would argue that AchievesGoal missing Action is a spec violation and we should just stop the agent. @igordayen Do you disagree with that? |
==> thanks @deleSerna , @tuannx - missing in your analysis? thanks |
@deleSerna @alexheifetz I've just verified: What
|
| Caller | Line | On null |
Effect |
|---|---|---|---|
AgentScanningPostProcessorDelegate |
AutoRegistration.kt:57-70 |
if (agentMetadata != null), then return bean |
Agent is skipped, the application boots normally. This is the Spring path |
SingleInstanceAgentScopeBuilder |
AgentScopeBuilder.kt:59-60 |
?: throw IllegalArgumentException |
Throws while building the scope |
FromInstancesAgentScopeBuilder |
AgentScopeBuilder.kt:71-72 |
?: throw IllegalArgumentException |
Throws while building the scope |
RunSubagent.handle |
RunSubagent.kt:35 |
... as Agent — unchecked cast on a nullable |
NullPointerException at runtime, mid-action |
The deciding lines are in AutoRegistration.kt:
val agentMetadata = agentMetadataReader.createAgentMetadata(bean) // :57
if (agentMetadata != null) { // :58
...
agentPlatform.deploy(agentMetadata) // :67
}
return bean // :70return bean sits outside the if, so the bean is handed back to Spring and the context
finishes refreshing. return null has never blocked bootstrapping. It skips one agent
and the application starts with that capability missing.
Two more drop a goal rather than the agent, so the agent still deploys:
| Line | Condition | Log level |
|---|---|---|
| 444 | @AchievesGoal method returns void |
ERROR |
| 713 | @AchievesGoal method returns void |
ERROR |
Summary
In this codebase "rejecting an agent" has never meant stopping the server. It means quietly
doing less. Seven conditions already end that way, and the only thing that varies between
them is the log level — DEBUG, WARN or ERROR — chosen without an evident rule.
The same return null is benign on the Spring path, an IllegalArgumentException when
building a scope, and a NullPointerException at runtime under RunSubagent.
@tuannx @igordayen Agree that wording |
|
Summarizing - please correct if required: |
IMO, we should have been returning NULL in AgentMetaReader for the validation failure cases too.
IMO, we should skip the agent
|
|
@deleSerna - if you're referring to the piece with bull commented out - and if this is the only place - then I would still think to go with configurability. @tuannx FYI. We need consensus on this. Asking to link related issues. |
At present, AgentValidationManager and DefaultAgentValidationManager seem to be only used in AgentMetaDataReader other than tests. Therefore, ok to make it's behavior configure via configurable property. DefaultAgentStructureValidator is @component therefore, user can explicitly inject that it won't be covered by the configurable property. Would also like to see behavior of AchievableGoalValidator, introduced this in PR, to be controlled by the same configurable property? |
|
I'm getting confused, as I thought it would be applied in a single place in the code, specifically where "null" gor commented out. |
I can make But there are other checks in the AgentMetadadatReader that is not part of I was not planning to move those 6 checks behind the configurable property as it's a change in behavior. Is this clear now? |
Signed-off-by: deleSerna <nadeeshtv@gmail.com>
Signed-off-by: deleSerna <nadeeshtv@gmail.com>
Signed-off-by: deleSerna <nadeeshtv@gmail.com>
Thanks, @deleSerna - now 100% clear. "null"==> skipAgentDeploymentIfRequired API, and inside it apply config check? At least the name is more suggestive than just 'null' in the very first place. And add a paragraph to adoc. |
Signed-off-by: deleSerna <nadeeshtv@gmail.com>
| return method.isAnnotationPresent(Action::class.java) && | ||
| // declared in the given agent class, or in its super type? | ||
| (agentClass.declaredMethods.contains(method) || isMethodFromSupertype(method, agentClass)) && | ||
| // TODO please fill after discussion. |
There was a problem hiding this comment.
@igordayen @alexheifetz I do not know what exactly meant by this condition . I have move dit from AgentMetadaReader. Could one of you help me in filling the TODO here?
(!method.returnType.isInterface || !requireInterfaceDeserializationAnnotations || hasRequiredJsonDeserializeAnnotationOnInterfaceReturnType(
There was a problem hiding this comment.
@deleSerna im offline today, seeing very substantial changes, and getting concerned. Will try to review code tomorrow . This week main focus on release 2.0.0 with remaining issues only due to spring ai upgrade. Thank you for understanding
Signed-off-by: deleSerna <nadeeshtv@gmail.com>
Signed-off-by: deleSerna <nadeeshtv@gmail.com>
| targetType.name, | ||
| ) | ||
| return null | ||
| if (skipAgentDeploymentOnError) { |
There was a problem hiding this comment.
@igordayen Can you look into EmbabelMockitoIntegrationTestBlockingTest. It's failing because the it's not returning by default.
Looks likeAutoRegistration.kt can pick up any Bean and it's really depend on this check to not behind any condition.
Therefore, I am not putting return null behind property check.
Issue - #1796
Context - Currently, we are not consistently handling whenever an Agent is failing to adhere to a valid structure ( annotated properly, parameters and return type as expected etc..). In some cases, we are skipping such agent and in some cases, we are just logging a message.
Solution