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

[ML] adds support for transforming LGBMRegressor models #247

Merged
merged 5 commits into from
Aug 11, 2020
Merged

[ML] adds support for transforming LGBMRegressor models #247

merged 5 commits into from
Aug 11, 2020

Conversation

benwtrent
Copy link
Member

This adds support for lightgbm LGBMRegressor models.

Verified that all the supported objective functions and various booster types work great.

relates to #240

@benwtrent benwtrent added enhancement New feature or request topic:NLP Issue or PR about NLP model support and eland_import_hub_model labels Aug 6, 2020
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.

We'll need a few changes to noxfile.py:

  • Add the files that are typed to the TYPED_FILES collection
  • Add a section for testing with and without lightgbm in the test_ml_deps job

And one change in setup.py:

  • Add the lightgbm extra so we can version constrain in the future if needed

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.

Handful of nits but now this is ready for a technical review :D Thanks much!

Also, I noticed the version pin to 2.3.X, it looks like a release candidate for v3 got published literally minutes ago. As a separate item I'll open an issue to test against v3 and see if it's easy for us to support both or if its more work.

eland/ml/imported_ml_model.py Outdated Show resolved Hide resolved
eland/ml/transformers/__init__.py Outdated Show resolved Hide resolved
eland/ml/transformers/lightgbm.py Show resolved Hide resolved
eland/ml/transformers/lightgbm.py Outdated Show resolved Hide resolved
eland/ml/transformers/lightgbm.py Outdated Show resolved Hide resolved
eland/tests/ml/test_imported_ml_model_pytest.py Outdated Show resolved Hide resolved
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.

One tiny comment then this LGTM

if decider == ">=":
return "gte"
raise ValueError(
"Unsupported splitting decider: %s. Only <=, " "<, >=, and > are allowed."
Copy link
Contributor

Choose a reason for hiding this comment

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

This is two string literals joining together, you can change it to one. (see "...<=, " "<,...")

Copy link

@tveasey tveasey left a comment

Choose a reason for hiding this comment

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

Overall looks good. I haven't checked the format for LGBM, but the test passing convinces me you're converting correctly :). On that front, I think we probably ought to have a test for a classifier as well.

eland/ml/imported_ml_model.py Show resolved Hide resolved
eland/ml/imported_ml_model.py Outdated Show resolved Hide resolved
eland/ml/transformers/lightgbm.py Show resolved Hide resolved
eland/ml/transformers/lightgbm.py Show resolved Hide resolved
Copy link

@tveasey tveasey left a comment

Choose a reason for hiding this comment

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

LGTM

@sethmlarson sethmlarson merged commit 6ee282e into elastic:master Aug 11, 2020
@sethmlarson
Copy link
Contributor

Thanks @benwtrent and @tveasey!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request topic:NLP Issue or PR about NLP model support and eland_import_hub_model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants