Added Tests for Search Pipeline and Notifications Plugin #668

AbitraryYu · 2024-02-06T10:38:14Z

Description

Describe what this change achieves.

This change tries to solve #474

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

If this is resolved, this closes #474

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.

codecov · 2024-02-06T10:58:56Z

Codecov Report

Attention: Patch coverage is 56.52174% with 40 lines in your changes are missing coverage. Please review.

Project coverage is 71.80%. Comparing base (ba715b9) to head (3d7586c).
Report is 11 commits behind head on main.

❗ Current head 3d7586c differs from pull request most recent head 891c250. Consider uploading reports for the commit 891c250 to get more accurate results

Files	Patch %	Lines
opensearchpy/_async/plugins/notifications.py	53.57%	13 Missing ⚠️
opensearchpy/plugins/notifications.py	53.57%	13 Missing ⚠️
opensearchpy/_async/client/search_pipeline.py	50.00%	7 Missing ⚠️
opensearchpy/client/search_pipeline.py	50.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #668      +/-   ##
==========================================
- Coverage   71.95%   71.80%   -0.16%     
==========================================
  Files          91       95       +4     
  Lines        8001     8103     +102     
==========================================
+ Hits         5757     5818      +61     
- Misses       2244     2285      +41

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

orangewise · 2024-02-06T15:35:14Z

Question, would this also work with opensearch serverless (aws)?

dblock

Was this code actually generated? The "right" way to do this is to add search pipelines to https://github.com/opensearch-project/opensearch-api-specification and then to generate the client.

AbitraryYu · 2024-02-07T07:39:04Z

I see there's a PR in opensearch-api-specification . If that PR is merged, then does that mean the search pipeline api for opensearch-py can be generated? Thanks.

saimedhi · 2024-02-07T08:08:30Z

I see there's a PR in opensearch-api-specification . If that PR is merged, then does that mean the search pipeline api for opensearch-py can be generated? Thanks.

@AbitraryYu, once the spec is merged, we can generate the search pipeline API. After generation, we'll review it for any necessary fixes to the generator code.
Your contribution to getting the spec merged would be greatly appreciated, and it will help us move forward with API generation. Thank you.

lambda-science · 2024-02-28T19:31:13Z

Any follow up on this ? .. We need search pipeline API

saimedhi · 2024-02-28T19:44:13Z

Any follow up on this ? .. We need search pipeline API

@lambda-science, @ArbitraryYu, please feel free to contribute to the spec to get it merged.

opensearch-project/opensearch-api-specification#172

AbitraryYu · 2024-03-26T03:11:13Z

When I try to generate the code using nox -rs generate it fails at /utils/generate_api.py line 595. I have printed some of the debug logs (by inserting some print statements on my own) and saw the following:

{'name': 'config_id', 'in': 'query', 'description': 'A channel ID.', 'schema': {'type': 'string', 'description': 'A channel ID.'}}
{'name': 'config_id_list', 'in': 'query', 'description': 'A comma-separated list of channel IDs.', 'schema': {'type': 'string', 'description': 'A comma-separated list of channel IDs.'}}
{'name': 'last_updated_time_ms', 'in': 'query', 'schema': {'type': 'integer', 'format': 'int64'}}
Traceback (most recent call last):
  File "<repo_dir>/utils/generate_api.py", line 853, in <module>
    dump_modules(read_modules())
                 ^^^^^^^^^^^^^^
  File "<repo_dir>/utils/generate_api.py", line 595, in read_modules
    type=m["schema"]["type"], description=m["description"]
                                          ~^^^^^^^^^^^^^^^
KeyError: 'description'
nox > Command python utils/generate_api.py failed with exit code 1
nox > Session generate failed.

I take a look at the OpenSearch.openapi.json at raw.github, and find out it is the GET parameters of /_plugins/_notifications/configs that lack the description (on line 13377, or see the picture)

I saw some other parameters also lack description, I am not sure if there are more parameters that lack description.

My question is: Are there any methods that I can skip the problematic part and just generate the search pipeline API part?If not, is it a new issue that need to be raised in the opensearch-api-specification repository?

saimedhi · 2024-03-26T05:28:24Z

