Fixes #150 (lightgbm converters), tests converted models with onnxruntime, moves lightgbm in a separate folder #144
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…on to be None" This reverts commit e257ca1.
… the original model
|
|
xadupre
changed the title
xadupre
changed the title
wenbingl
reviewed
Oct 26, 2018
| @@ -80,6 +80,14 @@ def add_output(self, variable): | |||
| self._outputs.append(variable) | |||
|
|
|||
|
|
|||
| class SklearnModelContainer(CommonSklearnModelContainer): | |||
There was a problem hiding this comment.
If there is no any difference between sklearn and lightbgm model containter, can we just rename SklearnModelContainer as SklearnAndLightGBMModelContainer or a better name?
There was a problem hiding this comment.
XGBoost will probably follow the same design, that's why I split. I chose CommonSklearnModelContainer because many frameworks follow sklearn API now.
| def convert_lightgbm(model, name=None, initial_types=None, doc_string='', targeted_onnx=onnx.__version__, | ||
| custom_conversion_functions=None, custom_shape_calculators=None): | ||
| if not utils.sklearn_installed(): | ||
| raise RuntimeError('scikit-learn is not installed. Please install lightgbm to use this feature.') |
There was a problem hiding this comment.
still call it sklearn_installed?
| @@ -10,7 +10,7 @@ | |||
| from collections import Counter | |||
| from lightgbm import LGBMClassifier, LGBMRegressor | |||
| from ...common._registration import register_converter | |||
| from .TreeEnsemble import _get_default_tree_classifier_attribute_pairs | |||
| from ...sklearn.operator_converters.TreeEnsemble import _get_default_tree_classifier_attribute_pairs | |||
There was a problem hiding this comment.
move it in the common folder to avoid cross reference among converters.
And remove _ from the beginning of the function name.
wenbingl
reviewed
Oct 26, 2018
| @@ -1,7 +1,6 @@ | |||
| dist: xenial | |||
| language: python | |||
| env: | |||
| - ONNX_VERSION="==1.1.2" ONNX_ML=1 | |||
There was a problem hiding this comment.
This backward combability is required. Is there any thing blocked this version?
There was a problem hiding this comment.
No, I just wanted to have faster CI. I'll add it back.
There was a problem hiding this comment.
haven't seen it was added back yet, or do I miss something?
There was a problem hiding this comment.
It just wanted to keep the test short:
python 3.5 - tests against onnx==1.2.3
python 3.6 - tests against onnx==1.3.0
python3.7 - test against onnx (latest version)
It is specified below in the file.
There was a problem hiding this comment.
I mean ONNX_VERSION == 1.1.2 need be tested as well since there is backward compatible requirement for this version.
… of onnxmlruntime (but not the dev version)
wenbingl
pushed a commit
to wenbingl/onnxmltools
that referenced
this pull request
Nov 14, 2018
…xruntime, moves lightgbm in a separate folder (onnx#144) * remove unnecessary print, add quote around filenames in some places * replaces as_matrix by values (pandas warnings) * changes variable name to avoid getting warnings about invalid names * better consistency for converted, allows targetted onnx version to be None * Revert "better consistency for converted, allows targetted onnx version to be None" This reverts commit e257ca1. * handle the comparison of ONNX versions in only one place * fix bug with OneHotEncoder and scikit-learn 0.20 * release the constraint on scikit-learn (0.20.0 allowed) * fix one type issue for Python 2.7 * add documentation to compare_strict_version * implements Keras conversion for mobilenet * first unit test checking the onnx models produces the same outputs as the original model * disable tests with onnxruntime on python 2.7 as onnxruntime is not available * fix travis setup * extends the list of models tested with a runtime * fix travis.yml * fix travis.yml * switch to ubuntu 16.04 * do not store err, out on files (travis.yml) * update travis * Revert "Merge branch 'mobilenet' of https://github.com/xadupre/onnxmltools into testrt" This reverts commit 35059d7, reversing changes made to 2625051. * skip unit test with relu6 if not available * fix missing import * update travis * update travis * disable one text runtime test * show capture in travis * show runtime status as warning * fix warning message * fix conversion for logisticregression and multiclass==2 * complete unit tests for linear models * use conda to install tensorflow * test linear regressors with onnxruntime * fixes converters, test against onnxruntime * disable two tests for the runtime * documentation * better handling of exceptions * update error message * update for Berunoulli and Multinomial * disable runtime test for NaiveBayes, update OneHotEncoder with scikit-learn 0.22 * add test for pipeline * Fix issue for python 2.7 (use of from for exception) * add test for SVR, SVC * add test for coremltools * add first runtime test on keras * fix missing import * One test for model tested with the runtime * fix misspelling * Disable runtime test for version 0.1.3 * Fix misspelling * Move lightgbm converters in a separate folders, fix lightgbm converters * Refactoring, split unit test into multiple ones * add function lightgbm_installed * finalize refactoring about lightgbm * refactor tests with keras * add instructions to test a backend * Fix import issue in coremltools * Call onnxruntime in the same series process not in a second call. * fixes travis build * Enables complex conditions for test failure * Fix python 2.7, disable one test not running with the current version of onnxmlruntime (but not the dev version) * fix one function, enable disabled part * checks that backend is enabled * Disables unit test for some version of onnxruntime * fixes for python 3.6 and onnx==1.2.1 * disable runtime test on onnx==1.2.1 * Update travis.yml with different onnx versions * Update .travis.yml
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PR adds functions in the current series of unit tests to dump data, prédictions, models and converted models (pickle). Another test series retrieves all dumped files, unpickles them, runs the prediction with onnxruntime and compares the predictions against the expected outputs.
It is done only for one model. Adding more later if this design is approved.