Repository: mlflow/mlflow
Pull request: Don't allow run creation if the experiment is deleted.
Should resolve https://github.com/mlflow/mlflow/issues/468.

Open question for reviewer, should we validate on APIs like log_param? Would this make the performance too poor?

Problem statement:
Broken mlruns after experiment deletion
### System information
- No custom code
- macOS 10.13.6
- pip
- `mlflow --version` = 0.6.0
- **Python version**: 3.6
- **Exact command to reproduce**: `mlflow experiments delete 1`

### Describe the problem
After deleting an experiment the UI breaks. An error is printed:

```
Exception: Yaml file '/my/project/dir/mlruns/.trash/meta.yaml' does not exist.
```

After manually deleting the directory `.trash`:

```
rm -rf mlruns/.trash
```

The UI functions as expected.

**EDIT**: However, after running another run, the problem pops up again.

Hints from issue discussion:
+1 getting the same issue on Ubuntu with no custom code, etc. 

I copied the meta.yaml file from another experiment and the problem seemed to go away.
Thanks for reporting the issue. I can report most of these issues and am working on a fix. The specific issue here is that we don't validate creating runs against deleted experiments.

Commit message: generate protos

Patch to review:
diff --git a/mlflow/entities/experiment.py b/mlflow/entities/experiment.py
index d52c3b32134b1..d423ef24750cb 100644
--- a/mlflow/entities/experiment.py
+++ b/mlflow/entities/experiment.py
@@ -7,12 +7,15 @@ class Experiment(_MLflowObject):
     Experiment object.
     """
     DEFAULT_EXPERIMENT_ID = 0
+    ACTIVE_LIFECYCLE = 'active'
+    DELETED_LIFECYCLE = 'deleted'
 
-    def __init__(self, experiment_id, name, artifact_location):
+    def __init__(self, experiment_id, name, artifact_location, lifecycle_stage):
         super(Experiment, self).__init__()
         self._experiment_id = experiment_id
         self._name = name
         self._artifact_location = artifact_location
+        self._lifecycle_stage = lifecycle_stage
 
     @property
     def experiment_id(self):
@@ -29,18 +32,24 @@ def artifact_location(self):
         """String corresponding to the root artifact URI for the experiment."""
         return self._artifact_location
 
+    @property
+    def lifecycle_stage(self):
+        """Lifecycle stage of the experiment. Can either be 'active' or 'deleted'."""
+        return self._lifecycle_stage
+
     @classmethod
     def from_proto(cls, proto):
-        return cls(proto.experiment_id, proto.name, proto.artifact_location)
+        return cls(proto.experiment_id, proto.name, proto.artifact_location, proto.lifecycle_stage)
 
     def to_proto(self):
         proto = ProtoExperiment()
         proto.experiment_id = self.experiment_id
         proto.name = self.name
         proto.artifact_location = self.artifact_location
+        proto.lifecycle_stage = self.lifecycle_stage
         return proto
 
     @classmethod
     def _properties(cls):
         # TODO: Hard coding this list of props for now. There has to be a clearer way...
-        return ["experiment_id", "name", "artifact_location"]
+        return ["experiment_id", "name", "artifact_location", "lifecycle_stage"]
diff --git a/mlflow/java/client/src/main/java/org/mlflow/api/proto/Service.java b/mlflow/java/client/src/main/java/org/mlflow/api/proto/Service.java
index 5204e80d02aea..632d377e1497e 100644
--- a/mlflow/java/client/src/main/java/org/mlflow/api/proto/Service.java
+++ b/mlflow/java/client/src/main/java/org/mlflow/api/proto/Service.java
@@ -8622,7 +8622,6 @@ public interface ExperimentOrBuilder extends
     /**
      * <pre>
      * Current life cycle stage of the experiment: "active" or "deleted".
-     * Deleted experiments are not returned by APIs.
      * </pre>
      *
      * <code>optional string lifecycle_stage = 4;</code>
@@ -8631,7 +8630,6 @@ public interface ExperimentOrBuilder extends
     /**
      * <pre>
      * Current life cycle stage of the experiment: "active" or "deleted".
-     * Deleted experiments are not returned by APIs.
      * </pre>
      *
      * <code>optional string lifecycle_stage = 4;</code>
@@ -8640,7 +8638,6 @@ public interface ExperimentOrBuilder extends
     /**
      * <pre>
      * Current life cycle stage of the experiment: "active" or "deleted".
-     * Deleted experiments are not returned by APIs.
      * </pre>
      *
      * <code>optional string lifecycle_stage = 4;</code>
@@ -8933,7 +8930,6 @@ public java.lang.String getArtifactLocation() {
     /**
      * <pre>
      * Current life cycle stage of the experiment: "active" or "deleted".
-     * Deleted experiments are not returned by APIs.
      * </pre>
      *
      * <code>optional string lifecycle_stage = 4;</code>
@@ -8944,7 +8940,6 @@ public boolean hasLifecycleStage() {
     /**
      * <pre>
      * Current life cycle stage of the experiment: "active" or "deleted".
-     * Deleted experiments are not returned by APIs.
      * </pre>
      *
      * <code>optional string lifecycle_stage = 4;</code>
@@ -8966,7 +8961,6 @@ public java.lang.String getLifecycleStage() {
     /**
      * <pre>
      * Current life cycle stage of the experiment: "active" or "deleted".
-     * Deleted experiments are not returned by APIs.
      * </pre>
      *
      * <code>optional string lifecycle_stage = 4;</code>
@@ -9733,7 +9727,6 @@ public Builder setArtifactLocationBytes(
       /**
        * <pre>
        * Current life cycle stage of the experiment: "active" or "deleted".
-       * Deleted experiments are not returned by APIs.
        * </pre>
        *
        * <code>optional string lifecycle_stage = 4;</code>
@@ -9744,7 +9737,6 @@ public boolean hasLifecycleStage() {
       /**
        * <pre>
        * Current life cycle stage of the experiment: "active" or "deleted".
-       * Deleted experiments are not returned by APIs.
        * </pre>
        *
        * <code>optional string lifecycle_stage = 4;</code>
@@ -9766,7 +9758,6 @@ public java.lang.String getLifecycleStage() {
       /**
        * <pre>
        * Current life cycle stage of the experiment: "active" or "deleted".
-       * Deleted experiments are not returned by APIs.
        * </pre>
        *
        * <code>optional string lifecycle_stage = 4;</code>
@@ -9787,7 +9778,6 @@ public java.lang.String getLifecycleStage() {
       /**
        * <pre>
        * Current life cycle stage of the experiment: "active" or "deleted".
-       * Deleted experiments are not returned by APIs.
        * </pre>
        *
        * <code>optional string lifecycle_stage = 4;</code>
@@ -9805,7 +9795,6 @@ public Builder setLifecycleStage(
       /**
        * <pre>
        * Current life cycle stage of the experiment: "active" or "deleted".
-       * Deleted experiments are not returned by APIs.
        * </pre>
        *
        * <code>optional string lifecycle_stage = 4;</code>
@@ -9819,7 +9808,6 @@ public Builder clearLifecycleStage() {
       /**
        * <pre>
        * Current life cycle stage of the experiment: "active" or "deleted".
-       * Deleted experiments are not returned by APIs.
        * </pre>
        *
        * <code>optional string lifecycle_stage = 4;</code>
diff --git a/mlflow/protos/service.proto b/mlflow/protos/service.proto
index 6957b4c6a2b0f..3ffa42f721c5c 100644
--- a/mlflow/protos/service.proto
+++ b/mlflow/protos/service.proto
@@ -46,7 +46,7 @@ service MlflowService {
   }
 
   // Get metadata for an experiment and a list of runs for the experiment.
-  //
+  // This RPC will work on deleted experiments.
   rpc getExperiment (GetExperiment) returns (GetExperiment.Response) {
     option (rpc) = {
       endpoints: [{
@@ -445,7 +445,6 @@ message Experiment {
   optional string artifact_location = 3;
 
   // Current life cycle stage of the experiment: "active" or "deleted".
-  // Deleted experiments are not returned by APIs.
   optional string lifecycle_stage = 4;
 
   // Last update time
diff --git a/mlflow/store/file_store.py b/mlflow/store/file_store.py
index 7dab234683106..7d51457a67dd5 100644
--- a/mlflow/store/file_store.py
+++ b/mlflow/store/file_store.py
@@ -87,12 +87,12 @@ def _get_experiment_path(self, experiment_id, view_type=ViewType.ALL):
         for parent in parents:
             exp_list = find(parent, str(experiment_id), full_path=True)
             if len(exp_list) > 0:
-                return exp_list
-        return []
+                return exp_list[0]
+        return None
 
     def _get_run_dir(self, experiment_id, run_uuid):
         _validate_run_id(run_uuid)
-        return build_path(self._get_experiment_path(experiment_id)[0], run_uuid)
+        return build_path(self._get_experiment_path(experiment_id), run_uuid)
 
     def _get_metric_path(self, experiment_id, run_uuid, metric_key):
         _validate_run_id(run_uuid)
@@ -139,7 +139,7 @@ def _create_experiment_with_id(self, name, experiment_id, artifact_uri):
         self._check_root_dir()
         meta_dir = mkdir(self.root_directory, str(experiment_id))
         artifact_uri = artifact_uri or build_path(self.artifact_root_uri, str(experiment_id))
-        experiment = Experiment(experiment_id, name, artifact_uri)
+        experiment = Experiment(experiment_id, name, artifact_uri, Experiment.ACTIVE_LIFECYCLE)
         write_yaml(meta_dir, FileStore.META_DATA_FILE_NAME, dict(experiment))
         return experiment_id
 
@@ -157,14 +157,18 @@ def create_experiment(self, name, artifact_location=None):
         return self._create_experiment_with_id(name, experiment_id, artifact_location)
 
     def _has_experiment(self, experiment_id):
-        return len(self._get_experiment_path(experiment_id)) > 0
+        return self._get_experiment_path(experiment_id) is not None
 
     def _get_experiment(self, experiment_id, view_type=ViewType.ALL):
         self._check_root_dir()
-        experiment_dirs = self._get_experiment_path(experiment_id, view_type)
-        if len(experiment_dirs) == 0:
+        experiment_dir = self._get_experiment_path(experiment_id, view_type)
+        if experiment_dir is None:
             raise Exception("Could not find experiment with ID %s" % experiment_id)
-        meta = read_yaml(experiment_dirs[0], FileStore.META_DATA_FILE_NAME)
+        meta = read_yaml(experiment_dir, FileStore.META_DATA_FILE_NAME)
+        if experiment_dir.startswith(self.trash_folder):
+            meta['lifecycle_stage'] = Experiment.DELETED_LIFECYCLE
+        else:
+            meta['lifecycle_stage'] = Experiment.ACTIVE_LIFECYCLE
         return Experiment.from_dictionary(meta)
 
     def get_experiment(self, experiment_id):
@@ -184,20 +188,20 @@ def get_experiment_by_name(self, name):
         return None
 
     def delete_experiment(self, experiment_id):
-        experiment_dirs = self._get_experiment_path(experiment_id, ViewType.ACTIVE_ONLY)
-        if len(experiment_dirs) == 0:
+        experiment_dir = self._get_experiment_path(experiment_id, ViewType.ACTIVE_ONLY)
+        if experiment_dir is None:
             raise Exception("Could not find experiment with ID %s" % experiment_id)
-        mv(experiment_dirs[0], self.trash_folder)
+        mv(experiment_dir, self.trash_folder)
 
     def restore_experiment(self, experiment_id):
-        experiment_dirs = self._get_experiment_path(experiment_id, ViewType.DELETED_ONLY)
-        if len(experiment_dirs) == 0:
+        experiment_dir = self._get_experiment_path(experiment_id, ViewType.DELETED_ONLY)
+        if experiment_dir is None:
             raise Exception("Could not find deleted experiment with ID %d" % experiment_id)
         conflict_experiment = self._get_experiment_path(experiment_id, ViewType.ACTIVE_ONLY)
-        if len(conflict_experiment) > 0:
+        if conflict_experiment is not None:
             raise Exception("Cannot restore eperiment with ID %d. "
                             "An experiment with same ID already exists." % experiment_id)
-        mv(experiment_dirs[0], self.root_directory)
+        mv(experiment_dir, self.root_directory)
 
     def delete_run(self, run_id):
         run_info = self._get_run_info(run_id)
@@ -244,9 +248,13 @@ def create_run(self, experiment_id, user_id, run_name, source_type,
         """
         Creates a run with the specified attributes.
         """
-        if self.get_experiment(experiment_id) is None:
+        experiment = self.get_experiment(experiment_id)
+        if experiment is None:
             raise Exception("Could not create run under experiment with ID %s - no such experiment "
                             "exists." % experiment_id)
+        if experiment.lifecycle_stage != Experiment.ACTIVE_LIFECYCLE:
+            raise Exception('Could not create run under non-active experiment with ID '
+                            '%s.' % experiment_id)
         run_uuid = uuid.uuid4().hex
         artifact_uri = self._get_artifact_dir(experiment_id, run_uuid)
         run_info = RunInfo(run_uuid=run_uuid, experiment_id=experiment_id,
@@ -269,6 +277,12 @@ def create_run(self, experiment_id, user_id, run_name, source_type,
             self.set_tag(run_uuid, RunTag(key=MLFLOW_RUN_NAME, value=run_name))
         return Run(run_info=run_info, run_data=None)
 
+    def _make_experiment_dict(self, experiment):
+        # Don't persist lifecycle_stage since it's inferred from the ".trash" folder.
+        experiment_dict = dict(experiment)
+        del experiment_dict['lifecycle_stage']
+        return experiment_dict
+
     def get_run(self, run_uuid):
         """
         Will get both active and deleted runs.
