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

Fix sort on get trace API and flaky test of conversation #1733

Conversation

Hailong-am
Copy link
Contributor

@Hailong-am Hailong-am commented Dec 5, 2023

Description

  1. Fix sort on get trace Api, in the case two traces with create time are "create_time": "2023-12-05T05:14:42.823172Z" and "create_time": "2023-12-05T05:14:42.823833Z" they are equally same date due to mapping field of create_time is date not date_nanos, either we change type to date_nanos or add one more sort field.

  2. Add refresh policy to update interaction api so that get conversation api will able to fetch latest data.

  3. fix flaky test for conversation

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.

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

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

Comparison is base (fcbfc7e) 68.94% compared to head (82d62c4) 68.92%.

Files Patch % Lines
...g/opensearch/ml/engine/memory/MLMemoryManager.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                        Coverage Diff                        @@
##             feature/agent_framework_dev    #1733      +/-   ##
=================================================================
- Coverage                          68.94%   68.92%   -0.03%     
+ Complexity                          2612     2611       -1     
=================================================================
  Files                                241      241              
  Lines                              12875    12876       +1     
  Branches                            1291     1291              
=================================================================
- Hits                                8877     8875       -2     
- Misses                              3392     3394       +2     
- Partials                             606      607       +1     
Flag Coverage Δ
ml-commons 68.92% <0.00%> (-0.03%) ⬇️

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.

@Hailong-am Hailong-am marked this pull request as draft December 6, 2023 02:56
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 6, 2023 03:20 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 6, 2023 03:20 — with GitHub Actions Inactive
@Hailong-am Hailong-am force-pushed the feature/agent_framework_dev branch from 44b5c9b to e2b6b3d Compare December 6, 2023 03:46
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 6, 2023 03:46 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 6, 2023 03:46 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 6, 2023 03:58 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 6, 2023 03:58 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 6, 2023 03:58 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 6, 2023 03:58 — with GitHub Actions Inactive
@Hailong-am Hailong-am force-pushed the feature/agent_framework_dev branch from eefa37e to 19d9123 Compare December 6, 2023 04:15
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 6, 2023 04:15 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 6, 2023 04:15 — with GitHub Actions Inactive
@Hailong-am Hailong-am marked this pull request as ready for review December 6, 2023 04:16
@Hailong-am Hailong-am changed the title Fix sort on get trace Api Fix sort on get trace API and flaky test of conversation Dec 6, 2023
@Hailong-am
Copy link
Contributor Author

@HenryL27 @Zhangxunmt please help to review

Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
@Hailong-am Hailong-am force-pushed the feature/agent_framework_dev branch from 19d9123 to 82d62c4 Compare December 7, 2023 00:47
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 7, 2023 00:47 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 7, 2023 00:48 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 7, 2023 00:48 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env December 7, 2023 00:48 — with GitHub Actions Inactive
Copy link
Collaborator

@Zhangxunmt Zhangxunmt left a comment

Choose a reason for hiding this comment

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

lgtm. This sorting may need to apply to "get Interactions" as well, but let's not overkill for now.

@Hailong-am
Copy link
Contributor Author

lgtm. This sorting may need to apply to "get Interactions" as well.

@Zhangxunmt there is no other sort key than create_time for interactions, do you mean we update the mappings of create_time filed to date_nanos?

@Zhangxunmt
Copy link
Collaborator

Zhangxunmt commented Dec 8, 2023

@Hailong-am , I mean the problem you described ideally applies to all docs that are sorted based on "date" field type, which includes conversation and interactions. However this race condition may be extremely rare for conversation/interaction because they are human-generated, so what you have changed in using the trace_number should be just enough.
Also we can always pick up another second key to sort, like the "updated_time" in conversation and "input" in interaction.

@ylwu-amzn ylwu-amzn merged commit 8dba50b into opensearch-project:feature/agent_framework_dev Dec 8, 2023
6 of 7 checks passed
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.

3 participants