adding executeAPI #165

AlibiZhenis · 2023-05-18T05:56:55Z

Issues Resolved

Closes #164

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
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.

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

AlibiZhenis · 2023-05-18T05:59:40Z

The tests will fail until PR #159 gets merged.

AlibiZhenis · 2023-05-26T06:40:51Z

@dhrubo-os Can you send the criteria for validating the json?

dhrubo-os · 2023-05-26T17:49:38Z

import json

def is_json(myjson):
  try:
    json.loads(myjson)
  except ValueError as e:
    return False
  return True

I'm talking about something simple like this.

dhrubo-os · 2023-05-26T17:51:21Z

And for the simple calculator what I was instructing to use is here:

POST _plugins/_ml/_execute/local_sample_calculator
{
    "operation": "max",
    "input_data": [1.0, 2.0, 3.0]
}

# Output
{
    "sample_result": 3.0
}

ref: https://github.com/opensearch-project/ml-commons/blob/main/docs/how-to-add-new-function.md#step-6-run-and-test-1

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

dhrubo-os · 2023-05-30T20:29:51Z

Can we please check why the integration test is failing here?

                 try:
>                   self.fp = io.open(file, filemode)
E                   FileNotFoundError: [Errno 2] No such file or directory: '/code/opensearch-py-ml/tests/ml_commons/../../../../../synthetic_queries.zip'

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

AlibiZhenis · 2023-06-01T11:25:30Z

This PR closed for some reason

dhrubo-os · 2023-05-30T20:28:12Z

opensearch_py_ml/ml_commons/model_execute.py