@@ -393,7 +407,7 @@ def get_all_tags(self, run_uuid):
 
     def _list_run_uuids(self, experiment_id, run_view_type):
         self._check_root_dir()
-        experiment_dir = self._get_experiment_path(experiment_id)[0]
+        experiment_dir = self._get_experiment_path(experiment_id)
         run_uuids = list_all(experiment_dir, os.path.isdir, full_path=False)
         if run_view_type == ViewType.ALL:
             return run_uuids
diff --git a/tests/entities/test_experiment.py b/tests/entities/test_experiment.py
index 63b367d2b9bc9..5b476e6f79f11 100644
--- a/tests/entities/test_experiment.py
+++ b/tests/entities/test_experiment.py
@@ -5,30 +5,35 @@
 
 
 class TestExperiment(unittest.TestCase):
-    def _check(self, exp, exp_id, name, location):
+    def _check(self, exp, exp_id, name, location, lifecyle_stage):
         self.assertIsInstance(exp, Experiment)
         self.assertEqual(exp.experiment_id, exp_id)
         self.assertEqual(exp.name, name)
         self.assertEqual(exp.artifact_location, location)
+        self.assertEqual(exp.lifecycle_stage, lifecyle_stage)
 
     def test_creation_and_hydration(self):
         exp_id = random_int()
         name = "exp_%d_%d" % (random_int(), random_int())
