aiplanethub / genai-stack

An End to End GenAI Framework
https://genaistack.aiplanet.com/
Apache License 2.0
126 stars 43 forks source link

Query argument in `retriever.get_chat_history()` #105

Open shreehari-aiplanet opened 11 months ago

shreehari-aiplanet commented 11 months ago

Description

Query arguement in retriever.get_chat_history() seems a bit ambiguous. Hence it can be removed.

dyaramatreya commented 11 months ago

Hi @shreehari-aiplanet Do you mean to remove the query at https://github.com/aiplanethub/genai-stack/blob/f15b5b32aa7471535889d24845658ace98ccf614/genai_stack/retriever/base.py#L42?

or to remove at https://github.com/aiplanethub/genai-stack/blob/f15b5b32aa7471535889d24845658ace98ccf614/genai_stack/retriever/base.py#L38

shreehari-aiplanet commented 11 months ago

@dyaramatreya It needs to be removed wherever it is declared. Retreiver's basically getting the chat history from memory. A mediator between them is responsible for the interaction between memory and retriever to get the chat history. Hence it needs to be removed from more than 2 places.

dyaramatreya commented 11 months ago

@shreehari-aiplanet So basically, we have to delete this retriever.get_chat_history() method definition as well as the places where it is used. Am I correct?

shreehari-aiplanet commented 11 months ago

No, get_chat_history() method should exist and the query parameter that is being passed to it should be removed.

dyaramatreya commented 11 months ago

Ok @shreehari-aiplanet. Can I try working on this? Can you assign this issue to me? I'll make the changes and raise the PR

dyaramatreya commented 11 months ago

But then the query parameter passed at https://github.com/aiplanethub/genai-stack/blob/f15b5b32aa7471535889d24845658ace98ccf614/genai_stack/stack/mediator.py#L72 must also be removed right?

shreehari-aiplanet commented 11 months ago

But then the query parameter passed at

https://github.com/aiplanethub/genai-stack/blob/f15b5b32aa7471535889d24845658ace98ccf614/genai_stack/stack/mediator.py#L72

must also be removed right?

yes

dyaramatreya commented 11 months ago

Thanks. Can you please assing this issue to me? I'll raise the PR in few hours

dyaramatreya commented 11 months ago

@shreehari-aiplanet Please have a look at the PR raised above and suggest if it's good or any changes are needed. Thanks