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 API specifications for k-NN 2.17 changes #588

Merged
merged 7 commits into from
Sep 24, 2024

Conversation

ryanbogan
Copy link
Member

Description

This PR updates the API specifications to be consistent with changes released for k-NN in OpenSearch 2.17.
These changes include adding rescore and oversampling factor to a k-NN query, and adding mode and compression options during index creation and training models.

Issues Resolved

#583

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.

Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Copy link
Contributor

github-actions bot commented Sep 24, 2024

Changes Analysis

Commit SHA: 6f8c43d
Comparing To SHA: 3d49360

API Changes

Summary

├─┬Paths
│ ├─┬/_plugins/_knn/models/{model_id}/_train
│ │ └─┬POST
│ │   ├─┬Responses
│ │   │ └─┬200
│ │   │   └──[➕] content (27982:11)
│ │   └─┬Requestbody
│ │     └─┬application/json
│ │       └─┬Schema
│ │         ├──[➖] required (25547:17)❌ 
│ │         ├──[➕] properties (25545:15)
│ │         ├──[➕] properties (25543:15)
│ │         └──[➕] properties (25549:15)
│ └─┬/_plugins/_knn/models/_train
│   └─┬POST
│     ├─┬Responses
│     │ └─┬200
│     │   └──[➕] content (27982:11)
│     └─┬Requestbody
│       └─┬application/json
│         └─┬Schema
│           ├──[➖] required (25547:17)❌ 
│           ├──[➕] properties (25545:15)
│           ├──[➕] properties (25543:15)
│           └──[➕] properties (25549:15)
└─┬Components
  ├─┬_common.mapping:KnnVectorPropertyBase
  │ ├──[➕] properties (37749:9)
  │ ├──[➕] properties (37745:9)
  │ ├──[➕] properties (37747:9)
  │ └──[➕] properties (37743:9)
  └─┬_common:KnnField
    ├──[➕] properties (30275:9)
    └──[➕] properties (30280:9)

Document Element Total Changes Breaking Changes
paths 10 2
components 6 0
  • BREAKING Changes: 2 out of 16
  • Removals: 2
  • Additions: 14
  • Breaking Removals: 2

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11020408398/artifacts/1973422498

API Coverage

Before After Δ
Covered (%) 584 (57.2 %) 584 (57.2 %) 0 (0 %)
Uncovered (%) 437 (42.8 %) 437 (42.8 %) 0 (0 %)
Unknown 25 25 0

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looking good.

  1. Adopt the movies/books theme in the examples since we plan to use those in the documentation.
  2. Add 2.17 to CI/CD, iterate to green (npm run lint--fix, etc.).

CHANGELOG.md Outdated Show resolved Hide resolved
tests/default/_core/search/knn_disk_based.yaml Outdated Show resolved Hide resolved
tests/default/_core/search/knn_disk_based.yaml Outdated Show resolved Hide resolved
tests/default/_core/search/knn_disk_based.yaml Outdated Show resolved Hide resolved
…og message

Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
@ryanbogan
Copy link
Member Author

@dblock Do I need to change the sha ref in the workflow file to get 3.0 to pass?

@dblock
Copy link
Member

dblock commented Sep 24, 2024

@dblock Do I need to change the sha ref in the workflow file to get 3.0 to pass?

Yes, because that's main and your additions exist in a newer build.

If you have a moment, since you are updating 3.0, also check whether the build you're picking fixes

and similar checks that were disabled because main didn't have a plugin.

@ryanbogan
Copy link
Member Author

Sure, I'll take a look

.github/workflows/test-spec.yml Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines +659 to +668
method_parameters:
type: object
x-version-added: '2.16'
additionalProperties:
type: number
rescore:
type: object
x-version-added: '2.17'
additionalProperties:
type: number

Choose a reason for hiding this comment

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

if I understand this correctly, the additionalProperties are more of a map whose values will always be of type number? correct?

if the above understanding is correct, then my worry is going forward we can add more parameters and those parameters might not be of number type. Can use some type like object or anything that can allow us to specify number, strings as additional parameter values in future.

@dblock thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we add them in the future, we can update this with:

anyOf:
  type:
    number
    string
    etc.

Copy link
Member

Choose a reason for hiding this comment

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

@navneet1v you're correct, and @ryanbogan is also correct, if the schema today is a number, leave it as such, and tomorrow it can be extended when it can actually be something else.

Choose a reason for hiding this comment

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

Sure make sense. In case we update in future, I am hoping the changes are BWC. can you please confirm that.
@dblock

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

@ryanbogan
Copy link
Member Author

ryanbogan commented Sep 24, 2024

Looks like the asynchronous search tests are still failing on 3.0, will add the version checks again

Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Copy link
Contributor

Spec Test Coverage Analysis

Total Tested
500 303 (60.6 %)

@dblock dblock merged commit 7d05664 into opensearch-project:main Sep 24, 2024
19 checks passed
@ryanbogan ryanbogan deleted the update_knn_specs branch September 24, 2024 19:47
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