+        lifecycle_stage = Experiment.ACTIVE_LIFECYCLE
         location = random_file(".json")
 
-        exp = Experiment(exp_id, name, location)
-        self._check(exp, exp_id, name, location)
+        exp = Experiment(exp_id, name, location, lifecycle_stage)
+        self._check(exp, exp_id, name, location, lifecycle_stage)
 
-        as_dict = {"experiment_id": exp_id, "name": name, "artifact_location": location}
+        as_dict = {"experiment_id": exp_id, "name": name, "artifact_location": location,
+                   "lifecycle_stage": lifecycle_stage}
         self.assertEqual(dict(exp), as_dict)
 
         proto = exp.to_proto()
         exp2 = Experiment.from_proto(proto)
-        self._check(exp2, exp_id, name, location)
+        self._check(exp2, exp_id, name, location, lifecycle_stage)
 
         exp3 = Experiment.from_dictionary(as_dict)
-        self._check(exp3, exp_id, name, location)
+        self._check(exp3, exp_id, name, location, lifecycle_stage)
 
     def test_string_repr(self):
-        exp = Experiment(experiment_id=0, name="myname", artifact_location="hi")
-        assert str(exp) == "<Experiment: experiment_id=0, name='myname', artifact_location='hi'>"
+        exp = Experiment(experiment_id=0, name="myname", artifact_location="hi",
+                         lifecycle_stage=Experiment.ACTIVE_LIFECYCLE)
+        assert str(exp) == "<Experiment: experiment_id=0, name='myname', artifact_location='hi', " \
+                           "lifecycle_stage='active'>"
diff --git a/tests/store/test_file_store.py b/tests/store/test_file_store.py
index 5b3f79fa28adf..878b17b3221a2 100644
--- a/tests/store/test_file_store.py
+++ b/tests/store/test_file_store.py
@@ -188,6 +188,8 @@ def test_delete_restore_experiment(self):
         self.assertTrue(exp_id not in self._extract_ids(fs.list_experiments(ViewType.ACTIVE_ONLY)))
         self.assertTrue(exp_id in self._extract_ids(fs.list_experiments(ViewType.DELETED_ONLY)))
         self.assertTrue(exp_id in self._extract_ids(fs.list_experiments(ViewType.ALL)))
