-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add host commandline argument for UI and sklearn server #27
Conversation
Thanks for the contribution! This looks great. Would you mind adding a PR description to give some context about why this change is made, including the steps you took to verify that this works as expected? |
mlflow/cli.py
Outdated
@@ -124,12 +124,14 @@ def run(uri, entry_point, version, param_list, experiment_id, mode, cluster_spec | |||
@click.option("--file-store-path", default=None, | |||
help="The root of the backing file store for experiment and run data. Defaults to %s." | |||
% file_store._default_root_dir()) | |||
def ui(file_store_path): | |||
@click.option("--host", default="127.0.0.1", | |||
help="The networking interface on which the UI server listens. Defaults to 127.0.0.1. Use 0.0.0.0 for docker.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this line (and the associated one) has a linter error due to line length. In general you can run bash lint.sh
locally to run the linter.
Maybe we could change the help a bit while we're at it (mostly to clarify the implication of the default and of setting 0.0.0.0):
@click.option("--host", default="127.0.0.1",
help="The IP address which the UI server listens."
" Defaults to 127.0.0.1, allowing only local connections."
" Use 0.0.0.0 to bind to all addresses.")
mlflow/sklearn.py
Outdated
@@ -76,7 +76,9 @@ def commands(): | |||
@click.argument("model_path") | |||
@click.option("--run_id", "-r", metavar="RUN_ID", help="Run ID to look for the model in.") | |||
@click.option("--port", "-p", default=5000, help="Server port. [default: 5000]") | |||
def serve_model(model_path, run_id=None, port=None): | |||
@click.option("--host", default="127.0.0.1", | |||
help="The networking interface on which the UI server listens. Defaults to 127.0.0.1. Use 0.0.0.0 for docker.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe here instead of "UI server" we can say "prediction server".
@aarondav I added a PR description and addressed your help text (and linting) comments. |
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
=========================================
+ Coverage 69.6% 69.71% +0.1%
=========================================
Files 44 44
Lines 2527 2536 +9
=========================================
+ Hits 1759 1768 +9
Misses 768 768
Continue to review full report at Codecov.
|
LGTM - merging! |
Support for running R and Python travis tests
* news post on website * note formatting * cran pkg is up
The UI and the model server run on 127.0.0.1 by default, but this doesn't work from within a docker container. This PR introduces a
-host
commandline flag so that you can pass in0.0.0.0
to listen on all interfaces.To verify that this works, you can do the following:
Then from within the container:
And go to http://127.0.0.1:5000 and notice that you don't see the UI. Then from within the container instead do:
and visit http://0.0.0.0:5000
To see the model serving stuff, go into the container and do
and note the model id. Then do
and from a separate terminal try (but fail) to get a prediction:
Instead try