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

Replace MLModel(overwrite) with es_if_exists #249

Merged
merged 5 commits into from
Aug 17, 2020

Conversation

V1NAY8
Copy link
Contributor

@V1NAY8 V1NAY8 commented Aug 10, 2020

Closes #225

  • Added es_if_exists in ImportedMLModel and deprecated the overwrite parameter.
  • Updated the tests of ImportedMLModel.
  • Updated Contributing guidelines for installation instructions using Docker for helping new contributors.
  • Updated info_es() to es_info() (because info_es() is deprecated)

@elasticmachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

Thanks for this, there's a decent amount of work to do before we can merge this.

The parameter needs to have different behavior than overwrite. The behavior should match for pandas_to_eland() where you can pass either "fail" to raise an error if an ML model with that ID exists (use ml.get_trained_models() with include_model_definition=False and if a successful response occurs then we need to raise an exception.
The other mode is replace which calls ml.delete_trained_model.

We also need tests for all of this behavior added in the PR.

eland/ml/imported_ml_model.py Outdated Show resolved Hide resolved
eland/ml/imported_ml_model.py Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
eland/ml/imported_ml_model.py Outdated Show resolved Hide resolved
eland/ml/imported_ml_model.py Outdated Show resolved Hide resolved
@sethmlarson
Copy link
Contributor

Jenkins test this please

@V1NAY8
Copy link
Contributor Author

V1NAY8 commented Aug 17, 2020

Jenkins test this please

1 similar comment
@sethmlarson
Copy link
Contributor

Jenkins test this please

@sethmlarson
Copy link
Contributor

It looks like Mypy is failing, could you try running nox -s lint locally:

From Jenkins:

07:23:16 eland/ml/imported_ml_model.py:165: error: Incompatible default for argument "es_if_exists" (default has type "None", argument has type "bool")
07:23:16 eland/ml/imported_ml_model.py:198: error: Non-overlapping container check (element type: "bool", container item type: "str")
07:23:16 eland/ml/imported_ml_model.py:200: error: Non-overlapping equality check (left operand type: "bool", right operand type: "Literal['fail']")
07:23:16 eland/ml/imported_ml_model.py:205: error: Non-overlapping equality check (left operand type: "bool", right operand type: "Literal['replace']").

@sethmlarson
Copy link
Contributor

jenkins test this please

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

This PR looks good to me, thank you for fixing everything up per my review comments 😍
I'll mention you in the changelog for the upcoming release. I hope to see future contributions from you :D

In the future don't be afraid to create multiple PRs if something is still an improvement but is unrelated to the current PR, makes it easier for me to review and merge :)

@sethmlarson sethmlarson merged commit 66b24f9 into elastic:master Aug 17, 2020
@V1NAY8
Copy link
Contributor Author

V1NAY8 commented Aug 17, 2020

Thanks @sethmlarson for the inputs and help. Will Continue to contribute 😃

@V1NAY8 V1NAY8 deleted the issue/225 branch August 17, 2020 17:13
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.

Replace MLModel(overwrite) with es_if_exists
3 participants