ml-commons icon indicating copy to clipboard operation
ml-commons copied to clipboard

Move ConversationalMemoryClient to ml-client

Open austintlee opened this issue 2 years ago • 6 comments

As suggested in #1275, this client, which is currently only used by the RAG pipeline, can move to opensearch-ml-client so that it can be used by other applications.

@jngz-es @dhrubo-os do you guys think this should be merged with MachineLearningClient or are you thinking of a separate client. By the way, MachineLearningClient currently gets built as a shadow jar and it is not meant for internal use. The main thing I want to discuss here is whether or not we should have an external version of this memory client that makes it easy for applications to interact with memory.

Related: #1150

austintlee avatar Sep 05 '23 23:09 austintlee

@minalsha @dbwiddis

austintlee avatar Sep 27 '23 19:09 austintlee

Thanks @austintlee for tagging us to the issue. We really need to get this available to AI-Workflow soon. Can you please share an ETA for this work? Thank You

minalsha avatar Sep 29 '23 18:09 minalsha

@austintlee from my understanding, as we already have rest APIs for Conversational Memory for external use, it is natural to have them in ML Client too.

jngz-es avatar Oct 02 '23 17:10 jngz-es

@jngz-es Given that the Memory API is still behind a feature flag (still an experimental feature), do you think it is OK to put them in the (external) client? Also, if we want to move them into the ml client, do you think we can still make 2.11?

cc: @ylwu-amzn

austintlee avatar Oct 02 '23 17:10 austintlee

^^ What PR #1417 does for this:

  • creates a new MemoryClient object accessed from the MLClient by mlClient.memory()
  • This memory client has the conversation memory APIs - createConversation, createInteraction, getConversations, getInteractions, deleteConversation
  • Use them by passing in the request objects, i.e. memoryClient.createConversation(new CreateConversationRequest()); this introduces a dependency on the opensearch-ml-memory subproject.

Essentially, it's just an extremely thin wrapper around Client.execute().

Questions:

  • should the memory client be its own "sub-client"? Instead we could simply put the APIs in the MLClient directly.
  • is this additional subproject dependency ok? Instead we would move the conversation transport request and response classes to opensearch-ml-common, which maybe we should do anyway.
  • are these the APIs we want to expose in the memory client? As I understand, there's work being done to create more complicated memory structures on top of the basic conversation memory, so we should be able to include those too, right? What do those APIs look like?
  • should the client take the actual string params and stuff and construct the transport requests locally and unpack the responses? Seems clunky, but also resolves the dependency thing

@jngz-es @ylwu-amzn @minalsha Totally happy to make any suggested changes, but let's agree on what exactly we want here

HenryL27 avatar Oct 04 '23 15:10 HenryL27

@HenryL27 , for your questions here are my thoughts.

  1. Let's put the APIs in the MLClient for now. I approved you PR.
  2. I think we can move all the memory requests/response in the ml-common repo, which is consistent with all other transport APIs.
  3. Let's merge the current APIs and add more incrementally. There are more APIs like search, update, and delete on the way.
  4. This may introduce too much work in the ml-common. the opensearch-ml-common should be a fairly simple and small dependency that to be consumed by other apps.

Zhangxunmt avatar Nov 20 '23 20:11 Zhangxunmt