Remove deprecations in ChatClient and Advisors #2993
Conversation
ThomasVitale
force-pushed
the
remove-deprecations-chat-client
branch
2 times, most recently
from
bd1f0d2 to
d0d2882
Compare
| public Builder userTextAdvise(String userTextAdvise) { | ||
| Assert.hasText(userTextAdvise, "The userTextAdvise must not be empty!"); | ||
| this.promptTemplate = PromptTemplate.builder().template(userTextAdvise).build(); | ||
| public Builder protectFromBlocking(boolean protectFromBlocking) { |
There was a problem hiding this comment.
This is superseded by the new Scheduler input argument. I kept it for backward compatibility. And I haven't marked it as deprecated since we're trying to avoid that for RC1. But this is a candidate to be eventually removed in future releases.
| } | ||
|
|
||
| /* | ||
| * ==========* OPTIONS * ========== |
There was a problem hiding this comment.
To ensure consistency with the implementation of the lower-level APIs, we are now using the same exact merge logic we use in the ChatModel API:
ToolCallingChatOptions.mergeToolNames()ToolCallingChatOptions.mergeToolCallbacks()ToolCallingChatOptions.mergeToolContext().
So there shouldn't be any surprise about how the values are merged. We have a consistent behaviour that follows the same rules as Spring Boot:
- lists are overriden
- maps are merged
From a backward compatibility point of view, toolNames and toolCallbacks have the same behaviour as before. Instead, the toolContext was not merged correctly. Now it is.
ThomasVitale
force-pushed
the
remove-deprecations-chat-client
branch
from
d0d2882 to
2427377
Compare
| @@ -87,80 +89,76 @@ public static Builder builder(VectorStore chatMemory) { | |||
| } | |||
|
|
|||
| @Override | |||
| public AdvisedResponse aroundCall(AdvisedRequest advisedRequest, CallAroundAdvisorChain chain) { | |||
| public ChatClientResponse adviseCall(ChatClientRequest chatClientRequest, CallAdvisorChain callAdvisorChain) { | |||
There was a problem hiding this comment.
PgVectorStoreWithChatMemoryAdvisorIT now fails with
java.lang.AssertionError:
Expecting actual:
UserMessage{content='joke', properties={messageType=USER}, messageType=USER}
to be an instance of:
org.springframework.ai.chat.messages.SystemMessage
but was instance of:
org.springframework.ai.chat.messages.UserMessage
There was a problem hiding this comment.
System messages shouldn't be added to the vector store for a similarity search I guess but generally speaking having the system prompt as part of the retrieved conversation is needed.
There was a problem hiding this comment.
There was some inconsistency in behaviour between the two cases where we try to augment a SystemMessage that doesn't exist and one that does.
I fixed that and also extended the PgVectorStoreWithChatMemoryAdvisorIT to validate both scenarios.
|
Other than the vector store comment above, this can be merged. @ilayaperumalg can you work with @ThomasVitale tomorrow to get this merged please. |
* Update remaining Advisors and related classes to use the new APIs. * In AbstractChatMemoryAdvisor, the “doNextWithProtectFromBlockingBefore()” protected method has been changed from accepting AdvisedRequest to ChatClientRequest. It’s a breaking change since the alternative was not part of M8. * MessageAggregator has a new method to aggregate messages from ChatClientRequest. The previous method aggregating messages from AdvisedRequest has been removed. Warning since it wasn’t marked as deprecated in M8. * In SimpleLoggerAdvisor, the “requestToString” input argument needs to be updated to use ChatClientRequest. It’s a breaking change since the alternative was not part of M8. Same thing about the constructor. * The “getTemplateRenderer” method has been removed from BaseAdvisorChain. Each Advisor is encouraged to accept a PromptTemplate to achieve self-contained prompt augmentation operations. * Remove deprecations in ChatClient and Advisors, and update tests accordingly. * When building a Prompt from the ChatClient input, the SystemMessage passed via systemText() is placed first in the message list. Before, it was put last, resulting in errors with several model providers. Relates to spring-projectsgh-2655 Signed-off-by: Thomas Vitale <[email protected]>
ThomasVitale
force-pushed
the
remove-deprecations-chat-client
branch
from
2427377 to
1706297
Compare
|
@markpollack good catch! There was some inconsistency in behaviour between the two cases where we try to augment a SystemMessage that doesn't exist and one that does. I fixed that and also extended the |
|
@ThomasVitale Thank you for update. Rebased and merged as 4fe74d8 |
Relates to gh-2655