Skip to content
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

Implement predict method and add unit tests #425

Merged
merged 7 commits into from
Dec 12, 2024

Conversation

Yerzhaisang
Copy link
Contributor

Description

  • Implemented the predict method in MLCommonClient.
  • Added unit tests to ensure the correctness of the predict method's behavior.
  • Integrated the method with the existing system and ensured it handles the parameters (text_docs, return_number, target_response) as expected.

Issues Resolved

#287

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: Yerzhaisang Taskali <tasqali1697@gmail.com>
Signed-off-by: Yerzhaisang Taskali <tasqali1697@gmail.com>
Signed-off-by: Yerzhaisang Taskali <tasqali1697@gmail.com>
@Yerzhaisang Yerzhaisang changed the title Implement predict method and add unit tests Implement predict method and added unit tests Nov 6, 2024
Signed-off-by: Yerzhaisang Taskali <tasqali1697@gmail.com>
@Yerzhaisang Yerzhaisang changed the title Implement predict method and added unit tests Implement predict method and add unit tests Nov 6, 2024
@@ -568,6 +568,29 @@ def unload_model(self, model_id: str, node_ids: List[str] = []) -> object:
body=API_BODY,
)

def predict(self, algorithm_name: str, model_id: str, predict_object: dict) -> dict:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a method generate_model_inference, which is in fact the predict functionality. We can just invoke this method here?

We could change the method name generate_model_inference to predict, but for backward compatibility keep the method as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

generate_model_inference and predict methods are not the same because:

  1. URLs are not the same
  2. generate_model_inference doesn't use algorithm_name

Copy link
Collaborator

Choose a reason for hiding this comment

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

For model prediction, we don't need algorithm_name. code ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you please take a look at the issue description?

Copy link
Contributor Author

@Yerzhaisang Yerzhaisang Nov 26, 2024

Choose a reason for hiding this comment

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

I see, @dhrubo-os thank you for the feedback!

Copy link
Collaborator

Choose a reason for hiding this comment

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

So aren't we going to remove the algorithm_name here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. I think it's better to keep this as a optional parameter for the BWC as before we needed to have algorithm_name for model prediction.

@Yerzhaisang currently algorithm_name isn't optional here. Can we do something like def predict(self, model_id: str, predict_object: dict, algorithm_name: str = None) -> dict: ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Yerzhaisang Taskali <tasqali1697@gmail.com>
Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.94%. Comparing base (529ee34) to head (eeffa36).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
opensearch_py_ml/ml_commons/ml_commons_client.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
- Coverage   91.53%   89.94%   -1.59%     
==========================================
  Files          42       43       +1     
  Lines        4395     4556     +161     
==========================================
+ Hits         4023     4098      +75     
- Misses        372      458      +86     

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

@dhrubo-os
Copy link
Collaborator

codecov/patch — 33.33% of diff hit (target 91.53%)

@Yerzhaisang could you please take a look?

Signed-off-by: Yerzhaisang Taskali <tasqali1697@gmail.com>
@Yerzhaisang
Copy link
Contributor Author

@dhrubo-os it's ready for overview

Signed-off-by: Yerzhaisang Taskali <tasqali1697@gmail.com>
@dhrubo-os dhrubo-os merged commit 155bdc3 into opensearch-project:main Dec 12, 2024
12 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