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.

Sign up for GitHub

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

Jump to bottom

Adding release configs for lucene filtering #663

Merged
merged 2 commits into from
Dec 14, 2022
Merged

Adding release configs for lucene filtering #663

merged 2 commits into from
Dec 14, 2022

Conversation

martin-gaievski
Copy link
Member

Signed-off-by: Martin Gaievski [email protected]

Description

Adding release configs for Lucene filtering, it can be used to run perf tests for releases.

Check List

  • Commits are signed as 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.

@martin-gaievski martin-gaievski added Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. backport 2.x labels Dec 10, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2022 •
edited
Loading

Codecov Report

Merging #663 (cc29b84) into main (79ae6c2) will decrease coverage by 0.32%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main     #663      +/-   ##
============================================
- Coverage     84.80%   84.47%   -0.33%     
+ Complexity     1055     1050       -5     
============================================
  Files           149      149              
  Lines          4291     4291              
  Branches        379      379              
============================================
- Hits           3639     3625      -14     
- Misses          477      489      +12     
- Partials        175      177       +2
Impacted Files Coverage Δ
...va/org/opensearch/knn/index/KNNCircuitBreaker.java 60.00% <0.00%> (-20.00%) ⬇️
...org/opensearch/knn/training/TrainingJobRunner.java 49.25% <0.00%> (-4.48%) ⬇️
...ain/java/org/opensearch/knn/index/KNNSettings.java 80.88% <0.00%> (-2.21%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@martin-gaievski
Copy link
Member Author

Currently blocked, this PR should unblock k-NN main #661

@jmazanec15
Copy link
Member

@martin-gaievski Can you rebase and try again? It should be fixed.

@martin-gaievski martin-gaievski force-pushed the release-config-for-lucene-filtering-perf-test branch from cc29b84 to f0669ca Compare December 13, 2022 17:21
@martin-gaievski martin-gaievski added the v2.5.0 'Issues and PRs related to version v2.5.0' label Dec 13, 2022
@martin-gaievski martin-gaievski marked this pull request as ready for review December 13, 2022 17:59
@martin-gaievski martin-gaievski requested a review from a team December 13, 2022 17:59
jmazanec15
jmazanec15 previously approved these changes Dec 13, 2022
View reviewed changes
heemin32
heemin32 reviewed Dec 13, 2022
View reviewed changes
Copy link
Collaborator

@heemin32 heemin32 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could you rename existing test file as well? We have index.json and text.yml in lucene-hnsw folder. Something like index-spec.json and default-test.yml?

heemin32
heemin32 reviewed Dec 13, 2022
View reviewed changes
benchmarks/perf-tool/release-configs/lucene-hnsw/relaxed-filter-test.yml Outdated
index_name: target_index
- name: create_index
index_name: target_index
index_spec: /home/ec2-user/[PATH]/index.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
index_spec: /home/ec2-user/[PATH]/index.json
index_spec: [INDEX_SPEC_PATH]/index.json

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think /home/ec2-user shouldn't be specified here.

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

benchmarks/perf-tool/release-configs/lucene-hnsw/relaxed-filter-test.yml Outdated
field_name: target_field
bulk_size: 500
dataset_format: hdf5
dataset_path: /home/ec2-user/data/sift-128-euclidean-with-attr.hdf5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
dataset_path: /home/ec2-user/data/sift-128-euclidean-with-attr.hdf5
dataset_path: [DATASET_PATH]/sift-128-euclidean-with-attr.hdf5

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

benchmarks/perf-tool/release-configs/lucene-hnsw/relaxed-filter-test.yml Outdated
index_name: target_index
field_name: target_field
dataset_format: hdf5
dataset_path: /home/ec2-user/data/sift-128-euclidean-with-attr.hdf5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
dataset_path: /home/ec2-user/data/sift-128-euclidean-with-attr.hdf5
dataset_path: [DATASET_PATH]/sift-128-euclidean-with-attr.hdf5

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

benchmarks/perf-tool/release-configs/lucene-hnsw/relaxed-filter-test.yml Outdated
dataset_format: hdf5
dataset_path: /home/ec2-user/data/sift-128-euclidean-with-attr.hdf5
neighbors_format: hdf5
neighbors_path: /home/ec2-user/data/sift-128-euclidean-with-filters.hdf5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
neighbors_path: /home/ec2-user/data/sift-128-euclidean-with-filters.hdf5
neighbors_path: [DATASET_PATH]/sift-128-euclidean-with-filters.hdf5

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

benchmarks/perf-tool/release-configs/lucene-hnsw/relaxed-filter-test.yml Outdated
neighbors_format: hdf5
neighbors_path: /home/ec2-user/data/sift-128-euclidean-with-filters.hdf5
neighbors_dataset: neighbors_filter_5
filter_spec: /home/ec2-user/[PATH]/relaxed-filter-spec.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
filter_spec: /home/ec2-user/[PATH]/relaxed-filter-spec.json
filter_spec: [FILTER_SPEC_PATH]/relaxed-filter-spec.json

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

@martin-gaievski
Copy link
Member Author

Could you rename existing test file as well? We have index.json and text.yml in lucene-hnsw folder. Something like index-spec.json and default-test.yml?

I think it's better to create new nested folder(s) for filtering. Renaming existing test config file may cause confusion/errors as its not consistent with tests for other engines.

@heemin32
Copy link
Collaborator

heemin32 commented Dec 13, 2022 •
edited
Loading

Could you rename existing test file as well? We have index.json and text.yml in lucene-hnsw folder. Something like index-spec.json and default-test.yml?

I think it's better to create new nested folder(s) for filtering. Renaming existing test config file may cause confusion/errors as its not consistent with tests for other engines.

We can change the name in other test cases as well. The reason that I think renaming might be better is because index-spec.json file is used for filtering test as well. Or having a separate index-spec.json for each test is another option.

heemin32
heemin32 approved these changes Dec 14, 2022
View reviewed changes
Copy link
Collaborator

@heemin32 heemin32 left a comment

Choose a reason for hiding this comment

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

LGTM

@martin-gaievski martin-gaievski merged commit 99ade43 into opensearch-project:main Dec 14, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 14, 2022
@martin-gaievski @github-actions
Adding release configs for lucene filtering (#663)
3992908 
* Adding release configs for lucene filtering

Signed-off-by: Martin Gaievski <[email protected]>
(cherry picked from commit 99ade43)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Dec 14, 2022
Merged
martin-gaievski added a commit that referenced this pull request Dec 14, 2022
@opensearch-trigger-bot @martin-gaievski
Adding release configs for lucene filtering (#663) (#679)
144a5e8 
* Adding release configs for lucene filtering

Signed-off-by: Martin Gaievski <[email protected]>
(cherry picked from commit 99ade43)

Co-authored-by: Martin Gaievski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmazanec15 jmazanec15 jmazanec15 approved these changes

@heemin32 heemin32 heemin32 approved these changes

Assignees
No one assigned
Labels
backport 2.x Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. v2.5.0 'Issues and PRs related to version v2.5.0'
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martin-gaievski @codecov-commenter @jmazanec15 @heemin32