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

Support source filtering for model search #162

Merged
merged 2 commits into from
Oct 30, 2021

Conversation

VijayanB
Copy link
Member

@VijayanB VijayanB commented Oct 28, 2021

Description

Update Model Search API response to Model Index document search response.

Issues Resolved

#160

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • 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.

Added test case to make sure output from get model can be
reduced by adding filter_path parameter.

Signed-off-by: Vijayan Balasubramanian <[email protected]>
@VijayanB VijayanB force-pushed the source-filtering branch 2 times, most recently from cbf2087 to 9224d68 Compare October 28, 2021 21:29
@VijayanB VijayanB requested a review from jmazanec15 October 28, 2021 21:44
@VijayanB VijayanB changed the title Source filtering Support source filtering for model search Oct 28, 2021
@VijayanB VijayanB requested a review from vamshin October 28, 2021 21:53
@VijayanB VijayanB self-assigned this Oct 28, 2021
@@ -395,7 +395,10 @@ public void search(SearchRequest request, ActionListener<SearchResponse> actionL
request.indices(MODEL_INDEX_NAME);
client.search(request,ActionListener.wrap(response -> {
for (SearchHit hit : response.getHits()) {
ToXContentObject xContentObject = Model.getModelFromSourceMap(hit.getSourceAsMap(), hit.getId());
if(hit.getSourceAsMap() == null){
Copy link
Member

Choose a reason for hiding this comment

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

Im not convinced we need to do anything special here. Why can't we just return the search response directly to the listener? I know we need to add the model_id, but I think it would be better to just add that to the mapping in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed processing. Now search response will have field called "model_id". This will be updated once model id is added to the mapping parameter.

@VijayanB VijayanB force-pushed the source-filtering branch 4 times, most recently from 9714763 to aa13e10 Compare October 29, 2021 01:40
@VijayanB VijayanB requested a review from jmazanec15 October 29, 2021 01:41
@VijayanB VijayanB added the Bug Fixes Changes to a system or product designed to handle a programming bug/glitch label Oct 29, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2021

Codecov Report

Merging #162 (7ae6668) into main (769a137) will decrease coverage by 0.15%.
The diff coverage is 75.86%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #162      +/-   ##
============================================
- Coverage     82.82%   82.66%   -0.16%     
- Complexity      801      803       +2     
============================================
  Files           117      117              
  Lines          3580     3582       +2     
  Branches        339      338       -1     
============================================
- Hits           2965     2961       -4     
- Misses          461      466       +5     
- Partials        154      155       +1     
Impacted Files Coverage Δ
...rc/main/java/org/opensearch/knn/indices/Model.java 77.77% <68.75%> (-6.84%) ⬇️
...java/org/opensearch/knn/indices/ModelMetadata.java 94.62% <83.33%> (-1.72%) ⬇️
...main/java/org/opensearch/knn/indices/ModelDao.java 81.25% <100.00%> (-0.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 769a137...7ae6668. Read the comment docs.

Copy link
Member

@vamshin vamshin left a comment

Choose a reason for hiding this comment

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

Minor comments. Otherwise looks good.

actionListener.onResponse(response);

}, actionListener::onFailure));
client.search(request,actionListener);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: space after ,

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

@@ -105,6 +108,44 @@ public void testGetModelExists() throws IOException {
assertEquals(testModelMetadata.getTimestamp(), responseMap.get(MODEL_TIMESTAMP));
}



Copy link
Member

Choose a reason for hiding this comment

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

Extra line

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

Return model index search response as output for
model search api.

Signed-off-by: Vijayan Balasubramanian <[email protected]>
@VijayanB VijayanB requested a review from vamshin October 29, 2021 02:46
Copy link
Member

@vamshin vamshin left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

Comment on lines +170 to +171
ModelMetadata modelMetadata = ModelMetadata.getMetadataFromSourceMap(sourceMap);
byte[] blob = getModelBlobFromResponse(sourceMap);
Copy link
Member

Choose a reason for hiding this comment

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

Want to confirm: what happens if these are null?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will not throw any exception. Field value will be empty.

@jmazanec15 jmazanec15 merged commit 549b8ac into opensearch-project:main Oct 30, 2021
martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 7, 2022
martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 7, 2022
Signed-off-by: Vijayan Balasubramanian <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fixes Changes to a system or product designed to handle a programming bug/glitch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants