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 mlflow model server #774

Merged
merged 14 commits into from
Aug 12, 2019
Merged

Conversation

axsaucedo
Copy link
Contributor

@axsaucedo axsaucedo commented Aug 9, 2019

This PR contains an MLFlow model server. Key things that needs to be confirmed before landing include:

  • MLFlow Model Server Created
  • REST works as expected
  • GRPC works as expected
  • Documentation added
  • Add samples
  • Add functionality to support from operator

Current issues

  • Attempted running with a public google bucket, but getting AUTH permission errors
  • GRPC seems to build as expected but when sending requests it doesn't seem to process

@axsaucedo
Copy link
Contributor Author

Using Seldon storage instead of MLFLow storage as MLFLow storage doesn't support anonymous access to google buckets.

@axsaucedo
Copy link
Contributor Author

Documentation has been added

@seldondev seldondev added size/XXL and removed size/XL labels Aug 9, 2019
@axsaucedo
Copy link
Contributor Author

Samples have been added (also to the google bucket), but can only be tested properly until operator changes are added.

@axsaucedo
Copy link
Contributor Author

Operator PR has been added via SeldonIO/seldon-operator#49 - waiting for that PR to be landed, and for the GRPC in this PR to be tested in order to land

doc/source/servers/mlflow.md Outdated Show resolved Hide resolved
proto/seldon_deployment.proto Outdated Show resolved Hide resolved
@axsaucedo
Copy link
Contributor Author

axsaucedo commented Aug 10, 2019

Added a full example using the MLFlow server which aligns to the blog post currently being written: https://github.com/axsaucedo/seldon-core/tree/mlflow_model_server/examples/models/mlflow_server_ab_test_ambassador. Looks so clean!

@ukclivecox ukclivecox changed the title WIP: Added mlflow model server Added mlflow model server Aug 12, 2019
@ukclivecox ukclivecox merged commit 8898f5a into SeldonIO:master Aug 12, 2019
# df = pd.DataFrame(data=X, columns=feature_names)
#else:
# df = pd.DataFrame(data=X)
result = self._model.predict(X)
Copy link

Choose a reason for hiding this comment

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

Is this contracting to the statement that

* The input to the model is set to be pandas by default, so the numpy array passed will be converted into a pandas dataframe

in doc/source/servers/mlflow.md?

Copy link

Choose a reason for hiding this comment

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

The results should also be converted back to numpy array.

# from the actual python wrapper. Raise exception instead
#raise requests.HTTPError("Model not loaded yet")
#if not feature_names is None and len(feature_names)>0:
# df = pd.DataFrame(data=X, columns=feature_names)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this pandas conversion was commented out?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cliveseldon is there any other to send a dataframe into a served MLFlow model? because this sort of breaks their expected API. maybe i'm missing something though..

@axsaucedo
Copy link
Contributor Author

@phsiao @lennon310 the image that was pushed to DockerHub doesn't have the Pandas Dataframe commented out so it would still work as expected in the documentation, which is why I must have missed it - thanks for the heads up! We'll submit a PR right away 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants