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

Allow method parameter override for training based indices (solves issue #2246) #2290

Conversation

buddharajusahil
Copy link
Contributor

@buddharajusahil buddharajusahil commented Nov 26, 2024

Description

#2246

From mode/disk-based feature, we introduced concept of mode and compression level that will dynamically set default resolution. For indices created directly from the method (i.e. mapping) we allow users to override parameters if they want.

However, for training, we added the mode and compression, but made them mutually exclusive with method. See: https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/plugin/rest/RestTrainModelHandler.java#L138-L139. We need to remove this and allow method overriding to provide a consistent ux experience across the plugin.

Related Issues

Resolves #2246

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • [N/A] API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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: Sahil Buddharaju <[email protected]>
@buddharajusahil buddharajusahil force-pushed the method_parameter_override branch from 040e7d1 to b192e8a Compare November 26, 2024 17:54
@navneet1v
Copy link
Collaborator

@owenhalpert can you please fix the changelog.

@heemin32
Copy link
Collaborator

Do we know why this was previously blocked, considering it only involves removing two lines of code?

Signed-off-by: Sahil Buddharaju <[email protected]>
@buddharajusahil
Copy link
Contributor Author

buddharajusahil commented Nov 26, 2024

Do we know why this was previously blocked, considering it only involves removing two lines of code?

Hi @heemin32,

So Jack said that these checks were added in this pr #2034 and not removed before the release. Then it got removed in this pr #2083 but training was missed

@jmazanec15
Copy link
Member

@buddharajusahil Can you run ./gradlew :spotlessApply to fix style?

Signed-off-by: Sahil Buddharaju <[email protected]>
Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

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

LGTM - we just need to fix CI

Copy link
Member

@jmazanec15 jmazanec15 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

@naveentatikonda naveentatikonda merged commit 19f045d into opensearch-project:main Dec 10, 2024
35 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2290-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 19f045d763cd7f302c2b96c8731b725484b14bec
# Push it to GitHub
git push --set-upstream origin backport/backport-2290-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2290-to-2.x.

@naveentatikonda
Copy link
Member

@buddharajusahil can you fix the conflicts and create a backport PR to 2.x ?

buddharajusahil added a commit to buddharajusahil/k-NN that referenced this pull request Dec 10, 2024
…sue opensearch-project#2246) (opensearch-project#2290)

* Allow method parameter override for training based indices

Signed-off-by: Sahil Buddharaju <[email protected]>

* Fixed code squashing imports

Signed-off-by: Sahil Buddharaju <[email protected]>

* Changed changelog

Signed-off-by: Sahil Buddharaju <[email protected]>

* spotlessApply styling

Signed-off-by: Sahil Buddharaju <[email protected]>

---------

Signed-off-by: Sahil Buddharaju <[email protected]>
Co-authored-by: Sahil Buddharaju <[email protected]>
(cherry picked from commit 19f045d)
buddharajusahil added a commit to buddharajusahil/k-NN that referenced this pull request Dec 11, 2024
…sue opensearch-project#2246) (opensearch-project#2290)

* Allow method parameter override for training based indices

Signed-off-by: Sahil Buddharaju <[email protected]>

* Fixed code squashing imports

Signed-off-by: Sahil Buddharaju <[email protected]>

* Changed changelog

Signed-off-by: Sahil Buddharaju <[email protected]>

* spotlessApply styling

Signed-off-by: Sahil Buddharaju <[email protected]>

---------

Signed-off-by: Sahil Buddharaju <[email protected]>
Co-authored-by: Sahil Buddharaju <[email protected]>
(cherry picked from commit 19f045d)
navneet1v pushed a commit that referenced this pull request Dec 11, 2024
…ices (#2317)

* Allow method parameter override for training based indices (solves issue #2246) (#2290)

* Allow method parameter override for training based indices

Signed-off-by: Sahil Buddharaju <[email protected]>

* Fixed code squashing imports

Signed-off-by: Sahil Buddharaju <[email protected]>

* Changed changelog

Signed-off-by: Sahil Buddharaju <[email protected]>

* spotlessApply styling

Signed-off-by: Sahil Buddharaju <[email protected]>

---------

Signed-off-by: Sahil Buddharaju <[email protected]>
Co-authored-by: Sahil Buddharaju <[email protected]>
(cherry picked from commit 19f045d)

* Fixed response map

Signed-off-by: Sahil Buddharaju <[email protected]>

* Used forked personal branch

Signed-off-by: Sahil Buddharaju <[email protected]>

* spotless apply

Signed-off-by: Sahil Buddharaju <[email protected]>

---------

Signed-off-by: Sahil Buddharaju <[email protected]>
Co-authored-by: Sahil Buddharaju <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow method parameter override for training based indices
7 participants