-
Notifications
You must be signed in to change notification settings - Fork 61
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
adding executeAPI #165
adding executeAPI #165
Changes from 7 commits
817f98e
9ce6338
ec6f1ea
25cbdaa
39da2b1
d05fda7
0102633
0d0300b
3f78672
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
Execute | ||
================== | ||
|
||
.. currentmodule:: opensearch_py_ml | ||
|
||
.. autofunction:: opensearch_py_ml.ml_commons.MLCommonClient.execute |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# The OpenSearch Contributors require contributions made to | ||
# this file be licensed under the Apache-2.0 license or a | ||
# compatible open source license. | ||
# Any modifications Copyright OpenSearch Contributors. See | ||
# GitHub history for details. | ||
|
||
import json | ||
|
||
from opensearchpy import OpenSearch | ||
|
||
from opensearch_py_ml.ml_commons.ml_common_utils import ML_BASE_URI | ||
|
||
|
||
class ModelExecute: | ||
""" | ||
Class for executing algorithms using ML Commons execute API. | ||
""" | ||
|
||
API_ENDPOINT = "models/_execute" | ||
|
||
def __init__(self, os_client: OpenSearch): | ||
self._client = os_client | ||
|
||
def _execute(self, algorithm_name: str, input_json: dict) -> dict: | ||
""" | ||
This method executes ML algorithms that can be only executed directly (i.e. do not support train and | ||
predict APIs), like anomaly localization and metrics correlation. | ||
The input json must be a dictionary or a deserializable Python object. | ||
The algorithm has to be supported by ML Commons. | ||
Refer to https://opensearch.org/docs/2.7/ml-commons-plugin/api/#execute | ||
|
||
:param algorithm_name: Name of the algorithm | ||
:type algorithm_name: string | ||
:param input_json: Dictionary of parameters or a deserializable string, byte, or bytearray | ||
:type input_json: dict | ||
:return: returns the API response | ||
:rtype: dict | ||
""" | ||
|
||
if not isinstance(input_json, dict): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can't we do something like this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will return false if the input is dictionary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that okay? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you mean if the input is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think so. json.loads accepts only string or bytearray There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked using this function:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code snippet you provided ( 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I will double check whether it accepts dictionaries. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
input_json = json.loads(input_json) | ||
|
||
return self._client.transport.perform_request( | ||
method="POST", | ||
url=f"{ML_BASE_URI}/_execute/{algorithm_name}", | ||
body=input_json, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,18 @@ def test_init(): | |
assert type(ml_client._model_uploader) == ModelUploader | ||
|
||
|
||
def test_execute(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add another test to show it works with string also? |
||
raised = False | ||
try: | ||
input_json = {"operation": "max", "input_data": [1.0, 2.0, 3.0]} | ||
ml_client.execute( | ||
algorithm_name="local_sample_calculator", input_json=input_json | ||
) | ||
except: # noqa: E722 | ||
raised = True | ||
assert raised == False, "Raised Exception during execute API testing" | ||
|
||
|
||
def test_DEPRECATED_integration_pretrained_model_upload_unload_delete(): | ||
raised = False | ||
try: | ||
|
@@ -381,3 +393,6 @@ def test_integration_model_train_register_full_cycle(): | |
except: # noqa: E722 | ||
raised = True | ||
assert raised == False, "Raised Exception in deleting model" | ||
|
||
|
||
test_integration_model_train_register_full_cycle() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first of all I think we should expect a string for
input_json
not a dict.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? Wouldn’t that be inconvenient for the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, it accepts both dict and string. Should I change anything?