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

Fixes #1137 - Adding query_image to Neural query #1138

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

urinud
Copy link
Contributor

@urinud urinud commented Aug 13, 2024

Description

Fixes missing field for query_image in Neural Query. See Neural query.

Issues Resolved

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: uri.nudelman <uriel.nudelman@offerup.com>
@dblock
Copy link
Member

dblock commented Aug 13, 2024

Thanks. If you can, please help make sure we don't break it via code generation. I see query_image in https://github.com/opensearch-project/opensearch-api-specification/blob/09824d0d0913a27ed9deeedf71fa4fffb1bed8ff/spec/schemas/_common.query_dsl.yaml#L1238, but I don't see any tests for it. Maybe you can add one into https://github.com/opensearch-project/opensearch-api-specification/tree/main/tests/default?

Signed-off-by: uri.nudelman <uriel.nudelman@offerup.com>
@urinud
Copy link
Contributor Author

urinud commented Aug 13, 2024

Thanks. If you can, please help make sure we don't break it via code generation. I see query_image in https://github.com/opensearch-project/opensearch-api-specification/blob/09824d0d0913a27ed9deeedf71fa4fffb1bed8ff/spec/schemas/_common.query_dsl.yaml#L1238, but I don't see any tests for it. Maybe you can add one into https://github.com/opensearch-project/opensearch-api-specification/tree/main/tests/default?

@dblock , I manually added the changes into NeuralQuery in this project. That link that you send me is for another project. How are these projects related ?

Signed-off-by: uri.nudelman <uriel.nudelman@offerup.com>
@dblock
Copy link
Member

dblock commented Aug 14, 2024

How are these projects related ?

@Xtansia is working on replacing all of these hand-rolled classes with a code generator that uses the spec in https://github.com/opensearch-project/opensearch-api-specification, beginning with #366.

@urinud
Copy link
Contributor Author

urinud commented Aug 14, 2024

Hello @Xtansia & @dblock , I reviewed the existing schema definition and executed the generator.
Before downloading the latest schema the generator executed, but not clases for querying where generated.
After downloading the latest version the generator thrown this error:

[CodeGenerator] FATAL - Unexpected Error
java.lang.IllegalArgumentException: Unknown status code: 204
        at org.opensearch.client.codegen.openapi.HttpStatusCode.from(HttpStatusCode.java:40) ~[main/:?]
        at org.opensearch.client.codegen.openapi.OpenApiElement.lambda$children$2(OpenApiElement.java:115) ~[main/:?]
        at org.opensearch.client.codegen.utils.Maps$EntryMapper.map(Maps.java:68) ~[main/:?]

I tried merging into the original file the Neural definition but then failed becase of the format binary not recognized:

[CodeGenerator] FATAL - Unexpected Error
java.lang.IllegalArgumentException: Unknown format: binary
        at org.opensearch.client.codegen.openapi.OpenApiSchemaFormat.from(OpenApiSchemaFormat.java:28) ~[main/:?]
        at org.opensearch.client.codegen.utils.Functional.ifNonnull(Functional.java:24) ~[main/:?]
        at org.opensearch.client.codegen.openapi.OpenApiSchema.<init>(OpenApiSchema.java:107) ~[main/:?]
        at org.opensearch.client.codegen.openapi.OpenApiElement.lambda$children$3(OpenApiElement.java:116) ~[main/:?]
        at org.opensearch.client.codegen.utils.Maps$EntryMapper.map(Maps.java:68) ~[main/:?]

Since the query_image is a String with a image in Base64 encoded format, seems safe to remove the format.
With that change the generation executed but still have not generated any query related object to check.
Thus I do not see compatibility issues with the schema definition and my change (except that I added a validation to check that at least one of the fields text or image are completed).
Not sure what else, I should check move forward.
Thank you

@dblock
Copy link
Member

dblock commented Aug 14, 2024

@urinud Thanks for looking into this. I didn't know that query_image is in a base64 format so that means the spec is incorrect. Make a fix into the spec and add a test?

@urinud
Copy link
Contributor Author

urinud commented Aug 14, 2024

@dblock , I created this PR in the API specs project.
Hope I do now know how to create a test in that project. If you can give me detailed instructions I can try to help.
Regarding this change for the Java Client what else need to be done to move forward ?
Thank you

@dblock
Copy link
Member

dblock commented Aug 14, 2024

@dblock , I created this PR in the API specs project. Hope I do now know how to create a test in that project. If you can give me detailed instructions I can try to help.

Certainly. We have a whole guide in that repo! https://github.com/opensearch-project/opensearch-api-specification/blob/main/TESTING_GUIDE.md.

Regarding this change for the Java Client what else need to be done to move forward ? Thank you

It LGTM, leaving it to @Xtansia to review/merge.

@urinud
Copy link
Contributor Author

urinud commented Aug 15, 2024

@Xtansia , are we good with these changes in this project ?
(I will keep working on the other)

@Xtansia Xtansia merged commit 7b3719b into opensearch-project:main Aug 15, 2024
56 checks passed
@Xtansia Xtansia added the backport 2.x Backport to 2.x branch label Aug 15, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 15, 2024
* Fixes #1137

Signed-off-by: uri.nudelman <uriel.nudelman@offerup.com>

* Adds missing documentation. Added changelog

Signed-off-by: uri.nudelman <uriel.nudelman@offerup.com>

* Added deserialization test

Signed-off-by: uri.nudelman <uriel.nudelman@offerup.com>

---------

Signed-off-by: uri.nudelman <uriel.nudelman@offerup.com>
Co-authored-by: uri.nudelman <uriel.nudelman@offerup.com>
(cherry picked from commit 7b3719b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dblock pushed a commit that referenced this pull request Aug 15, 2024
* Fixes #1137



* Adds missing documentation. Added changelog



* Added deserialization test



---------



(cherry picked from commit 7b3719b)

Signed-off-by: uri.nudelman <uriel.nudelman@offerup.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: uri.nudelman <uriel.nudelman@offerup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add query_image to Neural query
4 participants