+        self.assertEqual(fs.get_experiment(exp_id).lifecycle_stage,
+                         Experiment.DELETED_LIFECYCLE)
 
         # restore it
         fs.restore_experiment(exp_id)
@@ -200,6 +202,8 @@ def test_delete_restore_experiment(self):
         self.assertTrue(exp_id in self._extract_ids(fs.list_experiments(ViewType.ACTIVE_ONLY)))
         self.assertTrue(exp_id not in self._extract_ids(fs.list_experiments(ViewType.DELETED_ONLY)))
         self.assertTrue(exp_id in self._extract_ids(fs.list_experiments(ViewType.ALL)))
+        self.assertEqual(fs.get_experiment(exp_id).lifecycle_stage,
+                         Experiment.ACTIVE_LIFECYCLE)
 
     def test_delete_restore_run(self):
         fs = FileStore(self.test_root)
@@ -212,6 +216,15 @@ def test_delete_restore_run(self):
         fs.restore_run(run_id)
         assert fs.get_run(run_id).info.lifecycle_stage == 'active'
 
+    def test_create_run_in_deleted_experiment(self):
+        fs = FileStore(self.test_root)
+        exp_id = self.experiments[random_int(0, len(self.experiments) - 1)]
+        # delete it
+        fs.delete_experiment(exp_id)
+        with pytest.raises(Exception):
+            fs.create_run(exp_id, 'user', 'name', 'source_type', 'source_name', 'entry_point_name',
+                          0, None, [])
+
     def test_get_run(self):
         fs = FileStore(self.test_root)
         for exp_id in self.experiments:
