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

Add multi modal default preprocess function #2500

Merged
merged 15 commits into from
Jul 17, 2024
Merged

Add multi modal default preprocess function #2500

merged 15 commits into from
Jul 17, 2024

Conversation

zane-neo
Copy link
Collaborator

@zane-neo zane-neo commented Jun 5, 2024

Description

Add multi modal default preprocess function

Issues Resolved

#2364

Check List

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

@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 5, 2024 01:13 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 5, 2024 01:13 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 5, 2024 01:13 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 5, 2024 01:14 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 5, 2024 01:14 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 5, 2024 01:14 — with GitHub Actions Inactive
@ylwu-amzn
Copy link
Collaborator

Add IT to cover this ?

@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 5, 2024 02:08 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 5, 2024 02:08 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 5, 2024 02:08 — with GitHub Actions Inactive
martin-gaievski
martin-gaievski reviewed Jun 5, 2024
View reviewed changes
...ava/org/opensearch/ml/common/connector/functions/preprocess/ConnectorPreProcessFunction.java Outdated
@@ -46,6 +47,10 @@ public void validateTextDocsInput(MLInput mlInput) {
if (!(mlInput.getInputDataset() instanceof TextDocsInputDataSet)) {
throw new IllegalArgumentException("This pre_process_function can only support TextDocsInputDataSet");
Copy link
Member

Choose a reason for hiding this comment

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

can you make error message more meaningful to the user, if possible please add details of how/what they need to change to fix it.
in addition to that we can log more details at warn or info level, like what's the actual class of input dataset object

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It make sense we log details of input dataset type, but for changing the error message giving an TextDocsInputDataSet example doesn't seem make sense because the format of the error message will be odd. I prefer we give a link to the source code of TextDocsInputDataSet and user can learn the data structure in the code. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any tutorial / website link we can refer to instead of source code link?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have a good document describing this, I think it's fine to add the key data structure of this request to the error message. Will make the change soon.

...ava/org/opensearch/ml/common/connector/functions/preprocess/ConnectorPreProcessFunction.java Outdated
@@ -46,6 +47,10 @@ public void validateTextDocsInput(MLInput mlInput) {
if (!(mlInput.getInputDataset() instanceof TextDocsInputDataSet)) {
throw new IllegalArgumentException("This pre_process_function can only support TextDocsInputDataSet");
Copy link
Member

Choose a reason for hiding this comment

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

If you have context can you please add class and method level comments, somehow that is missing for this class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

...ensearch/ml/common/connector/functions/preprocess/MultiModalEmbeddingPreProcessFunction.java Outdated

import static org.opensearch.ml.common.utils.StringUtils.convertScriptStringToJsonString;

public class MultiModalEmbeddingPreProcessFunction extends ConnectorPreProcessFunction {
Copy link
Member

Choose a reason for hiding this comment

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

please add class level comments

Copy link
Member

Choose a reason for hiding this comment

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

Also can you think of a better name for this class? We're only work with text and image embeddings in a certain format, plus parent class is about connector while this child class is for embedding processor. Maybe MultiModalModelConnectorPreProcessFunction, or something similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The parent class name connector is a little bit redundant here since the pre process functions are specifically for connector use only. Also it's possible that in the future there'll be a pre process function of multi-modal connector not for embedding, but since the chance is low, I'll change this to the name you suggested.

...ensearch/ml/common/connector/functions/preprocess/MultiModalEmbeddingPreProcessFunction.java Outdated
validateTextDocsInput(mlInput);
}

// The input will must have inputText even it's null, input image is optional.
Copy link
Member

Choose a reason for hiding this comment

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

please correct wording for comment and format method level comment as per java conventions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

...ensearch/ml/common/connector/functions/preprocess/MultiModalEmbeddingPreProcessFunction.java Outdated Show resolved Hide resolved
...ensearch/ml/common/connector/functions/preprocess/MultiModalEmbeddingPreProcessFunction.java Outdated
TextDocsInputDataSet inputData = (TextDocsInputDataSet) mlInput.getInputDataset();
if (inputData.getDocs().size() == 1) {
return RemoteInferenceInputDataSet.builder().parameters(convertScriptStringToJsonString(Map.of("parameters", Map.of("inputText", inputData.getDocs().get(0))))).build();
} else {
Copy link
Member

Choose a reason for hiding this comment

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

we need to check if docs collection is not empty

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From neural search code: https://github.com/opensearch-project/neural-search/blob/main/src/main/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessor.java#L287, the collection will never be empty. But to make code more adapt to change, will add check for empty docs.

...ensearch/ml/common/connector/functions/preprocess/MultiModalEmbeddingPreProcessFunction.java Outdated Show resolved Hide resolved
...arch/ml/common/connector/functions/preprocess/MultiModalEmbeddingPreProcessFunctionTest.java Outdated

import static org.junit.Assert.assertEquals;

public class MultiModalEmbeddingPreProcessFunctionTest {
Copy link
Member

Choose a reason for hiding this comment

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

for this repo all unit tests are derived from OpenSearchTestCase or one of it children, any reason why it's different here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not all tests in this repo derived from OpenSearchTestCase, for tests in common folder, they basically not derived from it.

...arch/ml/common/connector/functions/preprocess/MultiModalEmbeddingPreProcessFunctionTest.java Outdated
}

@Test
public void process_NullInput() {
Copy link
Member

Choose a reason for hiding this comment

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

please use camelcase for method name, also please use following format when naming method: "itemBeingTested_scenario_expectedOutcome".

Copy link
Member

Choose a reason for hiding this comment

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

same comment for all methods in this test class

@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env June 6, 2024 07:44 — with GitHub Actions Error
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env June 6, 2024 07:44 — with GitHub Actions Error
@zane-neo zane-neo had a problem deploying to ml-commons-cicd-env June 6, 2024 07:44 — with GitHub Actions Failure
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 6, 2024 07:44 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 6, 2024 07:44 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 6, 2024 07:44 — with GitHub Actions Inactive
@zane-neo zane-neo force-pushed the multi-modal-preprocess-func branch from bd1722d to aee2a0b Compare June 6, 2024 07:57
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env June 6, 2024 07:57 — with GitHub Actions Inactive
zane-neo and others added 14 commits July 12, 2024 08:21
@zane-neo
Address comments
12dece7 
Signed-off-by: zane-neo <[email protected]>
@zane-neo
address comments
28a6ad2 
Signed-off-by: zane-neo <[email protected]>
@zane-neo
add IT
da814af 
Signed-off-by: zane-neo <[email protected]>
@zane-neo
Fix IT
6b30896 
Signed-off-by: zane-neo <[email protected]>
@zane-neo @ylwu-amzn
Update common/src/main/java/org/opensearch/ml/common/connector/functi…
c7472f7 
…ons/preprocess/MultiModalConnectorPreProcessFunction.java

Co-authored-by: Yaliang Wu <[email protected]>
Signed-off-by: zane-neo <[email protected]>
@ylwu-amzn @zane-neo
fix test
01b9caf 
Signed-off-by: Yaliang Wu <[email protected]>
@zane-neo
Add more ITs
919183d 
Signed-off-by: zane-neo <[email protected]>
@zane-neo
Fix failure ITs
fe22836 
Signed-off-by: zane-neo <[email protected]>
@zane-neo
fix failure IT
10c18bf 
Signed-off-by: zane-neo <[email protected]>
@zane-neo
Fix failure ITs
43d13fb 
Signed-off-by: zane-neo <[email protected]>
@zane-neo
format code
8eacfec 
Signed-off-by: zane-neo <[email protected]>
@zane-neo
Add error response to make it esay to figure out the failure root cause
62a45c3 
Signed-off-by: zane-neo <[email protected]>
@zane-neo
format code
f0054fd 
Signed-off-by: zane-neo <[email protected]>
@zane-neo
rebase main
31fb324 
Signed-off-by: zane-neo <[email protected]>
@zane-neo zane-neo force-pushed the multi-modal-preprocess-func branch from d6277ce to 31fb324 Compare July 12, 2024 00:21
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env July 12, 2024 00:21 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env July 12, 2024 00:21 — with GitHub Actions Inactive
@zane-neo zane-neo temporarily deployed to ml-commons-cicd-env July 12, 2024 01:22 — with GitHub Actions Inactive
@zane-neo zane-neo merged commit 0e89c17 into opensearch-project:main Jul 17, 2024
6 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 17, 2024
@zane-neo @github-actions
Add multi modal default preprocess function (#2500)
1c795fc 
* Add multi modal default preprocess function

Signed-off-by: zane-neo <[email protected]>

* Address comments

Signed-off-by: zane-neo <[email protected]>

* address comments

Signed-off-by: zane-neo <[email protected]>

* add IT

Signed-off-by: zane-neo <[email protected]>

* Fix IT

Signed-off-by: zane-neo <[email protected]>

* Update common/src/main/java/org/opensearch/ml/common/connector/functions/preprocess/MultiModalConnectorPreProcessFunction.java

Co-authored-by: Yaliang Wu <[email protected]>
Signed-off-by: zane-neo <[email protected]>

* fix test

Signed-off-by: Yaliang Wu <[email protected]>

* Add more ITs

Signed-off-by: zane-neo <[email protected]>

* Fix failure ITs

Signed-off-by: zane-neo <[email protected]>

* fix failure IT

Signed-off-by: zane-neo <[email protected]>

* Fix failure ITs

Signed-off-by: zane-neo <[email protected]>

* format code

Signed-off-by: zane-neo <[email protected]>

* Add error response to make it esay to figure out the failure root cause

Signed-off-by: zane-neo <[email protected]>

* format code

Signed-off-by: zane-neo <[email protected]>

* rebase main

Signed-off-by: zane-neo <[email protected]>

---------

Signed-off-by: zane-neo <[email protected]>
Signed-off-by: Yaliang Wu <[email protected]>
Co-authored-by: Yaliang Wu <[email protected]>
(cherry picked from commit 0e89c17)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jul 17, 2024
Merged
zane-neo added a commit that referenced this pull request Jul 18, 2024
@opensearch-trigger-bot @zane-neo
Add multi modal default preprocess function (#2500) (#2662)
7eadf6d 
* Add multi modal default preprocess function

Signed-off-by: zane-neo <[email protected]>

* Address comments

Signed-off-by: zane-neo <[email protected]>

* address comments

Signed-off-by: zane-neo <[email protected]>

* add IT

Signed-off-by: zane-neo <[email protected]>

* Fix IT

Signed-off-by: zane-neo <[email protected]>

* Update common/src/main/java/org/opensearch/ml/common/connector/functions/preprocess/MultiModalConnectorPreProcessFunction.java

Co-authored-by: Yaliang Wu <[email protected]>
Signed-off-by: zane-neo <[email protected]>

* fix test

Signed-off-by: Yaliang Wu <[email protected]>

* Add more ITs

Signed-off-by: zane-neo <[email protected]>

* Fix failure ITs

Signed-off-by: zane-neo <[email protected]>

* fix failure IT

Signed-off-by: zane-neo <[email protected]>

* Fix failure ITs

Signed-off-by: zane-neo <[email protected]>

* format code

Signed-off-by: zane-neo <[email protected]>

* Add error response to make it esay to figure out the failure root cause

Signed-off-by: zane-neo <[email protected]>

* format code

Signed-off-by: zane-neo <[email protected]>

* rebase main

Signed-off-by: zane-neo <[email protected]>

---------

Signed-off-by: zane-neo <[email protected]>
Signed-off-by: Yaliang Wu <[email protected]>
Co-authored-by: Yaliang Wu <[email protected]>
(cherry picked from commit 0e89c17)

Co-authored-by: zane-neo <[email protected]>
@b4sjoo b4sjoo added the v2.16.0 Issues targeting release v2.16.0 label Jul 26, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 19, 2024
@zane-neo @github-actions
Add multi modal default preprocess function (#2500)
fc72f70 
* Add multi modal default preprocess function

Signed-off-by: zane-neo <[email protected]>

* Address comments

Signed-off-by: zane-neo <[email protected]>

* address comments

Signed-off-by: zane-neo <[email protected]>

* add IT

Signed-off-by: zane-neo <[email protected]>

* Fix IT

Signed-off-by: zane-neo <[email protected]>

* Update common/src/main/java/org/opensearch/ml/common/connector/functions/preprocess/MultiModalConnectorPreProcessFunction.java

Co-authored-by: Yaliang Wu <[email protected]>
Signed-off-by: zane-neo <[email protected]>

* fix test

Signed-off-by: Yaliang Wu <[email protected]>

* Add more ITs

Signed-off-by: zane-neo <[email protected]>

* Fix failure ITs

Signed-off-by: zane-neo <[email protected]>

* fix failure IT

Signed-off-by: zane-neo <[email protected]>

* Fix failure ITs

Signed-off-by: zane-neo <[email protected]>

* format code

Signed-off-by: zane-neo <[email protected]>

* Add error response to make it esay to figure out the failure root cause

Signed-off-by: zane-neo <[email protected]>

* format code

Signed-off-by: zane-neo <[email protected]>

* rebase main

Signed-off-by: zane-neo <[email protected]>

---------

Signed-off-by: zane-neo <[email protected]>
Signed-off-by: Yaliang Wu <[email protected]>
Co-authored-by: Yaliang Wu <[email protected]>
(cherry picked from commit 0e89c17)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Nov 19, 2024
Merged
dhrubo-os pushed a commit that referenced this pull request Nov 19, 2024
@opensearch-trigger-bot @zane-neo
Add multi modal default preprocess function (#2500) (#3230)
c6cf378 
* Add multi modal default preprocess function

Signed-off-by: zane-neo <[email protected]>

* Address comments

Signed-off-by: zane-neo <[email protected]>

* address comments

Signed-off-by: zane-neo <[email protected]>

* add IT

Signed-off-by: zane-neo <[email protected]>

* Fix IT

Signed-off-by: zane-neo <[email protected]>

* Update common/src/main/java/org/opensearch/ml/common/connector/functions/preprocess/MultiModalConnectorPreProcessFunction.java

Co-authored-by: Yaliang Wu <[email protected]>
Signed-off-by: zane-neo <[email protected]>

* fix test

Signed-off-by: Yaliang Wu <[email protected]>

* Add more ITs

Signed-off-by: zane-neo <[email protected]>

* Fix failure ITs

Signed-off-by: zane-neo <[email protected]>

* fix failure IT

Signed-off-by: zane-neo <[email protected]>

* Fix failure ITs

Signed-off-by: zane-neo <[email protected]>

* format code

Signed-off-by: zane-neo <[email protected]>

* Add error response to make it esay to figure out the failure root cause

Signed-off-by: zane-neo <[email protected]>

* format code

Signed-off-by: zane-neo <[email protected]>

* rebase main

Signed-off-by: zane-neo <[email protected]>

---------

Signed-off-by: zane-neo <[email protected]>
Signed-off-by: Yaliang Wu <[email protected]>
Co-authored-by: Yaliang Wu <[email protected]>
(cherry picked from commit 0e89c17)

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

@dhrubo-os dhrubo-os dhrubo-os left review comments

@martin-gaievski martin-gaievski martin-gaievski left review comments

@ylwu-amzn ylwu-amzn ylwu-amzn left review comments

@xinyual xinyual xinyual approved these changes

@Zhangxunmt Zhangxunmt Zhangxunmt approved these changes

@jngz-es jngz-es jngz-es left review comments

@b4sjoo b4sjoo Awaiting requested review from b4sjoo b4sjoo is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@rbhavna rbhavna Awaiting requested review from rbhavna rbhavna is a code owner

@austintlee austintlee Awaiting requested review from austintlee austintlee is a code owner

@HenryL27 HenryL27 Awaiting requested review from HenryL27 HenryL27 is a code owner

@sam-herman sam-herman Awaiting requested review from sam-herman

Assignees
No one assigned
Labels
backport feature/multi_tenancy backport 2.x v2.16.0 Issues targeting release v2.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@zane-neo @ylwu-amzn @Zhangxunmt @jngz-es @xinyual @martin-gaievski @dhrubo-os @b4sjoo