Skip to content

Add spec for neural search plugin stats API #850

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

Merged
merged 7 commits into from
May 8, 2025

Conversation

q-andy
Copy link
Contributor

@q-andy q-andy commented Mar 24, 2025

Description

Spec for neural stats API added to feature branch in this PR: opensearch-project/neural-search#1256

Example API responses and test run output:

[INFO] => GET /_plugins/_neural/stats ({}) [application/json] 
[INFO] <= 200 (application/json; charset=UTF-8) | {
  "_nodes": {
    "total": 1,
    "successful": 1,
    "failed": 0
  },
  "cluster_name": "integTest",
  "info": {
    "cluster_version": "3.0.0",
    "processors": {
      "ingest": {
        "text_embedding_processors_in_pipelines": 0
      }
    }
  },
  "all_nodes": {
    "processors": {
      "ingest": {
        "text_embedding_executions": 0
      }
    }
  },
  "nodes": {
    "u4HvpBqoQPakwxqvrkOOWA": {
      "processors": {
        "ingest": {
          "text_embedding_executions": 0
        }
      }
    }
  }
}
[INFO] => GET /_plugins/_neural/stats/text_embedding_executions ({
  "flat_stat_paths": true,
  "include_metadata": true
}) [application/json] 
[INFO] <= 200 (application/json; charset=UTF-8) | {
  "_nodes": {
    "total": 1,
    "successful": 1,
    "failed": 0
  },
  "cluster_name": "integTest",
  "info": {},
  "all_nodes": {
    "processors.ingest.text_embedding_executions": {
      "value": 0,
      "stat_type": "timestamped_event_counter",
      "trailing_interval_value": 0,
      "minutes_since_last_event": 29047465
    }
  },
  "nodes": {
    "u4HvpBqoQPakwxqvrkOOWA": {
      "processors.ingest.text_embedding_executions": {
        "value": 0,
        "stat_type": "timestamped_event_counter",
        "trailing_interval_value": 0,
        "minutes_since_last_event": 29047465
      }
    }
  }
}

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.

@q-andy q-andy marked this pull request as ready for review April 16, 2025 16:45
@q-andy q-andy changed the title [Draft] Add spec for neural search plugin stats API Add spec for neural search plugin stats API Apr 16, 2025
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

If the neural plugin is enabled by default place tests under default. Adding something to tests/plugins/xyz doesn't automatically execute them, that requires updating CI test.yml.

@q-andy
Copy link
Contributor Author

q-andy commented Apr 17, 2025

Thanks for taking a look. This API is available from 3.0 beta onwards so I added a new entry in the test spect for it based on the release tag on docker hub following this part of the testing readme. I'm not super familiar with the CI and how the release process works, let me know if there's any additional changes needed

@dblock
Copy link
Member

dblock commented Apr 17, 2025

Thanks for taking a look. This API is available from 3.0 beta onwards so I added a new entry in the test spect for it based on the release tag on docker hub following this part of the testing readme. I'm not super familiar with the CI and how the release process works, let me know if there's any additional changes needed

This CI here may be confusing, so appreciate you hanging here with me. The documentation could use help to make it clearer (wink, wink).

  1. We should be testing 3.0-latest whichever one it is, there's no point in testing both alpha and beta. So do that upgrade first (possibly in a separate PR) to the latest 3.0 available.
  2. The neural plugin feature exists in 3.0 by default, which means you do not need to make a custom test suite in tests/plugins, that's only to install custom plugins, and thus you want to add to the default folder of tests which runs against all versions.
  3. Because the API is only available in 3.0, both the API spec and the tests need to be marked >= 3.0. Thus CI will only run the neural tests you are adding against 3.0+.

Does this make sense?

@q-andy
Copy link
Contributor Author

q-andy commented Apr 17, 2025

Yup, thanks for the swift reply!

  1. Sure, I believe currently 3.0.0 beta was just cut, does it make sense to wait until 3.0 GA to make the upgrade, and then merge this PR afterwards? Since if we change all of them to 3.0.0 beta, I'll assume we'll have to do switch them to 3.0 GA afterwards anyways, similar to something like this PR I'm assuming
  2. Ah, I don't believe neural exists in OpenSearch by default, it depends on kNN and ml-commons plugins which I see also have separate (non-default) specs so I assume it follows a similar pattern. I believe it makes sense to put them in the CI separately?
  3. Sure, I'll add a version: '>= 3.0' line to the neural test yaml, similar to this. For the API spec itself I've added the x-version-added: '3.0' tags, which should be enough?

@dblock
Copy link
Member

dblock commented Apr 18, 2025

  1. Sure, I believe currently 3.0.0 beta was just cut, does it make sense to wait until 3.0 GA to make the upgrade, and then merge this PR afterwards? Since if we change all of them to 3.0.0 beta, I'll assume we'll have to do switch them to 3.0 GA afterwards anyways, similar to something like this PR I'm assuming

