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

Update sklearn example #57

Merged
merged 9 commits into from
Dec 8, 2020
Merged

Update sklearn example #57

merged 9 commits into from
Dec 8, 2020

Conversation

danielfrg
Copy link
Contributor

Updating the scikit-learn sample app:

  • Updated the workflow to the new UI, forms and stuff
  • Added instructions on the environment selection
  • Updated the dependencies so it works with the python 3.7 environment
  • Changed the serialisation format to joblib, this is the recommended by scikit-learn
  • Added two query options, real time predict and batch based on a file

@danielfrg danielfrg requested a review from a team as a code owner December 7, 2020 21:27
@koverholt
Copy link
Contributor

Thanks, LGTM!

In the current/older version with the pickle file, I ended up pinning numpy and scipy in the requirements.txt for maximum reproducibility. Do you see a need to do that here as well?

Also, thanks for including the specific language and environment to use!

@danielfrg
Copy link
Contributor Author

Yeah, I think that makes sense, I will pin numpy. Is there a reason you included scipy?

@koverholt
Copy link
Contributor

koverholt commented Dec 7, 2020

I include numpy and scipy both since they are deps of sklearn. In theory pinning only sklearn would handle this, but you know how finicky these things can be.

For example, in the current version this works:

algorithmia>=1.0.0,<2.0
numpy==1.11.3
scipy==1.2.1
scikit-learn==0.17.1

but if you remove scipy, it will attempt to install the latest version of scipy and things won't work.

@danielfrg
Copy link
Contributor Author

Thanks for the review! added scipy pin and fixes your suggestions

Copy link
Contributor

@bnlb bnlb left a comment

Choose a reason for hiding this comment

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

lgtm!

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.

3 participants