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

General improvements: extended exposed latency metrics and allowed for filtering benchmark runs by client count. #101

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

Conversation

filipecosta90
Copy link
Contributor

This PR adds small changes that we're already using:

  • Adds a progress detail on the downloaded files
  • Allows to filter by a specific client count on the search benchmarks
  • Adds the overall p50 latency to the results
  • Ensures metadata is kept on the results files

… more metadata. Allowed to filter benchmark runs by client count
Copy link
Member

@KShivendu KShivendu left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Please have a look at my comments :)

benchmark/dataset.py Show resolved Hide resolved
engine/base_client/client.py Outdated Show resolved Hide resolved
Comment on lines 113 to 121
ef = "default"
if "search_params" in search_params:
ef = search_params["search_params"].get("ef", "default")
client_count = search_params.get("parallel", 1)
filter_client_count = len(parallels) > 0
if filter_client_count and (client_count not in parallels):
print(f"\tSkipping ef runtime: {ef}; #clients {client_count}")
continue
print(f"\tRunning ef runtime: {ef}; #clients {client_count}")
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain what are you trying to do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is to allow running specific client configs. The rest is just extra logging.

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.

2 participants