Someone should upgrade to beta ASAP to test things before they get released. Including this plugin.

  1. Ah, I don't believe neural exists in OpenSearch by default, it depends on kNN and ml-commons plugins which I see also have separate (non-default) specs so I assume it follows a similar pattern. I believe it makes sense to put them in the CI separately?

If the plugin is not installed by default then yes. You will need to customize Dockerfile accordingly.

I did click the button to run CI on this PR as is, it might yield other problems.

  1. Sure, I'll add a version: '>= 3.0' line to the neural test yaml, similar to this. For the API spec itself I've added the x-version-added: '3.0' tags, which should be enough?

Yep.

Copy link
Contributor

github-actions bot commented Apr 18, 2025

Changes Analysis

Commit SHA: f1b217f
Comparing To SHA: 0f7b8c5

API Changes

Summary

├─┬Paths
│ ├──[➕] path (5975:3)
│ ├──[➕] path (5938:3)
│ ├──[➕] path (5992:3)
│ └──[➕] path (5956:3)
└─┬Components
  ├──[➕] responses (32796:7)
  ├──[➕] parameters (24548:7)
  ├──[➕] parameters (24559:7)
  ├──[➕] parameters (24570:7)
  ├──[➕] parameters (24541:7)
  ├──[➕] schemas (59306:7)
  ├──[➕] schemas (59334:7)
  ├──[➕] schemas (59300:7)
  ├──[➕] schemas (59279:7)
  ├──[➕] schemas (59290:7)
  ├──[➕] schemas (59325:7)
  ├──[➕] schemas (59251:7)
  ├──[➕] schemas (59284:7)
  ├──[➕] schemas (59261:7)
  ├──[➕] schemas (59271:7)
  ├──[➕] schemas (59236:7)
  ├──[➕] schemas (59350:7)
  ├──[➕] schemas (59295:7)
  ├──[➕] schemas (59230:7)
  ├──[➕] schemas (59221:7)
  └──[➕] schemas (59321:7)

Document Element Total Changes Breaking Changes
paths 4 0
components 21 0
  • Total Changes: 25
  • Additions: 25

Report

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

API Coverage

Before After Δ
Covered (%) 663 (64.94 %) 663 (64.94 %) 0 (0 %)
Uncovered (%) 358 (35.06 %) 358 (35.06 %) 0 (0 %)
Unknown 49 53 4

@q-andy
Copy link
Contributor Author

q-andy commented Apr 24, 2025

@dblock 3.0.0-beta1 released a few days ago, I've included changes for the version matrix bump in this PR. Happy to break it up into a separate PR as well if that makes more sense.

I've also verified neural-search is included in the release artifact by default with the following:

$ docker run -it --entrypoint bash opensearchproject/opensearch:3.0.0-beta1 -c "cat /usr/share/opensearch/manifest.yml" | grep neural
  - name: neural-search
    repository: https://github.com/opensearch-project/neural-search.git
    location: https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/3.0.0-beta1/11026/linux/arm64/tar/builds/opensearch/plugins/opensearch-neural-search-3.0.0.0-beta1.zip

Some question about this:

If the neural plugin is enabled by default place tests under default. Adding something to tests/plugins/xyz doesn't automatically execute them, that requires updating CI test.yml.

  • So if neural-search is included as part of the release artifact then should it be under the default? If that's so, why would ml-commons and kNN be under test/plugins/xyz, shouldn't they also be undert default?
  • Should I still include a - version 2.19.1 entry in the version matrix to ensure the integration tests still run against a 2.19.x version? I still there will be 2.19.2, 2.19.3 release in the future: https://opensearch.org/releases/

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This repo tests multiple versions, undo all the changes from 2.19.1 to 3.0.0-beta1. You should do the upgrade from 3.0-alpha-1 to 3.0-beta-1 in a separate PR, then add tests for the neural stats here.

q-andy added 3 commits May 6, 2025 10:15
Signed-off-by: Andy Qin <[email protected]>
@q-andy
Copy link
Contributor Author

q-andy commented May 6, 2025

@dblock Addressed remaining comments and also the tests to default since neural search is installed by default. I've also added x-version-added: 3.0 tags to the schema since we plan to add new stats with new versions. Since 3.0.0-beta1 has been added to the test specs in #883, the workflows should run successfully.

Tests pass locally with npm run test:spec -- --opensearch-insecure --opensearch-url=http://localhost:9200 --tests tests/default/neural --verbose.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>
dblock
dblock previously approved these changes May 7, 2025
@dblock
Copy link
Member

dblock commented May 7, 2025

Changes look good. Fix spec linting / validation and it's good to go.

Copy link
Contributor

github-actions bot commented May 7, 2025

Spec Test Coverage Analysis

Total Tested
603 601 (99.67 %)

Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
@q-andy
Copy link
Contributor Author

q-andy commented May 7, 2025

Thanks, fixed the failing workflows. All of the following commands run successfully:

npm run test:spec -- --opensearch-insecure --opensearch-url=http://localhost:9200  --tests tests/default/neural          
npm run lint
npm run lint:spec

@dblock dblock merged commit c042c5f into opensearch-project:main May 8, 2025
32 checks passed
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.

3 participants