cli support for delete and restore + docs #367
Conversation
mparkhe
requested review from
a user,
mateiz,
aarondav,
andrewmchen,
smurching and
tomasatdatabricks
Codecov Report
@@ Coverage Diff @@
## master #367 +/- ##
==========================================
- Coverage 27.81% 27.76% -0.05%
==========================================
Files 114 114
Lines 5836 5846 +10
==========================================
Hits 1623 1623
- Misses 4213 4223 +10
Continue to review full report at Codecov.
|
docs/source/cli.rst
Outdated
|
|
||
|
|
||
| Create | ||
| ------ |
There was a problem hiding this comment.
I think this needs to be a separate kind of header from ---- to render correctly in the docs, since it's under Experiments. Right now these will look like the same level of heading. You can look at the Sphinx/RST docs to see the hierarchy of headings allowed.
There was a problem hiding this comment.
It might also be possible to just make this a bulleted list instead of sections, since there's a lot of detail about each one.
There was a problem hiding this comment.
Oh! I misread the documentations. Subsections are marked with "~" . Patching coming soon.
I had initially tried the bullet route, but it looked a bit bulky.
docs/source/cli.rst
Outdated
| Delete an active experiment. Command takes an mandatory argument experiment ID. If experiment | ||
| is already deleted or not found, the command will throw error. This deletes associated metadata, | ||
| runs and data as well. If the backend store controls locations of artifacts, they will be deleted | ||
| as well. Deleted experiments can be restored using ``restore`` command. |
There was a problem hiding this comment.
Maybe talk about how they can clear the trash here? Can also recommend setting up a cron job or something.
mlflow/experiments.py
Outdated
| def delete_experiment(experiment_id): | ||
| """ | ||
| Delete an experiment from backend store. | ||
| Will error out if experiment does not exist or already deleted. |
There was a problem hiding this comment.
This help should say that you mark it for deletion. Experiments marked this way can be restored with restore, or cleared based on the backend store (refer to docs for that).
mlflow/experiments.py
Outdated
| """ | ||
| store = _get_store() | ||
| store.restore_experiment(experiment_id) | ||
| print("Experiment with id %s has been restored and is now active." % str(experiment_id)) |
There was a problem hiding this comment.
I'd remove the "is now active".
mlflow/experiments.py
Outdated
| @@ -38,7 +38,31 @@ def list_experiments(): | |||
| List all experiments in the configured tracking server. | |||
| """ | |||
| store = _get_store() | |||
| experiments = store.list_experiments() | |||
| experiments = store.list_experiments("deleted_only") | |||
There was a problem hiding this comment.
ToDo : fix this. Should default to active_only
Codecov Report
@@ Coverage Diff @@
## master #367 +/- ##
==========================================
- Coverage 27.98% 27.94% -0.05%
==========================================
Files 115 115
Lines 5925 5948 +23
==========================================
+ Hits 1658 1662 +4
- Misses 4267 4286 +19
Continue to review full report at Codecov.
|
mlflow/experiments.py
Outdated
| def list_experiments(): | ||
| @click.option("--view", "-v", default="active_only", | ||
| help="Select view type for list experiments. Valid view types are " | ||
| "'active_only' (default), 'delete_only', and 'all'.") |
There was a problem hiding this comment.
Typo: delete_only should say deleted_only
|
LGTM except for a small typo. |
No description provided.