diff --git a/tests/utils/test_proto_json_utils.py b/tests/utils/test_proto_json_utils.py
index 02a8db3050e6d..6e8666c526a85 100644
--- a/tests/utils/test_proto_json_utils.py
+++ b/tests/utils/test_proto_json_utils.py
@@ -6,11 +6,12 @@
 
 
 def test_message_to_json():
-    json_out = message_to_json(Experiment(123, "name", "arty").to_proto())
+    json_out = message_to_json(Experiment(123, "name", "arty", 'active').to_proto())
     assert json.loads(json_out) == {
         "experiment_id": "123",
         "name": "name",
         "artifact_location": "arty",
+        "lifecycle_stage": 'active',
     }
 
 


Code review comments:
diff_hunk: @@ -87,12 +87,12 @@ def _get_experiment_path(self, experiment_id, view_type=ViewType.ALL):
         for parent in parents:
             exp_list = find(parent, str(experiment_id), full_path=True)
             if len(exp_list) > 0:
-                return exp_list
-        return []
+                return exp_list[0]
+        return None
path: mlflow/store/file_store.py
text: @user1:
Should we raise an exception in this case actually? Otherwise we might try to use `None` as a path segment down the line in `build_path`, which will result in a more obscure `AttributeError: 'NoneType' object has no attribute 'endswith'`. The case where we use `None` as a path segment is admittedly rare right now (I think it can only happen if you call `log_xxxx` on an experiment that was manually deleted from the filesystem), so open to leaving as is, but seems safest to raise the exception here