Add rename_experiment Tracking API, along with integration tests #570
Conversation
| @@ -27,6 +27,9 @@ def name(self): | |||
| """String name of the experiment.""" | |||
| return self._name | |||
|
|
|||
| def _set_name(self, new_name): | |||
There was a problem hiding this comment.
Needed internally to mutate in file store.
| @@ -133,6 +133,24 @@ public long createExperiment(String experimentName) { | |||
| return mapper.toCreateExperimentResponse(ojson).getExperimentId(); | |||
| } | |||
|
|
|||
| /** Mark an experiment and associated runs, params, metrics, etc for deletion. */ | |||
| public void deleteExperiment(long experimentId) { | |||
There was a problem hiding this comment.
Also added delete & restore experiment to Java Tracking API (already exists in Python API).
| @@ -93,30 +112,6 @@ String toJson(MessageOrBuilder mb) { | |||
| return builder.build(); | |||
| } | |||
|
|
|||
| GetMetric.Response toGetMetricResponse(String json) { | |||
There was a problem hiding this comment.
Removed these unused methods, can always add them back later when they're used.
| @@ -36,6 +36,6 @@ public static void assertTag(List<RunTag> tags, String key, String value) { | |||
| } | |||
|
|
|||
| static public String createExperimentName() { | |||
| return "TestExp_" + UUID.randomUUID().toString(); | |||
| return "JavaTestExp_" + UUID.randomUUID().toString(); | |||
There was a problem hiding this comment.
Just to help people grep for code.
| // ID of the associated experiment. | ||
| optional int64 experiment_id = 1 [(validate_required) = true]; | ||
|
|
||
| // If provided, the experiment's name will be changed to this. The new name must be unique. | ||
| optional string new_name = 2; |
There was a problem hiding this comment.
Used new_name to distinguish from the fact that we sometimes use experiment_name as an identifier for an experiment.
| @@ -57,8 +57,8 @@ def create_run(self, experiment_id, user_id=None, run_name=None, source_type=Non | |||
| experiment_id=experiment_id, | |||
| user_id=user_id if user_id is not None else _get_user_id(), | |||
| run_name=run_name, | |||
| source_type=source_type, | |||
| source_name=source_name, | |||
| source_type=source_type if source_type is not None else SourceType.LOCAL, | |||
There was a problem hiding this comment.
Added sane defaults here to look closer to the Java API; otherwise, create_run(experiment_id) does not work. Found via the integration test!
aarondav
force-pushed
the
add-update-experiment-name
branch
from
7b92452
to
2fc7096
Compare
Codecov Report
@@ Coverage Diff @@
## master #570 +/- ##
=======================================
Coverage 65.75% 65.75%
=======================================
Files 22 22
Lines 800 800
=======================================
Hits 526 526
Misses 274 274Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks really good, the integration tests are an awesome addition - just had one comment about checking if an experiment is deleted before renaming it (would be nice to test that case too). Also, it'd be cool to add rename_experiment to the experiments CLI, though we could do that in a follow-up.
| meta_dir = os.path.join(self.root_directory, str(experiment_id)) | ||
| experiment = self._get_experiment(experiment_id) | ||
| experiment._set_name(new_name) | ||
| write_yaml(meta_dir, FileStore.META_DATA_FILE_NAME, dict(experiment), overwrite=True) |
There was a problem hiding this comment.
We should check if the experiment is deleted here, since it'll be in the trash folder (a different directory) if it is. A simple option would be disallowing renaming deleted experiments (matches our existing pattern of not allowing updating/logging params/metrics to deleted runs). Another would be using self._get_experiment_path() to get the path of the experiment's metadata file, and updating the name there.
There was a problem hiding this comment.
^Either way, we'd still be subject to bugs in the case of concurrent attempts to delete & rename an experiment. However that race condition would hopefully be rare, so it seems worth it to try to detect if an experiment is deleted.
|
@smurching Addressed comments, including adding CLI support. |
mlflow/experiments.py
Outdated
|
|
||
| @commands.command("rename") | ||
| @click.argument("experiment_id") | ||
| @click.argument("new_name", help="New name to give to the experiment.") |
There was a problem hiding this comment.
Unfortunately click.argument doesn't have a help argument (see https://click.palletsprojects.com/en/7.x/documentation/#help-texts), I think we remove that tests will pass (barring an unrelated failure on some Keras tests that I'll look into).
aarondav
force-pushed
the
add-update-experiment-name
branch
from
2a06651
to
e3348a6
Compare
Adds a new REST API called
UpdateExperimentwhich currently only supports changing name via settingnew_name, but may be able to mutate other properties in the future. Exposed to users via the tracking API is justrename_experimentto change the name -- the is similar to howset_terminatedworks on runs today.Also added support in the Python & Java Tracking APIs and file store, and
experimentsCLI.Finally, in adding this I realized that although there are Java client integration tests, there are no tests which spin up a REST store and make end-to-end calls via the tracking API (in other words, the tracking API is not tested). To deal with this, I wrote an integration test at this level.
I think in general the expectation should be if you want to add a new API, you will have to add unit tests to the file store (backing implementation) and integration tests in the Java and Python Tracking API code.