Skip to content
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

userText cannot be null or empty caused by incomplete/incorrect check for messages in DefaultChatClientClass #1665

Open
github2atauhiq opened this issue Nov 4, 2024 · 3 comments

Comments

@github2atauhiq
Copy link

DefaultChatClientClass is assuming that the last message in the messages array is of type "USER".
If the last message is not of type USER, userText is not being set and the subsequent null check will fail, although a USER message has been provided:

Pseudo-Code that causes the problem:

chatClient .prompt( new Prompt( List.of( createUserMessage(request), createSystemMessage() ))) .call() .chatResponse();

will result in :

java.lang.IllegalArgumentException: userText cannot be null or empty
at org.springframework.util.Assert.hasText(Assert.java:240) ~[spring-core-6.1.10.jar:6.1.10]
at org.springframework.ai.chat.client.advisor.api.AdvisedRequest.(AdvisedRequest.java:87) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
at org.springframework.ai.chat.client.DefaultChatClient.toAdvisedRequest(DefaultChatClient.java:129) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
at org.springframework.ai.chat.client.DefaultChatClient$DefaultCallResponseSpec.doGetChatResponse(DefaultChatClient.java:483) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
at org.springframework.ai.chat.client.DefaultChatClient$DefaultCallResponseSpec.lambda$doGetObservableChatResponse$1(DefaultChatClient.java:477) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
at io.micrometer.observation.Observation.observe(Observation.java:565) ~[micrometer-observation-1.13.1.jar:1.13.1]
at org.springframework.ai.chat.client.DefaultChatClient$DefaultCallResponseSpec.doGetObservableChatResponse(DefaultChatClient.java:477) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
at org.springframework.ai.chat.client.DefaultChatClient$DefaultCallResponseSpec.doGetChatResponse(DefaultChatClient.java:461) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]
at org.springframework.ai.chat.client.DefaultChatClient$DefaultCallResponseSpec.chatResponse(DefaultChatClient.java:505) ~[spring-ai-core-1.0.0-SNAPSHOT.jar:1.0.0-SNAPSHOT]

Offending lines from DefaultChatClient, method toAdvisedRequest ll 109ff

List<Message> messages = inputRequest.messages; if (!StringUtils.hasText(userText) && !CollectionUtils.isEmpty(messages)) { Message lastMessage = (Message)messages.get(messages.size() - 1); if (lastMessage.getMessageType() == MessageType.USER) { UserMessage userMessage = (UserMessage)lastMessage; if (StringUtils.hasText(userMessage.getContent())) { userText = lastMessage.getContent(); }

Please make no assumptions about the order of messages an external caller to your API may provide.

@ThomasVitale
Copy link
Contributor

ThomasVitale commented Nov 4, 2024

@github2atauhiq thanks for reporting this. Could you elaborate a bit more on this specific use case where there is no user message as the last one in a list of messages sent to an LLM?

ChatClient requires each AI workflow to have a user message well identified. Either directly (via .user()) or as part of a list of messages (via .prompt() or .messages()). In the latter case, ChatClient would try to find the user message as the last element of the list of messages (which is also what an LLM provider would do in the presence of multiple messages). If neither places contain a user message, the ChatClient cannot guarantee the integrity and consistency of its behaviour, so it's important to get the exception thrown. By design, a user message needs to be provided. Or else, all the augmentations that it could provide like memory, RAG or guardrails would not be predictable nor robust enough.

Besides ensuring a clear and consistent behaviour for ChatClient, this requirement also helps preventing mistakes when interacting with a model. LLMs expect to get a user message. If there's a list of messages, LLMs look for the user message to answer to as the last one in the list. If that's not true, they would still answer something, but the outcome is even more unpredictable than it usually is, because the order of messages matters and therefore changes the LLM behaviour, sometimes in a very significant way. Some models would not process the misplaced user message at all and answer completely unexpected things.

@github2atauhiq
Copy link
Author

Hello Thomas,

thank you for your feedback.
I wasn't aware of any particular order that the message must to be supplied in. There is no hint in the documentation that specifies that the prompts have to be ordered in a certain way. There weren't any issues so far with providing the user-message as the first message in the list.
Since spring-ai is meant to be an abstraction, why do I have to care about the order of the messages that the underlying LLM is expecting anyways?
I also find it odd, that i am able to construct an instance of Prompt although the order is incorrect. I would have expected the API to fail immediately and throw a more specific exception.

Of course, it is no problem for us to change the order (we are already doing that), but the behavior of spring-ai regarding this topic clearly changed between last week and this week and that was reason enough for me to indicate this to you.

Best regards

Manuel

@ThomasVitale
Copy link
Contributor

ThomasVitale commented Nov 4, 2024

Thanks a lot for providing additional context. Before, this same behaviour was happening, but in case no user message was found, one was created with empty content, which was leading to unpredictable outcomes (especially when using Advisors). Now, there is a check to ensure a proper user message is identified, so to ensure consistency and no surprises in the resulting behaviour. You can blame me for this change :) I'm also working on some additional documentation and release notes before this change is released in the upcoming milestone.

The underlying ChatModel abstraction doesn't assume anything, so you can build the messages any way you want. The ChatClient is a higher-level API that brings some boundaries to be able to build such AI workflows in a robust way. For example, when using RAG via an Advisor, it will not work as expected if the user message is not identified correctly.

I wonder if there could be a way to make this more flexible, while maintaining the null-safety and predictability of the API. For example, would it work having ChatClient accept a lambda to use for customising the "user message identification" strategy? What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants