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
[FEATURE] Retrieve Documents when using AiService #1015
base: main
Are you sure you want to change the base?
[FEATURE] Retrieve Documents when using AiService #1015
Conversation
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.
@KaisNeffati thank you a lot for your contribution!
.build(); | ||
|
||
// when | ||
assertThrows(IllegalArgumentException.class, () -> assistant.answerWithNoGenericType("Can I cancel my booking?")); |
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.
minor: it would be nice to do this validation and fail on AI Service creation (eager), not on usage (lazy).
|
||
@ParameterizedTest | ||
@MethodSource("models") | ||
void should_use_query_transformer_and_content_retriever_and_through_exception_when_generic_type_is_not_set(ChatLanguageModel model) { |
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.
query transformer is not necessary for this test, please remove it to simplify the test
assertThrows(IllegalArgumentException.class, () -> assistant.answerWithNoGenericType("Can I cancel my booking?")); | ||
|
||
} | ||
|
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.
Wondering: should AI Service creation fail also when return type is WIthSources, but no retriever/augmentor is set up?
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.
Absolutely! I agree, adopting a straightforward solution like that would indeed be beneficial. Otherwise, we might encounter numerous complaints from users about the WithSources
object lacking contents or other augement information
|
||
@ParameterizedTest | ||
@MethodSource("models") | ||
void should_use_query_transformer_and_content_retriever_and_retrieve_sources(ChatLanguageModel model) { |
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.
query transformer is not necessary for this test, please remove it to simplify the test
// when | ||
assertThrows(IllegalArgumentException.class, () -> assistant.answerWithNoGenericType("Can I cancel my booking?")); | ||
|
||
} |
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.
please add a test for the scenario when structured output is required: WithSources<Pojo>
*/ | ||
@Getter | ||
@Builder | ||
public class WithSources<T> { |
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.
After a second thought, I think we should make this class a bit more generic, to accomodate for other useful information, such as:
- tools used during the call
- token usage
- etc
(Please do not add these in this PR.)
Any ideas for the good name for the class that can hold that?
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.
ResponseMessage
or ResponsePrompt
Well when we talk about framework like Spring or the javax package, they provide powerful tools for handling HTTP requests and responses. One key concept is how they handle responses.
In Spring, for example, ResponseEntity is a class that encapsulates an HTTP response, including the response body and additional metadata such as headers and status codes. It's a neat way to package up all the necessary information about a response in one object.
Similarly, in the javax package, there's the ResponseBuilder class. This class allows you to construct an HTTP response step by step, setting things like the status code, headers, and entity (the response body). It's a flexible and convenient way to craft custom responses
Now, let's bring this analogy back to our prompt usecase. Just as ResponseEntity and ResponseBuilder provide a structured way to handle HTTP responses. ResponseMessage class aims to provide a structured way to handle prompts response
By using ResponseMessage (or ResponsePrompt), we can create responses that not only contain the text of the prompt itself but also include additional contextual information, just like how ResponseEntity includes metadata with the response body. This makes our response handling more organized, easier to manage, and more adaptable to different scenarios
ResponseMessage responseMessage = ResponseMessage.create("How can I assist you today?")
.contents(...)
.tools(...)
.tokens(...)
.build();
I can include this in a seperated PR if needed
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.
I am not sure about these names, they are not really responses, and not messages/prompts.
These object can contain much more:
- Answer/Response itself (
String
orCustomPOJO
or smth else) - Retrieval:
List<Content> retrievedContents
- Used queries (not sure if needed)
- Used retrievers (not sure if needed)
- LLM calls:
TokenUsage
(for all calls combined and/or for each call separately)FinishReason
(of the last call)- Model parameters:
modelName
/temperature
/etc? - List of all calls with requests and responses (not sure if needed)?
- Tool executions:
- Executed tools and their inputs/outputs
I am having a hard time to find a good name right now, maybe it is an indicator of a bad design, or maybe I need to go and rest a bit 😆
Have a nice weekend!
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.
Taking a breather and diving into some meditation is the perfect recipe for this naming problem 😙
As for me, I'm rolling with 'WithSources' for now. But if you've got more brainwaves brewing, feel free to open another issue, and we'll keep this brainstorm party going! Yep, it's definitely more than just a response 🤔
@@ -23,5 +23,5 @@ public interface RetrievalAugmentor { | |||
* @param metadata The {@link Metadata} that may be useful or necessary for retrieval and augmentation. | |||
* @return The augmented {@link UserMessage}. | |||
*/ | |||
UserMessage augment(UserMessage userMessage, Metadata metadata); | |||
T augment(UserMessage userMessage, Metadata metadata); |
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.
This is a breaking change, is there a way to avoid this?
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.
The purpose of this is to retrieve the contents used for augmentation.
I have two solutions in mind:
- First solution: Add another method in the
RetrievalAugmentor
which returns another object containing theUserMessage
and the list of contents. - Second solution: Add the list of contents (
List<TextSegments>
) in theUserMessage
class.
I noticed another list of contents in the UserMessage
, but it's only used to specify the type of content used for augmentation.
What do you suggest to make retrieving the UserMessage
with the list of content possible in the DefaultAiServices
class?
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.
I assume that in the long term, regardless of this change, we will have to make internals of UserMessage
more fine-grained, e.g. having user message and retrieved contents separate (solution 2). I foresee that LLM APIs in the future will separate user message from supporting (retrieved) content to be able to adjust/fine-tune their LLMs better. Some LLM providers already do this (see documents
parameter). But this is pretty big change, and introduces more complexity, so I would better postpone it untill it will be really required.
I guess for now we can go with the first solution, something like this?
public interface RetrievalAugmentor {
@Deprecated // let's deprecate this
UserMessage augment(UserMessage userMessage, Metadata metadata); // existing API
default AugmentationResult augment(AugmentationRequest augmentationRequest) { // new API
UserMessage augmented = augment(augmentationRequest.chatMessage, augmentationRequest.metadata);
return new AugmentationResult(augmented, null);
}
}
// introduce request/result classes to make it more extendable in the future
public class AugmentationRequest {
ChatMessage chatMessage; // instead of UserMessage, this will be useful for issue #793
Metadata metadata;
}
public class AugmentationResult {
ChatMessage augmentedChatMessage;
List<Content> retrievedContents;
}
This feature will not work for existing custom implementations of RetrievalAugmentor
, but I guess it is fine as there are most probably not many such users and this class is explicitly marked @Experimental
.
Most users are probably using DefaultRetrievalAugmentor
, where we will have a proper implementation of the new method.
WDYT?
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.
I'll include the ChatMessage in a separate PR since it requires numerous modifications
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.
@KaisNeffati what is the change exactly? Is it breaking?
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.
To utilize ChatMessage
within both AugmentationRequest
and AugmentationResult
, the interface should be adjusted as follows:
@Deprecated
UserMessage augment(UserMessage userMessage, Metadata metadata);
UserMessage augment(ChatMessage chatMessage, Metadata metadata);
/**
* Augments the provided {@link AugmentationRequest} with retrieved content.
*
* @param augmentationRequest The {@link AugmentationRequest} containing the user message and metadata.
* @return The {@link AugmentationResult} containing the augmented user message.
*/
default AugmentationResult augment(AugmentationRequest augmentationRequest) { // new API
UserMessage augmented = augment(augmentationRequest.getChatMessage(), augmentationRequest.getMetadata());
return AugmentationResult.builder()
.augmentedUserMessage(augmented)
.build();
}
This adjustment necessitates modifying the contentInjector to accept:
UserMessage inject(List<Content> contents, ChatMessage chatMessage);
etc...
So for this PR i kept using the UserMessage , And i'll include the use of ChatMessage in another PR
@Builder | ||
public class WithSources<T> { | ||
private T response; // The response associated with the augmented information. | ||
private AugmentedMessage augmentedMessage; // Wrapper for the augmentation details. |
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.
private AugmentedMessage augmentedMessage; // Wrapper for the augmentation details. | |
private List<Content> retrievedContents; |
WDYT?
951bc77
to
7cfac82
Compare
7cfac82
to
2d16e44
Compare
Hi @KaisNeffati, do you mind if I will do and push some changes? |
Feel free to do it @langchain4j |
Context
#660
Change
Checklist
Before submitting this PR, please check the following points:
Checklist for adding new embedding store integration