Bug fix: Make FileStore respect the default artifact location (mlflow…

…#1332)

* Fix with test

* Revert test

* WIP

* Valuerror

* test fixes

* assert fix

* Fix tests and address comment

* Fix experiment_id append

* Smaller change

* Adjust logic for concatenating experiment ID to artifact root path, fix test

* Revert change to construction order

* Revert and use posixpath

* Test for creation of artifact location containing experiment ID

* Test debug

* Test debug

* Fix edge case where absolute path is not detected with trailing slash

* Fix broken test

* Higher fidelity exp id ending test

* Keep subdiring? Shouldn't be related to sqlalchemy...

* Optimistically refert abspath fix

mlflow/R/mlflow/tests/testthat/test-tracking-experiments.R

@@ -45,7 +45,7 @@ test_that("mlflow_list_experiments() works properly", {
   experiments_list <- mlflow_list_experiments(client = client)
   expect_setequal(experiments_list$experiment_id, c("0", "1", "2"))
   expect_setequal(experiments_list$name, c("Default", "foo1", "foo2"))
-  default_artifact_loc <- file.path("file:/", getwd(), "mlruns", "0", fsep = "/")
+  default_artifact_loc <- file.path(getwd(), "mlruns", "0", fsep = "/")
   expect_setequal(experiments_list$artifact_location, c(default_artifact_loc,
                                                         "art_loc1",
                                                         "art_loc2"))
@@ -54,7 +54,7 @@ test_that("mlflow_list_experiments() works properly", {
   experiments_list <- mlflow_list_experiments()
   expect_setequal(experiments_list$experiment_id, c("0", "1", "2"))
   expect_setequal(experiments_list$name, c("Default", "foo1", "foo2"))
-  default_artifact_loc <- file.path("file:/", getwd(), "mlruns", "0", fsep = "/")
+  default_artifact_loc <- file.path(getwd(), "mlruns", "0", fsep = "/")
   expect_setequal(experiments_list$artifact_location, c(default_artifact_loc,
                                                         "art_loc1",
                                                         "art_loc2"))

mlflow/store/file_store.py

@@ -190,10 +190,9 @@ def list_experiments(self, view_type=ViewType.ACTIVE_ONLY):
         return experiments
 
     def _create_experiment_with_id(self, name, experiment_id, artifact_uri):
+        artifact_uri = artifact_uri or posixpath.join(self.artifact_root_uri, str(experiment_id))
         self._check_root_dir()
         meta_dir = mkdir(self.root_directory, str(experiment_id))
-        artifact_uri = artifact_uri or path_to_local_file_uri(
-            os.path.join(self.root_directory, str(experiment_id)))
         experiment = Experiment(experiment_id, name, artifact_uri, LifecycleStage.ACTIVE)
         write_yaml(meta_dir, FileStore.META_DATA_FILE_NAME, dict(experiment))
         return experiment_id

tests/store/test_file_store.py

@@ -1,6 +1,7 @@
 #!/usr/bin/env python
 # -*- coding: utf-8 -*-
 import os
+import posixpath
 import random
 import shutil
 import six
@@ -16,7 +17,7 @@
 from mlflow.exceptions import MlflowException, MissingConfigException
 from mlflow.store import SEARCH_MAX_RESULTS_DEFAULT
 from mlflow.store.file_store import FileStore
-from mlflow.utils.file_utils import write_yaml, read_yaml
+from mlflow.utils.file_utils import write_yaml, read_yaml, path_to_local_file_uri
 from mlflow.protos.databricks_pb2 import ErrorCode, RESOURCE_DOES_NOT_EXIST, INTERNAL_ERROR
 from mlflow.utils.search_utils import SearchFilter
 
@@ -188,6 +189,8 @@ def test_create_experiment(self):
         # get the new experiment (by id) and verify (by name)
         exp1 = fs.get_experiment(created_id)
         self.assertEqual(exp1.name, name)
+        self.assertEqual(exp1.artifact_location,
+                         path_to_local_file_uri(posixpath.join(self.test_root, created_id)))
 
         # get the new experiment (by name) and verify (by id)
         exp2 = fs.get_experiment_by_name(name)

tests/tracking/test_rest_tracking.py

@@ -7,7 +7,9 @@
 from subprocess import Popen
 import os
 import sys
+import posixpath
 import pytest
+from six.moves import urllib
 import socket
 import shutil
 from threading import Thread
@@ -364,7 +366,14 @@ def test_set_terminated_status(mlflow_client):
 
 def test_artifacts(mlflow_client):
     experiment_id = mlflow_client.create_experiment('Art In Fact')
+    experiment_info = mlflow_client.get_experiment(experiment_id)
+    assert experiment_info.artifact_location.startswith(
+        path_to_local_file_uri(SUITE_ARTIFACT_ROOT_DIR))
+    artifact_path = urllib.parse.urlparse(experiment_info.artifact_location).path
+    assert posixpath.split(artifact_path)[-1] == experiment_id
+
     created_run = mlflow_client.create_run(experiment_id)
+    assert created_run.info.artifact_uri.startswith(experiment_info.artifact_location)
     run_id = created_run.info.run_id
     src_dir = tempfile.mkdtemp('test_artifacts_src')
     src_file = os.path.join(src_dir, 'my.file')