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

update hippo client #283

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

update hippo client #283

wants to merge 3 commits into from

Conversation

ybtsdst
Copy link

@ybtsdst ybtsdst commented Mar 1, 2024

  1. update hippo client
  • enable hippo client
  • add more params
  • enable scalar index for filter search cases
  1. some small improvements
  • enable avg latency

@sre-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ybtsdst
To complete the pull request process, please assign xuanyang-cn after the PR has been reviewed.
You can assign the PR to them by writing /assign @xuanyang-cn in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@alwayslove2013 alwayslove2013 self-requested a review March 1, 2024 06:56

[*.json]
indent_style = space
indent_size = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

each file should end with a newline, same for other files.

@@ -0,0 +1,93 @@
from transwarp_hippo_api.hippo_client import HippoClient, HippoField
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this file be included in this PR? Seems it's just for debugging purpose, we should keep hippo client same as others.

@@ -161,7 +161,7 @@ def _load_train_data(self):
finally:
runner = None

def _serial_search(self) -> tuple[float, float]:
def _serial_search(self) -> tuple[float, float, float]:
"""Performance serial tests, search the entire test data once,
calculate the recall, serial_latency_p99
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also change the docstring? TBH I think the avg latency should be in a seperate PR ;)

Copy link
Collaborator

@XuanYang-cn XuanYang-cn left a comment

Choose a reason for hiding this comment

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

@ybtsdst Thanks for contribution!

Hippo clients part LGTM, but this PR carries many unnececcery changes as @zhjwpku suggests. I'll merge this PR if this PR contians changes of hippo client only. Which means the follow change requests:

  • remove all the changes about editor and vscode, except .gitignore
  • remove the test.py in dir hippo/
  • remove the new added avg_latency metric, better in a different PR

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