opensearch-api-specification icon indicating copy to clipboard operation
opensearch-api-specification copied to clipboard

Published 20 hours ago β€’ verified publisher icon opensearch-project

Added Query Set API Specs

Open ajleong623 opened this issue 6 months ago β€’ 2 comments

Description

Added some specifications for the query sets in the search relevance workbench.

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.

ajleong623 avatar May 19 '25 09:05 ajleong623

@wrigleydan can you give some eyes to the specific SRW apis defined here?

epugh avatar May 19 '25 14:05 epugh

We are very interested in understanding if @ajleong623 has appropriately structured a new API addition. The underlying effort is the new Search Relevance Workbench, coming in 3.1!

epugh avatar May 19 '25 15:05 epugh

Great start to have specs to our APIs! Thanks @ajleong623

We might want to add a little to distinguish PUT and POST from each other: Currently PUT is to create a query set by uploading queries manually, and POST creates a query set by sampling queries from user behavior data.

In general, I think x-version-added needs to be 3.1 everywhere, since we aim for a 3.1 release for these APIs (see https://github.com/opensearch-project/opensearch-api-specification/blob/main/DEVELOPER_GUIDE.md#openapi-extensions).

Spec tests would be awesome so that we not only have a description but actually use the specs to validate our developments and see when we need to update things. (see https://github.com/opensearch-project/opensearch-api-specification/blob/main/DEVELOPER_GUIDE.md#writing-spec-tests). I don't know if that's a hard requirement but I definitely think that tests would help us in the development process.

wrigleyDan avatar May 20 '25 08:05 wrigleyDan

I don't know if that's a hard requirement but I definitely think that tests would help us in the development process.

Tests are a hard requirement. Thanks!

dblock avatar May 20 '25 21:05 dblock

@ajleong623 do we need to cut a version of the search-relevance plugin that you can refer to in the spec tests (and somehow load into the opensearch fired up via docker?)

epugh avatar May 21 '25 13:05 epugh

@epugh, unfortunately, I am not sure. All I did was ./gradlew run on the search-relevance repository when I had to connect to a cluster for the tests.

ajleong623 avatar May 21 '25 16:05 ajleong623

@ajleong623 You'll need the plugin installed and tests to pass in CI. See https://github.com/opensearch-project/opensearch-api-specification/blob/main/TESTING_GUIDE.md.

dblock avatar May 24 '25 13:05 dblock

@dblock I don’t think the plugin has been distributed at the moment, so I am not sure if I can install it. I will just write the rest of the specs for the search relevance workbench APIs for now then figure out how to install it.

ajleong623 avatar May 24 '25 17:05 ajleong623

You can install a -SNAPSHOT build of it.

dblock avatar May 24 '25 18:05 dblock

The functional tests and the snapshot have been added for all the listed queries.

ajleong623 avatar May 28 '25 17:05 ajleong623

@ajleong623 mark this PR ready when it's ready and we can take a look

dblock avatar May 29 '25 20:05 dblock

I think it is ready, but how do I run the CI tests?

ajleong623 avatar May 31 '25 17:05 ajleong623

I think it is ready, but how do I run the CI tests?

They should have kicked off. Not sure why they aren't πŸ€”

Either way this is a new plugin setup that would need to be added to https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/test-spec.yml, so rebase your PR and add that?

dblock avatar May 31 '25 23:05 dblock

I added the workflows. What was preventing the tests from running was that the changelog was not being cleanly merged, but I resolved the conflict. I think someone also has to approve the workflows.

ajleong623 avatar Jun 01 '25 05:06 ajleong623

Changes Analysis

Commit SHA: 4b0a9a0e901e90adb352577312bff95a3ab79830 Comparing To SHA: 3ce15ec9e2cc1e73a192ce937646009e56a76ec4

API Changes

Summary

β”œβ”€β”¬Paths
β”‚ β”œβ”€β”€[βž•] path (9515:3)
β”‚ β”œβ”€β”€[βž•] path (9468:3)
β”‚ β”œβ”€β”€[βž•] path (9546:3)
β”‚ β”œβ”€β”€[βž•] path (9437:3)
β”‚ β”œβ”€β”€[βž•] path (9374:3)
β”‚ β”œβ”€β”€[βž•] path (9405:3)
β”‚ β”œβ”€β”€[βž•] path (9577:3)
β”‚ └──[βž•] path (9343:3)
└─┬Components
  β”œβ”€β”€[βž•] requestBodies (30384:7)
  β”œβ”€β”€[βž•] requestBodies (30398:7)
  β”œβ”€β”€[βž•] responses (34371:7)
  β”œβ”€β”€[βž•] responses (34346:7)
  β”œβ”€β”€[βž•] responses (34304:7)
  β”œβ”€β”€[βž•] requestBodies (30453:7)
  β”œβ”€β”€[βž•] requestBodies (30469:7)
  β”œβ”€β”€[βž•] requestBodies (30414:7)
  β”œβ”€β”€[βž•] responses (34338:7)
  β”œβ”€β”€[βž•] responses (34322:7)
  β”œβ”€β”€[βž•] responses (34354:7)
  β”œβ”€β”€[βž•] responses (34316:7)
  β”œβ”€β”€[βž•] responses (34298:7)
  β”œβ”€β”€[βž•] responses (34365:7)
  β”œβ”€β”€[βž•] responses (34330:7)
  β”œβ”€β”€[βž•] responses (34310:7)
  β”œβ”€β”€[βž•] responses (34380:7)
  β”œβ”€β”€[βž•] responses (34391:7)
  β”œβ”€β”€[βž•] parameters (26193:7)
  β”œβ”€β”€[βž•] parameters (26221:7)
  β”œβ”€β”€[βž•] parameters (26186:7)
  β”œβ”€β”€[βž•] parameters (26200:7)
  β”œβ”€β”€[βž•] parameters (26214:7)
  β”œβ”€β”€[βž•] parameters (26207:7)
  β”œβ”€β”€[βž•] parameters (26228:7)
  β”œβ”€β”€[βž•] parameters (26235:7)
  β”œβ”€β”€[βž•] schemas (64961:7)
  β”œβ”€β”€[βž•] schemas (64944:7)
  β”œβ”€β”€[βž•] schemas (64933:7)
  β”œβ”€β”€[βž•] schemas (64976:7)
  └──[βž•] schemas (64908:7)
Document Element Total Changes Breaking Changes
paths 8 0
components 31 0
  • Total Changes: 39
  • Additions: 39

Report

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

API Coverage

Before After Ξ”
Covered (%) 663 (64.94 %) 663 (64.94 %) 0 (0 %)
Uncovered (%) 358 (35.06 %) 358 (35.06 %) 0 (0 %)
Unknown 70 87 17

github-actions[bot] avatar Jun 01 '25 12:06 github-actions[bot]

@ajleong623 I clicked the approve button, iterate to 🟒 , will keep doing that

dblock avatar Jun 01 '25 12:06 dblock

I am still working on having all the tests and the linter ones pass.

ajleong623 avatar Jun 02 '25 06:06 ajleong623

I think that the tests are passing because I ran the linter tests on my end.

Edit: I actually still need approval from maintainers for the workflows.

ajleong623 avatar Jun 03 '25 00:06 ajleong623

Looks like we are getting some warnings still, but getting closer!

WARNING Multiple paths detected, please group similar tests together and move paths not being tested to prologues or epilogues.
WARNING Invalid path detected, please move /tests/plugins/search_relevance/experiments.yaml to plugins/search_relevance/query_sets.yaml.
WARNING Invalid path detected, please move /tests/plugins/search_relevance/experiments.yaml to plugins/search_relevance/search_configurations.yaml.

epugh avatar Jun 03 '25 20:06 epugh

Looks like we are getting some warnings still, but getting closer!

WARNING Multiple paths detected, please group similar tests together and move paths not being tested to prologues or epilogues.
WARNING Invalid path detected, please move /tests/plugins/search_relevance/experiments.yaml to plugins/search_relevance/query_sets.yaml.
WARNING Invalid path detected, please move /tests/plugins/search_relevance/experiments.yaml to plugins/search_relevance/search_configurations.yaml.

Looking better!

epugh avatar Jun 03 '25 23:06 epugh

@epugh are there still warnings on your end? I do not see it on my end.

ajleong623 avatar Jun 03 '25 23:06 ajleong623

@Xtansia if it’s possible, could you please review the updated changes?

ajleong623 avatar Jun 06 '25 13:06 ajleong623

Spec Test Coverage Analysis

Total Tested
637 635 (99.69 %)

github-actions[bot] avatar Jun 07 '25 14:06 github-actions[bot]

Overall looks OK. I have an important question below.

Less important in the code there are a few things like "I am not sure why ..." change those to clear TODO: figure out ...

@ajleong623 if you want some help reformulating those comments into more action oriented.. Also, maybe ping @sstults on if we can actually get someting more specific ;-). Getting closer!

epugh avatar Jun 07 '25 15:06 epugh

@dblock I changed the comments to now begin with TODO and the specific action that can be taken. Also, @epugh answered the more important question about the build.

ajleong623 avatar Jun 07 '25 21:06 ajleong623

@epugh @ajleong623 I am still not clear on whether CI will start suddenly failing with a future build of OpenSearch 3.1. The Dockerfile uses opensearchstaging/opensearch:3.1.0 which is a moving target, while the -SNAPSHOT build of the plugin being downloaded is a fixed, manually uploaded version. So I think we have to use a fixed SHA or download a latest matching build for both the plugin and OpenSearch core, or wait till 3.1.0 gets released and the plugin gets released to add it here?

dblock avatar Jun 08 '25 22:06 dblock

@dblock @epugh in test-spec.yml, I specified the sha to use for testing found here: https://hub.docker.com/layers/opensearchstaging/opensearch/3.1.0/images/sha256-72ce9569524d12a2af5912a84c72ea73639aee0e9af04e56584cbc84be8899b3. Is that the right approach with using a fixed sha?

ajleong623 avatar Jun 09 '25 17:06 ajleong623

@dblock @epugh in test-spec.yml, I specified the sha to use for testing found here: https://hub.docker.com/layers/opensearchstaging/opensearch/3.1.0/images/sha256-72ce9569524d12a2af5912a84c72ea73639aee0e9af04e56584cbc84be8899b3. Is that the right approach with using a fixed sha?

Maybe? i hope these SHA stay for a while and don't go away..

One reason I am pushing on this is that I'm hoping in 3.1 to use this api spec in the documentation for the Plugin. However, it may be that we have a kind of chicken/egg thing. We need OpenSearch 3.1, the new API spec, and our plugin all to land ;-) It may be that merging this and tweaking the docs to reference this has to happen after 3.1 and the plugin are released?

epugh avatar Jun 10 '25 11:06 epugh

Is that the right approach with using a fixed sha?

Yes, this looks better now.

dblock avatar Jun 10 '25 18:06 dblock

@ajleong623 there is one more API added to search-relevance recently, stats API https://github.com/opensearch-project/search-relevance/issues/47. Do we need to update spec and include new API?

martin-gaievski avatar Jun 12 '25 00:06 martin-gaievski