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

Added lightgbm integration #544

Merged
merged 11 commits into from Apr 27, 2022
Merged

Added lightgbm integration #544

merged 11 commits into from Apr 27, 2022

Conversation

htahir1
Copy link
Contributor

@htahir1 htahir1 commented Apr 26, 2022

Describe changes

I added lightgbm integration as i thought it was a quick win after xgboost. Its basically the same with some caveats as #538!

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • If my change requires a change to docs, I have updated the documentation accordingly.
  • If I have added an integration, I have updated the integrations table.
  • I have added tests to cover my changes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (add details above)

@github-actions github-actions bot added internal To filter out internal PRs and issues enhancement New feature or request labels Apr 26, 2022
Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

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

Just some tiny things here FWIW.

examples/lightgbm/README.md Outdated Show resolved Hide resolved
examples/lightgbm/README.md Outdated Show resolved Hide resolved
examples/lightgbm/README.md Show resolved Hide resolved
Copy link
Collaborator

@schustmi schustmi left a comment

Choose a reason for hiding this comment

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

Just a few typos, apart from that it look good to merge from my side 👍

@@ -41,6 +41,7 @@ These are the third-party integrations that ZenML currently supports:
| Hugging Face | ✅ | Materializer | Use Hugging Face tokenizers, datasets and models. | [huggingface](https://github.com/zenml-io/zenml/tree/main/examples/huggingface) |
| KServe | ⛏ | Inference | Looking for community implementors. | |
| Kubeflow | ✅ | Orchestrator | Either full Kubeflow or Kubeflow Pipelines. Works for local environments currently. | [kubeflow](https://github.com/zenml-io/zenml/tree/main/examples/kubeflow) |
| lightgbm | ✅ | Training | Support for `Booster` and `Dataset` materialization. | [xgboost](https://github.com/zenml-io/zenml/tree/main/examples/lightgbm) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

still has some xgboost references in there

examples/lightgbm/README.md Outdated Show resolved Hide resolved

# Make a temporary phantom artifact
with tempfile.NamedTemporaryFile(
mode="w", suffix=".json", delete=True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that it will break anything, but I'm a little confused why the suffix is .json here and the default filename is model.txt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its a copy pasta! Fixed now!

htahir1 and others added 5 commits April 27, 2022 09:27
Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com>
Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com>
Co-authored-by: Michael Schuster <schustmi@users.noreply.github.com>
@htahir1
Copy link
Contributor Author

htahir1 commented Apr 27, 2022

@strickvl @schustmi Fixed both your review comments now. Thank you!

Copy link
Collaborator

@AlexejPenner AlexejPenner left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Collaborator

@schustmi schustmi left a comment

Choose a reason for hiding this comment

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

LGTM

@htahir1 htahir1 merged commit d407683 into develop Apr 27, 2022
@htahir1 htahir1 deleted the feature/add-lightgmb branch April 27, 2022 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internal To filter out internal PRs and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants