[#297] Complete Python SDK #299

aarondav · 2018-08-14T16:16:00Z

The goal of this PR is to complete the Python SDK. I split the currently-exposed APIs into two kinds:

mlflow.tracking -- exposes Pythonic versions of the REST API calls via a MLflowService object. This is implemented internally in mlflow.tracking.service.
mlflow -- intended for the "fluent" version of the API, where invoking log_param is a top-level function that will start a run if none exists, and contains a notion of an "active run". This is implemented internally in mlflow.tracking.fluent.

The goal here is that users can use the top-level mlflow API in their simple run code, but if they want to do more complicated or management-level operations (e.g., delete an experiment), they can rely on the Python version of the REST API, effectively.

aarondav · 2018-08-14T16:16:42Z

mlflow/tracking/utils.py

@@ -0,0 +1,123 @@
+from __future__ import print_function


This stuff could also be in api, I just split it out for the sake of code readability.

mateiz · 2018-08-14T19:58:43Z

I think the names "flow" and "api" are kind of weird, though I guess they are just internal names. Maybe a better name for "flow" would be "functions" since it has all the top-level functions, and then "api" could be "objects" and contain all the objects. Then you could have mlflow.tracking expose all the "explicit" functions to create runs, etc and mlflow expose the "implicit" ones that work with a global active run.

aarondav · 2018-08-15T16:57:28Z

Updated based on feedback. Still need to do the rename of the internal files -- was thinking about changing api.py to service.py since that's now what it mainly provides.

aadamson · 2018-08-15T17:22:14Z

mlflow/tracking/flow.py

+
+_EXPERIMENT_ID_ENV_VAR = "MLFLOW_EXPERIMENT_ID"
+_RUN_ID_ENV_VAR = "MLFLOW_RUN_ID"
+_active_run = None


Have you considered making _active_run thread local? I'm imagining a scenario in which a user is running a bunch of different runs simultaneously using joblib or a standard library thread pool and gets unexpectedly bitten when all the threads end up sharing a run.

Makes a lot of sense. Changing the scope of _active_run, or letting users manually create and pass around fluent runs, are both on the table, but I would prefer to leave that to a future PR.

codecov-io · 2018-08-16T01:08:32Z

Codecov Report

Merging #299 into master will increase coverage by 0.13%.
The diff coverage is 87.37%.

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
+ Coverage   55.94%   56.07%   +0.13%     
==========================================
  Files         108      112       +4     
  Lines        5554     5566      +12     
==========================================
+ Hits         3107     3121      +14     
+ Misses       2447     2445       -2

Impacted Files	Coverage Δ
mlflow/h2o.py	`98% <100%> (ø)`	⬆️
mlflow/tensorflow.py	`97.05% <100%> (ø)`	⬆️
mlflow/projects/submitted_run.py	`83.78% <100%> (ø)`	⬆️
mlflow/pyfunc/cli.py	`62.06% <100%> (ø)`	⬆️
mlflow/tracking/__init__.py	`100% <100%> (+17.03%)`	⬆️
mlflow/server/handlers.py	`31.07% <100%> (-0.78%)`	⬇️
mlflow/keras.py	`97.14% <100%> (ø)`	⬆️
mlflow/entities/__init__.py	`100% <100%> (ø)`
mlflow/store/gcs_artifact_repo.py	`87.01% <100%> (ø)`	⬆️
mlflow/azureml/__init__.py	`75.86% <100%> (ø)`	⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f7c63e...9c75759. Read the comment docs.

smurching

Thanks @aarondav, this looks pretty solid - had a few questions about ActiveRun behavior changes but otherwise just small nits :)

Also thanks @aadamson for helping review!

smurching · 2018-08-16T01:53:24Z

mlflow/tracking/flow.py

+    def __enter__(self):
+        pass
+
+    def __exit__(self, exc_type, exc_val, exc_tb):


Could/should we call end_run here? I do like having ActiveRun only mutate / alter its own state, so maybe we don't want to call end_run. If we don't call end_run, we should also unset _RUN_ID_ENV_VAR here as end_run does, so that future runs launched in start_run don't reuse the same run ID here

smurching · 2018-08-16T01:55:25Z

mlflow/tracking/flow.py

+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        status = "FINISHED" if exc_type is None else "FAILED"
+        get_service().set_terminated(self.info.run_uuid, status)


Here I think we might see failures if a user calls mlflow.set_tracking_uri within a with mlflow.start_run(...) block, since set_tracking_uri could change the store we use in get_service (this was why the original ActiveRun kept track of the run's backing store). I do like the shift towards a simpler ActiveRun, maybe the right thing to do is raise an exception if set_tracking_uri is called while a run is active, or just tell users not to call set_tracking_uri within a run.

smurching · 2018-08-16T02:01:32Z

tests/keras/test_keras_model_export.py

@@ -44,42 +44,43 @@ def predicted(model, data):

 def test_model_save_load(model, data, predicted):


nit: here we can use pytest's tmpdir fixture (i.e. just pass tmpdir to the test function and it'll automatically create/cleanup a tempdir for us)

smurching · 2018-08-16T02:02:25Z

tests/keras/test_keras_model_export.py

-        assert all(pyfunc_loaded.predict(x).values == predicted)
+    # Loading pyfunc model
+    pyfunc_loaded = mlflow.pyfunc.load_pyfunc(path)
+    assert all(pyfunc_loaded.predict(x).values == predicted)


 def test_model_log(model, data, predicted):


nit: here we can use the tracking_uri_mock fixture from tests/projects/utils.py, which sets the tracking URI to a tempdir for us

smurching · 2018-08-16T02:02:38Z

tests/keras/test_keras_model_export.py



 def test_model_log(model, data, predicted):
    x, y = data
-    old_uri = tracking.get_tracking_uri()
+    old_uri = mlflow.get_tracking_uri()


nit: here we can use the tracking_uri_mock fixture from tests/projects/utils.py, which sets the tracking URI to a tempdir for us / unsets it afterwards. Although feel free to ignore that suggestion if you think it'll make the diff really big (using tracking_uri_mock will basically unindent a ton of code blocks, we could shift to using it in a separate PR)

smurching · 2018-08-16T02:02:58Z

tests/projects/test_projects.py

@@ -53,6 +53,7 @@ def test_fetch_project(local_git_repo, local_git_repo_uri):
        (os.path.dirname(TEST_PROJECT_DIR), os.path.basename(TEST_PROJECT_DIR), TEST_PROJECT_DIR),
    ]
    for base_uri, subdirectory, expected in test_list:
+        print("HELLO %s" % base_uri)


Nit: maybe change the wording of this print statement?

smurching · 2018-08-16T02:04:28Z

tests/store/test_sftp_artifact_repo.py

@@ -13,6 +13,7 @@ def sftp_mock():
    return MagicMock(autospec=pysftp.Connection)


+@pytest.mark.large


Q: Why the large annotation here? Seems fine to add, but was this test slow beforehand?

It takes about 60 seconds to throw the error.

smurching · 2018-08-16T02:14:56Z

mlflow/tracking/__init__.py

+    "get_service",
+    "get_tracking_uri",
+    "set_tracking_uri",
+    "_get_git_commit",


Q: why expose these internal methods in __all__? Doing so means that from mlflow.tracking import * will import methods like _get_git_commit.

If _get_git_commit etc is used in other modules maybe we could make it a "public" API within mlflow.tracking.flow (e.g. mlflow.tracking.flow.get_git_commit), since mlflow.tracking.flow itself won't be a publicly documented module (assuming I understand correctly). Admittedly this would make our code less robust to future refactors of mlflow.tracking.flow.

mateiz · 2018-08-16T17:21:38Z

mlflow/tracking/fluent.py

@@ -0,0 +1,198 @@
+


It would be great to add a doc comment to each module. I know some don't have them yet, but this makes the code easier to understand.

mateiz · 2018-08-16T17:25:01Z

Do we want to write a small migration instruction thing for this? Could just go into the changelog under 0.5.0.

mateiz · 2018-08-16T17:25:40Z

docs/source/python_api/mlflow.entities.rst

@@ -0,0 +1,8 @@
+mlflow.entities


We should probably also update docs/source/tracking.rst to describe the two APIs, right?

Documentation is important, and I would like to get that closely reviewed. Let me do that in a follow-up PR which I'll go ahead and draft.

mateiz · 2018-08-16T22:27:42Z

mlflow/__init__.py

@@ -1,22 +1,43 @@
+"""Provides the MLflow fluent API, allowing management of an active MLflow run.


Micro style nit: I would put the """ on its own line in this case

I was following Google & PEP8's style here...

(I don't mind changing, but I just wanted something to point to for future people -- let me know if you have a style guide in mind)

I think PEP8 allows you to have the quotes on their own line in this case. Specifically in https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings : " The summary line may be on the same line as the opening quotes or on the next line. The entire docstring is indented the same as the quotes at its first line (see example below)."

Great, switched all the module comments to this form.

mateiz · 2018-08-16T22:28:33Z

mlflow/__init__.py

@@ -1,22 +1,43 @@
+"""Provides the MLflow fluent API, allowing management of an active MLflow run.
+For example::


Using ..code:: instead of :: will make it look better in the generated docs (for instance it adds the "copy this code" button and syntax highlighting).

mateiz · 2018-08-16T22:29:47Z

mlflow/entities/__init__.py

@@ -0,0 +1,23 @@
+from mlflow.entities.experiment import Experiment


Could add a doc comment here too

mateiz · 2018-08-16T22:32:31Z

mlflow/tracking/utils.py

+_tracking_uri = None
+
+
+def set_tracking_uri(uri):


It seems that this and get_tracking_uri are public functions, so do we want to move them or keep them in tracking? The rest of the stuff in this module is private. I guess you can set them through mlflow directly so it may be OK to move them, but it might also be better to just keep this module private.

The dependency structure is a bit weird. _get_store() has a lot of helper utilities to it (which is why I wanted to move it to its own py file), and it relies on _tracking_uri which is set by set_tracking_uri. fluent and service depend on utils, so I can't move _tracking_uri.

I could move the set/get methods to mutate a global specified in utils.py, but I'm not sure that's cleaner.

aarondav · 2018-08-17T01:34:45Z

All comments addressed @mateiz @smurching

mateiz · 2018-08-17T04:46:16Z

Went over it again and it looks good to me.

…API in mlflow#299

aarondav commented Aug 14, 2018

View reviewed changes

aarondav changed the title ~~[RFC] [#297] Complete Python SDK~~ [#297] Complete Python SDK Aug 15, 2018

aadamson reviewed Aug 15, 2018

View reviewed changes

smurching self-requested a review August 16, 2018 01:32

smurching reviewed Aug 16, 2018

View reviewed changes

mateiz reviewed Aug 16, 2018

View reviewed changes

aarondav added 3 commits August 16, 2018 17:09

[mlflow#297] Complete Python SDK

33c94a5

Address Matei and Sid comments

6abb6b4

Fix merge conflicts

d8630ea

aarondav force-pushed the python-apis branch from 01026f2 to d8630ea Compare August 17, 2018 00:13

Move """

9c75759

mateiz added the LGTM label Aug 17, 2018

aarondav merged commit 60610c5 into mlflow:master Aug 17, 2018

aarondav mentioned this pull request Aug 17, 2018

Add docs for mlflow.tracking #316

Closed

smurching added a commit to vfdev-5/mlflow that referenced this pull request Aug 17, 2018

Merge master, call into refactored methods after updates to tracking …

4f9d2ed

…API in mlflow#299

This was referenced Aug 22, 2018

Python API is missing methods from REST API #246

Closed

Complete Python SDK #297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#297] Complete Python SDK #299

[#297] Complete Python SDK #299

aarondav commented Aug 14, 2018 •

edited

aarondav Aug 14, 2018

mateiz commented Aug 14, 2018

aarondav commented Aug 15, 2018

aadamson Aug 15, 2018

aarondav Aug 16, 2018

codecov-io commented Aug 16, 2018 •

edited

smurching left a comment

smurching Aug 16, 2018

smurching Aug 16, 2018

smurching Aug 16, 2018

smurching Aug 16, 2018

smurching Aug 16, 2018

smurching Aug 16, 2018

smurching Aug 16, 2018

aarondav Aug 16, 2018

smurching Aug 16, 2018

mateiz Aug 16, 2018

mateiz commented Aug 16, 2018

mateiz Aug 16, 2018

aarondav Aug 17, 2018

mateiz Aug 16, 2018

aarondav Aug 16, 2018

aarondav Aug 17, 2018

mateiz Aug 17, 2018 •

edited

aarondav Aug 17, 2018

mateiz Aug 16, 2018

mateiz Aug 16, 2018

aarondav Aug 17, 2018

mateiz Aug 16, 2018

aarondav Aug 16, 2018

aarondav commented Aug 17, 2018

mateiz commented Aug 17, 2018

		@@ -44,42 +44,43 @@ def predicted(model, data):

		def test_model_save_load(model, data, predicted):

		@@ -13,6 +13,7 @@ def sftp_mock():
		return MagicMock(autospec=pysftp.Connection)


		@pytest.mark.large

		@@ -1,22 +1,43 @@
		"""Provides the MLflow fluent API, allowing management of an active MLflow run.

		@@ -1,22 +1,43 @@
		"""Provides the MLflow fluent API, allowing management of an active MLflow run.
		For example::

		@@ -0,0 +1,23 @@
		from mlflow.entities.experiment import Experiment

[#297] Complete Python SDK #299

[#297] Complete Python SDK #299

Conversation

aarondav commented Aug 14, 2018 • edited

Choose a reason for hiding this comment

mateiz commented Aug 14, 2018

aarondav commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 16, 2018 • edited

Codecov Report

smurching left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateiz commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateiz Aug 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarondav commented Aug 17, 2018

mateiz commented Aug 17, 2018

aarondav commented Aug 14, 2018 •

edited

codecov-io commented Aug 16, 2018 •

edited

mateiz Aug 17, 2018 •

edited