When I try to generate the code using nox -rs generate it fails at /utils/generate_api.py line 595. I have printed some of the debug logs (by inserting some print statements on my own) and saw the following:
{'name': 'config_id', 'in': 'query', 'description': 'A channel ID.', 'schema': {'type': 'string', 'description': 'A channel ID.'}}
{'name': 'config_id_list', 'in': 'query', 'description': 'A comma-separated list of channel IDs.', 'schema': {'type': 'string', 'description': 'A comma-separated list of channel IDs.'}}
{'name': 'last_updated_time_ms', 'in': 'query', 'schema': {'type': 'integer', 'format': 'int64'}}
Traceback (most recent call last):
  File "<repo_dir>/utils/generate_api.py", line 853, in <module>
    dump_modules(read_modules())
                 ^^^^^^^^^^^^^^
  File "<repo_dir>/utils/generate_api.py", line 595, in read_modules
    type=m["schema"]["type"], description=m["description"]
                                          ~^^^^^^^^^^^^^^^
KeyError: 'description'
nox > Command python utils/generate_api.py failed with exit code 1
nox > Session generate failed.
I take a look at the OpenSearch.openapi.json at raw.github, and find out it is the GET parameters of /_plugins/_notifications/configs that lack the description (on line 13377, or see the picture)

I saw some other parameters also lack description, I am not sure if there are more parameters that lack description.

My question is: Are there any methods that I can skip the problematic part and just generate the search pipeline API part?If not, is it a new issue that need to be raised in the opensearch-api-specification repository?

Hey @AbitraryYu, to avoid the generator error, we could tweak the code to first check if the parameter description exists before using it. Yet, I believe it's even better to include the parameter description that is missing in the spec.

@dblock, what's your take on this?

saimedhi · 2024-03-26T05:42:03Z

@AbitraryYu, please try fixing #710.

So, that all APIs from spec will be generated by update_api workflow and PR will be raised automatically if changes detected. Consider raising PR for spec repo with missing descriptions or fix generator to skip using description if not present. Thank you.

dblock · 2024-03-26T13:28:53Z

@dblock, what's your take on this?

If the description is not required, the generator should gracefully handle that.

AbitraryYu · 2024-04-02T03:11:50Z

The code is generated via nox, and I modified the generator in generate_api.py to gracefully ignore messages if the API specification is empty.

By the way, the raw link of the opensearch "https://raw.githubusercontent.com/opensearch-project/opensearch-api-specification/main/OpenSearch.openapi.json" seems giving me 404 (line 550-554), so I use my local version downloaded from last week.

AbitraryYu · 2024-04-02T06:15:45Z

A side note, the latest version of opensearch (docker) require admin password, and I cannot get it run tests locally. I am not sure if this repository supports password parsing so I am using an older version: 2.11.1 (docker).

dblock

Let me try to help with the password issue.

In 2.12 you need a default password, https://opensearch.org/blog/replacing-default-admin-credentials/, this repo was updated to act accordingly AFAIK.

We're moving things around in opensearch-api-specification, so you might hit issues, opensearch-project/opensearch-api-specification#189

Iterate this change to green?

utils/generate_api.py

saimedhi

Hello @AbitraryYu,

It appears there might have been an issue during the generation process.
Could you confirm if you ran nox -rs generate and it is successful? This command triggers both the generator and the formatter. If the formatter step was skipped, it might explain the extensive changes to files. Generally, with the formatter running, modifications should be limited to newly added APIs.
Feel free to reach out if you require any assistance. Your contribution is greatly appreciated!

utils/generate_api.py

saimedhi · 2024-04-08T18:27:21Z

@AbitraryYu, Thank you for implementing the requested changes! Could you also include tests for these generated APIs?

AbitraryYu · 2024-04-14T14:27:07Z

Hi @saimedhi , I got confused where I should implement the tests. I saw test_opensearch/test_helpers/test_search.py and test_opensearch/test_async/test_helpers/test_search.py, and they are using from opensearchpy.helpers import search. I am wondering do I need to create a new helper file in opensearchpy.helpers plus creating both new test files in test_opensearch/test_helpers/ and test_opensearch//test_async/test_helpers/. Thanks.

saimedhi · 2024-04-14T20:13:26Z

Hi @saimedhi , I got confused where I should implement the tests. I saw test_opensearch/test_helpers/test_search.py and test_opensearch/test_async/test_helpers/test_search.py, and they are using from opensearchpy.helpers import search. I am wondering do I need to create a new helper file in opensearchpy.helpers plus creating both new test files in test_opensearch/test_helpers/ and test_opensearch//test_async/test_helpers/. Thanks.

@AbitraryYu,

For search_pipeline unit tests, they can be added here. Please create a file for search_pipeline test cases in that folder.
For search_pipeline integration tests, removing the skip test here is sufficient.
Notification plugin tests can be added here, and async tests here.

saimedhi · 2024-04-19T18:03:13Z

Thank you for your contribution, @ArbitraryYu. The Search Pipeline and Notification APIs have now been added to opensearch-py. We would greatly appreciate your help in adding tests for the Notifications Plugin whenever you have the chance.

AbitraryYu · 2024-04-20T10:10:55Z

Sorry for all the dirty commits, I accidentally committed some sensitive data to this branch and I attempt to remove it both locally and remotely. I already force push the git history so the my local repo feature branch cannot access it. However, this PR still have the stray commit that contain the sensitive data.

I just submitted a Github support request for to clear the cached views but might need this PR's admin permission to actually take effect.

Update: The affected commit seems to be hidden after 2 hours, would let you guys know if Github support need your support. Thank you.

Update 2: The GitHub support team removed the affected commit cache and I confirmed the affected commit is no longer accessible, so I think the case is closed. Thank you.

saimedhi · 2024-04-22T05:25:40Z

opensearchpy/_async/client/__init__.py

@@ -224,6 +224,7 @@ class as kwargs, or a string in the format of ``host[:port]`` which will be
        self.ingest = IngestClient(self)
        self.nodes = NodesClient(self)
        self.remote = RemoteClient(self)
+        self.search_pipeline = SearchPipelineClient(self)


@ArbitraryYu, thanks for your contribution. Just a few adjustments needed.

No modification needed for this file. In a recent PR, self.search_pipeline = SearchPipelineClient(self) was added.

saimedhi · 2024-04-22T05:26:01Z

opensearchpy/client/__init__.py

@@ -224,6 +224,7 @@ class as kwargs, or a string in the format of ``host[:port]`` which will be
        self.ingest = IngestClient(self)
        self.nodes = NodesClient(self)
        self.remote = RemoteClient(self)
+        self.search_pipeline = SearchPipelineClient(self)


No modification needed for this file

saimedhi · 2024-04-22T05:40:47Z

test_opensearchpy/test_server/test_rest_api_spec.py

@@ -72,7 +72,6 @@
 # broken YAML tests on some releases
 SKIP_TESTS = {
    # Warning about date_histogram.interval deprecation is raised randomly
-    "OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/search_pipeline/10_basic",
    "OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic",


Please remove the change OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/search_pipeline/10_basic to avoid blocking this PR, as two tests are currently failing.

It will be handled in #726 . Integration tests with OpenSearch 2.7 and 2.8 are failing due to a server bug, and I'll raise an issue in the appropriate repo.

saimedhi · 2024-04-22T05:49:30Z

@ArbitraryYu, please resolve the failing DCO check.

AbitraryYu · 2024-04-22T11:08:45Z

For the DCO, I try to follow the DCO guide, git rebase HEAD~10 --signoff, going through the rebase process, and once I push it, the PR got 49+ files changed. I was wondering if I can add those signoff(s) by modifying a specific commit?

dblock · 2024-04-22T12:55:16Z

For the DCO, I try to follow the DCO guide, git rebase HEAD~10 --signoff, going through the rebase process, and once I push it, the PR got 49+ files changed. I was wondering if I can add those signoff(s) by modifying a specific commit?

It's probably easier to re-create the changes in the branch, or do https://code.dblock.org/2015/08/31/getting-out-of-your-first-git-mess.html.

git checkout feature/search_pipeline_api
git checkout -b feature/search_pipeline_api-backup
git checkout feature/search_pipeline_api
git reset HEAD~1 # repeat this until you've rewound your branch enough
git add .
git commit -s -m ...
git push origin feature/search_pipeline_api-backup -f

Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

…nflict. Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

AbitraryYu · 2024-04-23T02:41:37Z

Fixed the DCO, and the tests seems ok.

saimedhi · 2024-04-23T07:44:02Z

test_opensearchpy/test_server/test_plugins/test_notification.py

+
+
+class TestNotificationPlugin(OpenSearchTestCase):
+    async def test_create_channel_notification(self) -> None:


@AbitraryYu, please remove async from all the test cases in this file.

After removing async, you might notice that some test cases fail. Please fix them.

For example, test_delete_channel_configuration may fail. Remember, each test case is isolated, so create the configuration before deleting it within the same test case. See this reference. Thank you.

Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

saimedhi · 2024-04-26T21:07:04Z

@AbitraryYu, please try to fix failing tests.

Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

AbitraryYu · 2024-04-28T09:08:07Z

I followed the example on test_index_management.py of this reference, and I got errors like:

error: Unsupported target for indexed assignment ("Collection[str]") on here
error: Value of type "Collection[str]" is not indexable on here.

How can I fix that?

Meanwhile, the test_list_all_config() returns an empty list of config list, so I didn't write an assertEqual test; while test_get_config() returns something on the config list. I am not sure if test_list_all_config() is behaving properly, both tests create an index self.test_create_config() at the start.

saimedhi · 2024-04-29T16:38:58Z

I followed the example on test_index_management.py of this reference, and I got errors like:

error: Unsupported target for indexed assignment ("Collection[str]") on here

error: Value of type "Collection[str]" is not indexable on here.

How can I fix that?

@ArbitraryYu, follow these steps to resolve the errors:

Format the code using nox -rs format.
Add type hints for CONTENT here:

CONTENT: Dict[str, Any] = {
    "config_id": "sample-id",
    # ... other keys ...
}

Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

saimedhi · 2024-04-30T19:35:26Z

@dblock, I'm unable to merge the PR. Could you please review, approve, and merge it? Thank you!

…project#668) * Added Tests for Notification and Search Pipeline.ml Signed-off-by: AbitraryYu <nikkoyhc@gmail.com> * Add back the lines of comment in indices.py during resolving merge conflict. Signed-off-by: AbitraryYu <nikkoyhc@gmail.com> * Fix notification plugin tests, from async def to def. Signed-off-by: AbitraryYu <nikkoyhc@gmail.com> * Fix test errors. Rename function names and rewrite assertion tests. Signed-off-by: AbitraryYu <nikkoyhc@gmail.com> * Fix formatting errors. Added typings to CONTENT. Signed-off-by: AbitraryYu <nikkoyhc@gmail.com> --------- Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

AbitraryYu requested review from VachaShah, dblock, harshavamsi, axeoman, Shephalimittal, saimedhi and florianvazelle as code owners February 6, 2024 10:38

dblock requested changes Feb 6, 2024

View reviewed changes

saimedhi mentioned this pull request Feb 28, 2024

Search pipelines added opensearch-project/opensearch-api-specification#172

Merged

saimedhi added the API Spec Update Required label Mar 19, 2024

saimedhi mentioned this pull request Mar 26, 2024

[BUG]API generator failing #710

Closed

dblock requested changes Apr 2, 2024

View reviewed changes

utils/generate_api.py Outdated Show resolved Hide resolved

saimedhi requested changes Apr 2, 2024

View reviewed changes

saimedhi reviewed Apr 2, 2024

View reviewed changes

utils/generate_api.py Outdated Show resolved Hide resolved

saimedhi requested changes Apr 22, 2024

View reviewed changes

saimedhi added the skip-changelog Skips changelog verifier label Apr 22, 2024

AbitraryYu force-pushed the feature/search_pipeline_api branch 2 times, most recently from 846c25b to ead8373 Compare April 22, 2024 11:05

saimedhi changed the title ~~Added Search Pipeline API~~ Added Tests for Search Pipeline and Notifications Plugin Apr 22, 2024

saimedhi mentioned this pull request Apr 22, 2024

[FEATURE] Add Tests for Notifications Plugin #636

Closed

Added Tests for Notification and Search Pipeline.ml

82cebac

Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

AbitraryYu force-pushed the feature/search_pipeline_api branch from ead8373 to 82cebac Compare April 23, 2024 02:18

AbitraryYu added 2 commits April 23, 2024 10:29

Merge branch 'main' into feature/search_pipeline_api

debb7d1

Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

Add back the lines of comment in indices.py during resolving merge co…

e8c413d

…nflict. Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

saimedhi approved these changes Apr 23, 2024

View reviewed changes

saimedhi requested changes Apr 23, 2024

View reviewed changes

saimedhi added the opensearch-py 2.6.0 label Apr 23, 2024

Fix notification plugin tests, from async def to def.

2db146c

Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

Fix test errors. Rename function names and rewrite assertion tests.

3240540

Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

Fix formatting errors. Added typings to CONTENT.

891c250

Signed-off-by: AbitraryYu <nikkoyhc@gmail.com>

saimedhi approved these changes Apr 30, 2024

View reviewed changes

saimedhi requested a review from dblock April 30, 2024 19:29

dblock approved these changes Apr 30, 2024

View reviewed changes

dblock merged commit 0caacf8 into opensearch-project:main Apr 30, 2024
55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Tests for Search Pipeline and Notifications Plugin #668

Added Tests for Search Pipeline and Notifications Plugin #668

AbitraryYu commented Feb 6, 2024

codecov bot commented Feb 6, 2024 •

edited

Loading

orangewise commented Feb 6, 2024

dblock left a comment

AbitraryYu commented Feb 7, 2024

saimedhi commented Feb 7, 2024

lambda-science commented Feb 28, 2024

saimedhi commented Feb 28, 2024

AbitraryYu commented Mar 26, 2024

saimedhi commented Mar 26, 2024

saimedhi commented Mar 26, 2024 •

edited

Loading

dblock commented Mar 26, 2024

AbitraryYu commented Apr 2, 2024 •

edited

Loading

AbitraryYu commented Apr 2, 2024 •

edited

Loading

dblock left a comment •

edited

Loading

saimedhi left a comment

saimedhi commented Apr 8, 2024

AbitraryYu commented Apr 14, 2024

saimedhi commented Apr 14, 2024

saimedhi commented Apr 19, 2024

AbitraryYu commented Apr 20, 2024 •

edited

Loading

saimedhi Apr 22, 2024

saimedhi Apr 22, 2024

saimedhi Apr 22, 2024

saimedhi commented Apr 22, 2024

AbitraryYu commented Apr 22, 2024

dblock commented Apr 22, 2024

AbitraryYu commented Apr 23, 2024

saimedhi Apr 23, 2024

saimedhi commented Apr 26, 2024

AbitraryYu commented Apr 28, 2024 •

edited

Loading

saimedhi commented Apr 29, 2024 •

edited

Loading

saimedhi commented Apr 30, 2024



		class TestNotificationPlugin(OpenSearchTestCase):
		async def test_create_channel_notification(self) -> None:

Added Tests for Search Pipeline and Notifications Plugin #668

Added Tests for Search Pipeline and Notifications Plugin #668

Conversation

AbitraryYu commented Feb 6, 2024

Description

Issues Resolved

codecov bot commented Feb 6, 2024 • edited Loading

Codecov Report

orangewise commented Feb 6, 2024

dblock left a comment

Choose a reason for hiding this comment

AbitraryYu commented Feb 7, 2024

saimedhi commented Feb 7, 2024

lambda-science commented Feb 28, 2024

saimedhi commented Feb 28, 2024

AbitraryYu commented Mar 26, 2024

saimedhi commented Mar 26, 2024

saimedhi commented Mar 26, 2024 • edited Loading

dblock commented Mar 26, 2024

AbitraryYu commented Apr 2, 2024 • edited Loading

AbitraryYu commented Apr 2, 2024 • edited Loading

dblock left a comment • edited Loading

Choose a reason for hiding this comment

saimedhi left a comment

Choose a reason for hiding this comment

saimedhi commented Apr 8, 2024

AbitraryYu commented Apr 14, 2024

saimedhi commented Apr 14, 2024

saimedhi commented Apr 19, 2024

AbitraryYu commented Apr 20, 2024 • edited Loading

saimedhi Apr 22, 2024

Choose a reason for hiding this comment

saimedhi Apr 22, 2024

Choose a reason for hiding this comment

saimedhi Apr 22, 2024

Choose a reason for hiding this comment

saimedhi commented Apr 22, 2024

AbitraryYu commented Apr 22, 2024

dblock commented Apr 22, 2024

AbitraryYu commented Apr 23, 2024

saimedhi Apr 23, 2024

Choose a reason for hiding this comment

saimedhi commented Apr 26, 2024

AbitraryYu commented Apr 28, 2024 • edited Loading

saimedhi commented Apr 29, 2024 • edited Loading

saimedhi commented Apr 30, 2024

codecov bot commented Feb 6, 2024 •

edited

Loading

saimedhi commented Mar 26, 2024 •

edited

Loading

AbitraryYu commented Apr 2, 2024 •

edited

Loading

AbitraryYu commented Apr 2, 2024 •

edited

Loading

dblock left a comment •

edited

Loading

AbitraryYu commented Apr 20, 2024 •

edited

Loading

AbitraryYu commented Apr 28, 2024 •

edited

Loading

saimedhi commented Apr 29, 2024 •

edited

Loading