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

[Enhancement] Fetch system index mappings from json file instead of string constants #3153

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

pyek-bot
Copy link
Contributor

@pyek-bot pyek-bot commented Oct 23, 2024

Description

Moves the index mappings for system indices into json files and fetches the version from the file itself.
Version should be provided under "_meta"."schema_version"

All mappings were fetched by using the below method in MlIndex.java

public static void main(String[] args) {
        for (MLIndex index : values()) {
            System.out.println(index.getIndexName() + "\n"
                    + index.getMapping() + "\n");
        }
    }

Related Issues

Resolves #2951

Enhancements

  • Add a required_fields object under _meta object to validate if these fields are present in the mapping
  • Support placeholders to enable reusability

These enhancements will be raised in a separate PR: https://github.com/pyek-bot/ml-commons/pull/1/files

Testing

  • Mappings were compared by fetching the existing mappings from main and comparing against new mappings using the above mentioned code block
  • Added validation on mappings to ensure they follow a particular schema
  • Added a test case that initializes the enum so that any failures in mapping creation can be caught before runtime
    https://github.com/pyek-bot/ml-commons/pull/1/files
  • Added UTs for helper methods
  • Ran integration tests to ensure all indices are created
  • Manually created all system indices, fetched them via the GET api and validated the mappings

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • 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.

…ad of string constants

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
@pyek-bot pyek-bot requested a deployment to ml-commons-cicd-env-require-approval October 23, 2024 23:10 — with GitHub Actions Waiting
@pyek-bot pyek-bot requested a deployment to ml-commons-cicd-env-require-approval October 23, 2024 23:10 — with GitHub Actions Waiting
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 23, 2024 23:12 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 23, 2024 23:12 — with GitHub Actions Failure
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 23, 2024 23:17 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 23, 2024 23:17 — with GitHub Actions Failure
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 24, 2024 00:12 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 24, 2024 00:12 — with GitHub Actions Failure
@pyek-bot pyek-bot requested a deployment to ml-commons-cicd-env-require-approval October 24, 2024 00:20 — with GitHub Actions Waiting
@pyek-bot pyek-bot requested a deployment to ml-commons-cicd-env-require-approval October 24, 2024 00:20 — with GitHub Actions Waiting
@pyek-bot pyek-bot force-pushed the fetch_index_mappings_from_files branch from d50f8cf to 63a6d62 Compare October 24, 2024 00:22
@pyek-bot pyek-bot requested a deployment to ml-commons-cicd-env-require-approval October 24, 2024 00:22 — with GitHub Actions Waiting
@pyek-bot pyek-bot requested a deployment to ml-commons-cicd-env-require-approval October 24, 2024 00:22 — with GitHub Actions Waiting
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 24, 2024 17:49 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 24, 2024 17:49 — with GitHub Actions Failure
@pyek-bot pyek-bot marked this pull request as ready for review October 24, 2024 18:44
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 25, 2024 18:46 — with GitHub Actions Failure
Zhangxunmt
Zhangxunmt previously approved these changes Oct 25, 2024
…ums rather than use their own mappings

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
@pyek-bot pyek-bot requested a deployment to ml-commons-cicd-env-require-approval October 29, 2024 22:47 — with GitHub Actions Waiting
@pyek-bot pyek-bot requested a deployment to ml-commons-cicd-env-require-approval October 29, 2024 22:47 — with GitHub Actions Waiting
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 29, 2024 22:51 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 29, 2024 22:51 — with GitHub Actions Failure
@@ -18,15 +18,15 @@
package org.opensearch.ml.common.conversation;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@austintlee @HenryL27 I have also modified this class to use the MLIndex enum, please review the same. I didn't want to do a full refactor for now as it may impact functionality. Added as a todo on top of the class.

@dhrubo-os
Copy link
Collaborator

dhrubo-os commented Oct 31, 2024

Let's add more details about how do you test this feature in the PR description?

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 31, 2024 22:06 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 31, 2024 22:06 — with GitHub Actions Failure
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval November 2, 2024 00:21 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval November 2, 2024 00:21 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval November 4, 2024 20:06 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval November 4, 2024 20:06 — with GitHub Actions Failure
@dhrubo-os
Copy link
Collaborator


IndexUtilsTest > testGetMappingFromFile FAILED
    org.junit.ComparisonFailure at IndexUtilsTest.java:71
    ```

@pyek-bot
Copy link
Contributor Author

pyek-bot commented Nov 4, 2024


IndexUtilsTest > testGetMappingFromFile FAILED
    org.junit.ComparisonFailure at IndexUtilsTest.java:71
    ```

On it

Edit: Fixed - issue was due to eol characters being different on different OS

…cter issue

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval November 5, 2024 00:04 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval November 5, 2024 00:04 — with GitHub Actions Failure
@pyek-bot
Copy link
Contributor Author

pyek-bot commented Nov 8, 2024

@Zhangxunmt @dhrubo-os please re-review when you get the time

I am looking at the test failures, seems to be flaky tests

@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval November 11, 2024 18:39 — with GitHub Actions Failure
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.

[Enhancement] Use index mapping json file
5 participants