+        :rtype: dict
+        """
+
+        if not isinstance(input_json, dict):


import json def is_valid_json(input_string): try: json.loads(input_string) return True except ValueError: return False

Can't we do something like this?

It will return false if the input is dictionary

Is that okay?

you mean if the input is {"a":"abc"} then it will fail?

Yeah, I think so. json.loads accepts only string or bytearray

I checked using this function:

Provided input string is valid: {"a":"abc"} Provided input string is valid: {"operation": "max", "input_data": [1.0, 2.0, 3.0]} Provided input string is not valid: zaa Provided input string is valid: {"index_name": "rca-index","attribute_field_names": ["attribute"],"aggregations": [{"sum": {"sum": {"field": "value"}}}],"time_field_name": "timestamp","start_time": 1620630000000,"end_time": 1621234800000,"min_time_interval": 86400000,"num_outputs": 10}

The code snippet you provided (if not isinstance(input_json, dict):) does not directly check whether a given input is a valid JSON string or not. The isinstance() function in Python checks if an object is an instance of a particular class or data type. In this case, it checks if the input_json variable is not an instance of a dictionary (dict).

While a valid JSON string can be represented as a dictionary object in Python, this check alone does not guarantee that the input is a valid JSON string. It only verifies that the input_json is not a dictionary.

Hmm, I will double check whether it accepts dictionaries.

If the input is not an instance of a dictionary, json.loads will be called. Then, if it’s not a deserializable json, it will raise an exception. Otherwise, it returns a dictionary that is used in the query.

dhrubo-os · 2023-06-02T16:42:28Z

Also can you please check why the integration test is failing here?

dhrubo-os · 2023-06-02T16:43:38Z

I'll prioritize this commit. Let's close out this commit asap if possible. Thanks

AlibiZhenis · 2023-06-04T08:17:38Z

The problem is with the TESTDATA_SYNTHETIC_QUERY_ZIP constant path string.
TESTDATA_SYNTHETIC_QUERY_ZIP = os.path.join( PROJECT_DIR, "../../../../..", "synthetic_queries.zip" )
I don't know why it started failing all of a sudden. Can you explain why there is a "../../../../.." argument here?

AlibiZhenis · 2023-06-04T08:18:40Z

FileNotFoundError: [Errno 2] No such file or directory: '/code/opensearch-py-ml/tests/ml_commons/../../../../../synthetic_queries.zip'

It just concatenates the double dots as the other 2 arguments.

dhrubo-os · 2023-06-04T18:49:16Z

tests/ml_commons/test_ml_commons_client.py

@@ -33,7 +33,9 @@
 MODEL_CONFIG_FILE_NAME = "ml-commons_model_config.json"

 TEST_FOLDER = os.path.join(PROJECT_DIR, "test_model_files")
-TESTDATA_SYNTHETIC_QUERY_ZIP = os.path.join(PROJECT_DIR, "..", "synthetic_queries.zip")
+TESTDATA_SYNTHETIC_QUERY_ZIP = os.path.join(


Looks like you are changing the path here? Can we revert this to pass the integration test?

Oh, I don't remember changing it. Will fix.

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

codecov · 2023-06-05T05:46:04Z

Codecov Report

Merging #165 (0d0300b) into main (3d4ff7f) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 0d0300b differs from pull request most recent head 3f78672. Consider uploading reports for the commit 3f78672 to get more accurate results

@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
+ Coverage   90.76%   90.79%   +0.03%     
==========================================
  Files          36       37       +1     
  Lines        4027     4042      +15     
==========================================
+ Hits         3655     3670      +15     
  Misses        372      372

Impacted Files	Coverage Δ
opensearch_py_ml/ml_commons/ml_commons_client.py	`80.00% <100.00%> (+0.66%)`	⬆️
opensearch_py_ml/ml_commons/model_execute.py	`100.00% <100.00%> (ø)`

dhrubo-os · 2023-06-06T00:35:20Z

opensearch_py_ml/ml_commons/model_execute.py

+    def __init__(self, os_client: OpenSearch):
+        self._client = os_client
+
+    def _execute(self, algorithm_name: str, input_json: dict) -> dict:


first of all I think we should expect a string for input_json not a dict.

Are you sure? Wouldn’t that be inconvenient for the user?

So, if you can see in the post requests customers are used to send these values as string into the api. I believe this will keep the experience same.

For dict, customer eventually need to build the dict.

May be we can expect both (string and dict) if you think that can improve customer experience?

Right now, it accepts both dict and string. Should I change anything?

dhrubo-os · 2023-06-09T17:05:34Z

tests/ml_commons/test_ml_commons_client.py

@@ -72,6 +72,18 @@ def test_init():
    assert type(ml_client._model_uploader) == ModelUploader


+def test_execute():


Can you please add another test to show it works with string also?

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

dhrubo-os · 2023-06-12T04:26:28Z

tests/ml_commons/test_ml_commons_client.py

+
+    raised = False
+    try:
+        input_json = '{"operation": "max", "input_data": [1.0, 2.0, 3.0]}'


I just realized, we should get result from this input. In the try block can we also match value? In both cases.

Also can we try to cover the catch condition. I'm planning to add start a initiative to start covering for the except cases. This can be good start. Pls lmk if you have any question.

Can you elaborate more on the covering except part? What do you men by cover?

Nevermind, ignore my last comment. Approving the PR.

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

* adding executeAPI Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * updating documentation Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * changing code and test Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * changing code Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * fixing tests Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * changing tests Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * checking output value in integration tests Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> --------- Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> (cherry picked from commit 8eb26eb)

* adding executeAPI Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * updating documentation Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * changing code and test Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * changing code Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * fixing tests Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * changing tests Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> * checking output value in integration tests Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> --------- Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com> (cherry picked from commit 8eb26eb) Co-authored-by: Alibi Zhenis <92104549+AlibiZhenis@users.noreply.github.com>

adding executeAPI

817f98e

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

AlibiZhenis requested review from dhrubo-os, greaa-aws and ylwu-amzn as code owners May 18, 2023 05:56

updating documentation

9ce6338

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

AlibiZhenis added 2 commits May 28, 2023 14:36

changing code and test

ec6f1ea

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

changing code

25cbdaa

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

AlibiZhenis closed this Jun 1, 2023

AlibiZhenis force-pushed the ExecuteAPI branch 2 times, most recently from 25cbdaa to 7d01104 Compare June 1, 2023 11:19

resolving conflict

39da2b1

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

AlibiZhenis reopened this Jun 1, 2023

dhrubo-os reviewed Jun 2, 2023

View reviewed changes

dhrubo-os reviewed Jun 4, 2023

View reviewed changes

AlibiZhenis and others added 2 commits June 5, 2023 13:23

fixing tests

d05fda7

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

Merge branch 'opensearch-project:main' into ExecuteAPI

0102633

dhrubo-os reviewed Jun 6, 2023

View reviewed changes

dhrubo-os reviewed Jun 9, 2023

View reviewed changes

changing tests

0d0300b

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

dhrubo-os reviewed Jun 12, 2023

View reviewed changes

checking output value in integration tests

3f78672

Signed-off-by: Alibi Zhenis <alibizhenis4@gmail.com>

dhrubo-os approved these changes Jun 13, 2023

View reviewed changes

greaa-aws approved these changes Jun 14, 2023

View reviewed changes

dhrubo-os merged commit 8eb26eb into opensearch-project:main Jun 14, 2023

dhrubo-os added the backport 1.x label Jun 20, 2023

opensearch-trigger-bot bot mentioned this pull request Jun 20, 2023

[Backport 1.x] adding executeAPI #179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding executeAPI #165

adding executeAPI #165

AlibiZhenis commented May 18, 2023 •

edited

Loading

AlibiZhenis commented May 18, 2023

AlibiZhenis commented May 26, 2023

dhrubo-os commented May 26, 2023

dhrubo-os commented May 26, 2023

dhrubo-os commented May 30, 2023

AlibiZhenis commented Jun 1, 2023

dhrubo-os May 30, 2023

AlibiZhenis Jun 4, 2023

AlibiZhenis Jun 4, 2023

dhrubo-os Jun 4, 2023

AlibiZhenis Jun 5, 2023

dhrubo-os Jun 5, 2023

dhrubo-os Jun 5, 2023

AlibiZhenis Jun 6, 2023

AlibiZhenis Jun 6, 2023

dhrubo-os commented Jun 2, 2023

dhrubo-os commented Jun 2, 2023

AlibiZhenis commented Jun 4, 2023

AlibiZhenis commented Jun 4, 2023

dhrubo-os Jun 4, 2023 •

edited

Loading

AlibiZhenis Jun 5, 2023

codecov bot commented Jun 5, 2023 •

edited

Loading

dhrubo-os Jun 6, 2023

AlibiZhenis Jun 6, 2023

dhrubo-os Jun 6, 2023

AlibiZhenis Jun 9, 2023

dhrubo-os Jun 9, 2023

dhrubo-os Jun 12, 2023

dhrubo-os Jun 12, 2023

AlibiZhenis Jun 13, 2023

dhrubo-os Jun 13, 2023

		@@ -72,6 +72,18 @@ def test_init():
		assert type(ml_client._model_uploader) == ModelUploader


		def test_execute():

adding executeAPI #165

adding executeAPI #165

Conversation

AlibiZhenis commented May 18, 2023 • edited Loading

Issues Resolved

Check List

AlibiZhenis commented May 18, 2023

AlibiZhenis commented May 26, 2023

dhrubo-os commented May 26, 2023

dhrubo-os commented May 26, 2023

dhrubo-os commented May 30, 2023

AlibiZhenis commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhrubo-os commented Jun 2, 2023

dhrubo-os commented Jun 2, 2023

AlibiZhenis commented Jun 4, 2023

AlibiZhenis commented Jun 4, 2023

dhrubo-os Jun 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 5, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlibiZhenis commented May 18, 2023 •

edited

Loading

dhrubo-os Jun 4, 2023 •

edited

Loading

codecov bot commented Jun 5, 2023 •

edited

Loading