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

rename the conversation/interaction indices to memory meta/message #1678

Conversation

Zhangxunmt
Copy link
Collaborator

Description

Rename the conversation and interaction indices to memory meta/message based on the discussion in #1614

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@austintlee
Copy link
Collaborator

Do we have a name for things we're storing in this new index other than interactions?

@navneet1v I think you suggested the name change. Can you sign off on the new names of the indexes?

@dhrubo-os
Copy link
Collaborator

fix spotless.

@navneet1v
Copy link
Contributor

Do we have a name for things we're storing in this new index other than interactions?

@navneet1v I think you suggested the name change. Can you sign off on the new names of the indexes?

looks good from my side

@austintlee
Copy link
Collaborator

@Zhangxunmt what else are we storing in the 'message' index? Will there be a new resource type called 'message'?

@dhrubo-os
Copy link
Collaborator

Do we have a name for things we're storing in this new index other than interactions?
@navneet1v I think you suggested the name change. Can you sign off on the new names of the indexes?

looks good from my side

We have

Do we have a name for things we're storing in this new index other than interactions?
@navneet1v I think you suggested the name change. Can you sign off on the new names of the indexes?

looks good from my side

now index name: plugins-ml-memory-message

How much does it make sense to have interaction_id as a id field?
Same for plugins-ml-memory-meta, id field is conversation_id :)

I hope in the long run, it doesn't get more confusing.

@Zhangxunmt
Copy link
Collaborator Author

The spotless error is coming from the indexMapping Tool. @dhrubo-os
The conversation_id and interaction_id still make sense because they align with the current name convention in the conversational search https://opensearch.org/docs/latest/ml-commons-plugin/conversational-search/, unless we are going to rename this feature. @dhrubo-os

The change of the index name only reflects that this index is not only for conversation/interaction. It can be reused for other memory related data like a message (not necessary an interaction), or anything that can be stored as history data. We don't have a concrete idea of what the "message" application_type looks like though. Let's assume this "message" scope is larger than "interaction"? @austintlee

@Zhangxunmt Zhangxunmt force-pushed the feature/agent_framework_dev branch from 81e7357 to 3fc5c82 Compare November 22, 2023 23:36
@Zhangxunmt Zhangxunmt temporarily deployed to ml-commons-cicd-env November 22, 2023 23:36 — with GitHub Actions Inactive
@Zhangxunmt Zhangxunmt temporarily deployed to ml-commons-cicd-env November 22, 2023 23:36 — with GitHub Actions Inactive
@Zhangxunmt Zhangxunmt temporarily deployed to ml-commons-cicd-env November 22, 2023 23:36 — with GitHub Actions Inactive
@Zhangxunmt Zhangxunmt temporarily deployed to ml-commons-cicd-env November 22, 2023 23:36 — with GitHub Actions Inactive
Copy link
Collaborator

@austintlee austintlee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine, but if this can wait a few days, I would also get a sign-off from @HenryL27 .

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (fd26f46) 66.47% compared to head (3fc5c82) 66.54%.
Report is 1 commits behind head on feature/agent_framework_dev.

Files Patch % Lines
...g/opensearch/ml/engine/tools/IndexMappingTool.java 80.59% 8 Missing and 5 partials ⚠️
Additional details and impacted files
@@                        Coverage Diff                        @@
##             feature/agent_framework_dev    #1678      +/-   ##
=================================================================
+ Coverage                          66.47%   66.54%   +0.07%     
- Complexity                          2525     2530       +5     
=================================================================
  Files                                232      233       +1     
  Lines                              12562    12629      +67     
  Branches                            1269     1278       +9     
=================================================================
+ Hits                                8350     8404      +54     
- Misses                              3657     3665       +8     
- Partials                             555      560       +5     
Flag Coverage Δ
ml-commons 66.54% <80.59%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Zhangxunmt
Copy link
Collaborator Author

@HenryL27 , do you still have concerns for this name change?

Copy link
Collaborator

@HenryL27 HenryL27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is acceptable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants