search

deepset-ai / haystack

AI orchestration framework to build customizable, production-ready LLM applications. Connect components (models, vector DBs, file converters) to pipelines or agents that can interact with your data. With advanced retrieval methods, it's best suited for building RAG, question answering, semantic search or conversational agent chatbots.
https://haystack.deepset.ai
Apache License 2.0
17.72k stars 1.92k forks source link

Add top_k parameter to ChatMessageRetriever #8258

Closed Emil-io closed 2 months ago

Emil-io commented 2 months ago

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 commented 2 months ago

can I get this issue?

julian-risch commented 2 months ago

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

vblagoje commented 2 months ago

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

Emil-io commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

TuanaCelik commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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