-
Notifications
You must be signed in to change notification settings - Fork 179
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
Add support for H2O GBM MOJO #358
Conversation
This pull request introduces 6 alerts when merging 48f0eec into 8ce90a3 - view on LGTM.com new alerts:
|
This pull request introduces 5 alerts when merging 035f097 into 8ce90a3 - view on LGTM.com new alerts:
|
This pull request introduces 5 alerts when merging 7adb7c4 into 8ce90a3 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 17da084 into 8ce90a3 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 2c5372e into 8ce90a3 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging eead297 into 8ce90a3 - view on LGTM.com new alerts:
|
waiting for next release of H2O for the tests to start passing |
This pull request introduces 2 alerts when merging dc06f9e into 9b34fc4 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging ca77241 into 9b34fc4 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 06b1689 into 9b34fc4 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 8af5395 into 9b34fc4 - view on LGTM.com new alerts:
|
@wenbingl please review |
@@ -94,3 +94,9 @@ def convert_xgboost(*args, **kwargs): | |||
from .xgboost.convert import convert | |||
return convert(*args, **kwargs) | |||
|
|||
def convert_h2o(*args, **kwargs): | |||
if not utils.h2o_installed(): |
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.
Given this method was added, please update the requirements.txt for onnxconverter-common version.
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.
this was not yet released, do you know when the next release is planned?
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.
OK, I'll take care of it.
|
||
@classmethod | ||
def tearDownClass(cls): | ||
h2o.cluster().shutdown() |
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.
Is JRE needed to run these cases?
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.
yes, is that an issue?
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.
No, a double confirm.
name = str(uuid4().hex) | ||
|
||
mojo_str = h2o.print_mojo(model_path, format="json") | ||
mojo_model = json.loads(mojo_str) |
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.
All other converters are converting the model already loaded in memory, Can this new converter follow the same behavior?
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.
unfortunately this is impossible, since h2o core is in java our Python API is just a wrapper over a REST API exposed by the back-end. Furhter more the MOJO model format is meant for final deployment outside of h2o so there is little support for it in python.
the closes thing we could do is to accept the JSON representation of the model (generated by json.loads(h2o.print_mojo(path, "json"))
but that would only make this less user-friendly IMO
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.
The point is the model passed to converter are all in-memory. so it is important that we can keep the consistence among the converter APIs.
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.
I understand you concern. Would it be better if instead of a filesystem path the argument was the zip-file loaded into memory?
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.
Any type of the in-memory object is ok with me.
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.
fixed
A dumb question, is it possible that the H2O Mojo model can be converted to xgboost model? |
Maybe for some cases that could be done, but definitely not for the general case. |
This pull request introduces 2 alerts when merging 06d7af2 into 37e51ab - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 7be8641 into 37e51ab - view on LGTM.com new alerts:
|
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.
Can you fix the LGTM warning as well?
This pull request introduces 2 alerts when merging 4a49efe into 37e51ab - view on LGTM.com new alerts:
|
Fixed one of the LGTM warnings, the other is a common patter used thoughout the code-base I would much rather stick to the code-style of the project. |
This pull request introduces 2 alerts and fixes 1 when merging 1413a71 into 37e51ab - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 1 when merging ef356d1 into 37e51ab - view on LGTM.com new alerts:
fixed alerts:
|
@wenbingl should be ready to merge at your convenience or perhaps a release of onnxconverter-common is still required? |
No description provided.