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

Add top_k parameter to ChatMessageRetriever #8258

Closed
Emil-io opened this issue Aug 20, 2024 · 9 comments · Fixed by deepset-ai/haystack-experimental#68
Closed

Add top_k parameter to ChatMessageRetriever #8258

Emil-io opened this issue Aug 20, 2024 · 9 comments · Fixed by deepset-ai/haystack-experimental#68
Assignees
Labels
P2 Medium priority, add to the next sprint if no P1 available

Comments

@Emil-io
Copy link

Emil-io commented Aug 20, 2024

Is your feature request related to a problem? Please describe.
The ChatMessageRetriever for RAG + Chat always provides the whole conversation history. That way, at some point the context window is exceeded.

Describe the solution you'd like
Something similar to a top_k parameter in the init of the component. That way, only a specified number of the latest chat messages is retrieved.
This could the also potentially be provided to a summary prompt node, that summarizes multiple ChatMessages.

Describe alternatives you've considered
Custom logic, where the last chat messages are stored separately and are concatenated to the query.

@MetroCat69
Copy link

can I get this issue?

@julian-risch
Copy link
Member

Thanks for the initiative @MetroCat69 Sure, please feel free to create a draft pull request early on so that we can give feedback.

@julian-risch julian-risch added the P2 Medium priority, add to the next sprint if no P1 available label Aug 23, 2024
@julian-risch julian-risch changed the title ChatMessageRetriever top_k Add top_k parameter to ChatMessageRetriever Aug 23, 2024
@vblagoje
Copy link
Member

vblagoje commented Sep 2, 2024

What's the proper name here top_k, last_k or something else cc @TuanaCelik @Emil-io @julian-risch ?

@Emil-io
Copy link
Author

Emil-io commented Sep 2, 2024

What's the proper name here top_k, last_k or something else cc @TuanaCelik @Emil-io @julian-risch ?

Maybe last_k, so that people do not think it also filters down to the best matching text messages? Also it better describes what this parameter is doing, but this is just my opinion.

@vblagoje
Copy link
Member

vblagoje commented Sep 2, 2024

Yeah, good points @Emil-io thank you - I'll update the PR to use last_k as it better clarifies what's going on.

@vblagoje
Copy link
Member

vblagoje commented Sep 2, 2024

Hmm, but we also then deviate from retriever + top_k usage. Hmm, really not sure how to name this one. @TuanaCelik @julian-risch ?

@TuanaCelik
Copy link
Contributor

I think we are making a mistake by calling this a retriever and this conversation made me come to this conclusion.

A retriever has too much connotation for something that will be retrieving based on similarity, embedding, etc. In which case top_k makes sense.
However, my understanding from the ChatMessageRetriever is that it simply fetches all of the messages in the store right? Would we have any use case where this retriever would be used to retrieve 'relevant messages' based on a similarity metirc? In which case, I think having both top_k and last_k may make sense but I'm a bit worried this mixes up concepts too much.

TLDR: I don't think we should use top_k to do something that is not actual 'top k'...

@anakin87 - What's your take on this?

@anakin87
Copy link
Member

anakin87 commented Sep 4, 2024

I'm just commenting on the best name to give to this parameter (not the component name).

I think something like last_k, last_k_messages, or last_n_messages might work...

@vblagoje
Copy link
Member

vblagoje commented Sep 4, 2024

last_k it is, we can add top_k later when these become searching retrievers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Medium priority, add to the next sprint if no P1 available
Projects
None yet
6 participants