Skip to content

Commit

Permalink
Experiment_id from int64 to string in protos (mlflow#1067)
Browse files Browse the repository at this point in the history
* Add changes to protos

* Update experiment_id for RestStore to be string. Maintain int for local stores, update handlers to reflect the server change, casting local store results where necessary

* Keep same convention throughout

* Maintain type for RunInfo copy

* Attempt to fix java client, handle string to long conversion in mapper. Need to explore builder if current changes are not enough

* Resolve typo

* Fix python tests, resolve compilation errors reported in java build

* Resolve two java test failures

* try catch int cast within handlers to avoid premature errors in handlers.py

* Resolve java lint errors

* Move comments to requeue since the last build had an internal failure

* Return None by default, handle that case in get_experiment calls

* Update experiment_id PR to reflect issue. Return None by default, add handling for None in file_store and sqlalchemt_store, revert changes to handler.py. Update tests for store to reflect the changes

* Fix docker tests, use default experiment id instead of 0

* Use default experiment id instead of 0

* Pass id to store as string

* Fix local store tests, only create default store once

* Fix remaining broken tests

* Try to fix cli test and lint issues

* Linter ignored noqa, reverting part of the pytest changes for now. Trying to debug docker test failer.

* Make cli assume int. Still dealing with old remote mlflow sdk against
new cli"

* Use read and write persisted paradigm to write int experiment_ids as int and read them back into string when deserializing experiment_id. This is a workaround. The test should run against the latest code in the submitted environemnt. However, this sort of backwards compatibility may be worth while since user's may pin version on local or remote and have unexpected errors

* Resolve flake8 error. Update cli call to reflect new type and default for experimnent_id cli calls

* Fix python 2 lint issue

* Resolve comments, move default out of Experiment and into respective stores. Fix bug in cli not updating the experiment id version

* Introduce minor change for a requeue of the unrelated R build failure

* Remove str cast of mock

* Remove all references to Long experimentId in java implementation

* Remove override type in write

* Add backcompat for 0.9.1 release, continue to return '0' for default experiment_id

* Fix bug in file_store rename code path

* Revert move to pytest

* Temporary hack while evaluatin best option for remote docker tests

* Resolve missing fixture

* Resolve flake8 error

* Resolve test change to zero default for 0.9.1

* Test int as string backcompat

* Add test for reading old experiment and run yamls, where experiment_id is int

* Mini commit to requeu builds

* Remove subtest, broke python 2.7 build. Make 2 new tests for back compat instead

* Handle and test experiment_id int to string before deserialization to avoid failure in create_experiment with existing servers

* Revert "Handle and test experiment_id int to string before deserialization to avoid failure in create_experiment with existing servers"

This reverts commit 0a8d944.

* Add deserializer for old requests, add tests for permutations

* Simplify backcompat helper, only cover top two layers of the returned json, update tests

* Add TODO and remove prints

* Fix silly bug in int type check for experiment id

* Resolve bad merge for change in location of DEFAULT_EXPERIMENT_ID constant

* Addressed some review comments.

* Removed leftover debug changes.

* Avoid python-level backcompat issue with MlflowClient

* Add docs

* Standardize reference to experiment_id in docs

* Address feedback on moving util to tests

* Skip backwards compatibility test on builds, keep test for local debugging

* Remove duplicated back compat code path assert. Keep it in the back compat test

* Fix extra lines

* Resolve lint issues in test_backwards_compatability.py

* Remove pytest, lint.sh locally did not pick up on this

* Remove skipped tests, they were run locally
  • Loading branch information
eddiestudies authored and tomasatdatabricks committed Apr 19, 2019
1 parent b817e28 commit 0b871a4
Show file tree
Hide file tree
Showing 39 changed files with 1,462 additions and 503 deletions.
6 changes: 2 additions & 4 deletions mlflow/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import mlflow.sagemaker.cli
import mlflow.runs

from mlflow.entities.experiment import Experiment
from mlflow.tracking.utils import _is_local_uri
from mlflow.utils.logging_utils import eprint
from mlflow.utils.process import ShellCommandException
Expand Down Expand Up @@ -53,9 +52,8 @@ def cli():
@click.option("--experiment-name", envvar=tracking._EXPERIMENT_NAME_ENV_VAR,
help="Name of the experiment under which to launch the run. If not "
"specified, 'experiment-id' option will be used to launch run.")
@click.option("--experiment-id", envvar=tracking._EXPERIMENT_ID_ENV_VAR, type=click.INT,
help="ID of the experiment under which to launch the run. Defaults to %s" %
Experiment.DEFAULT_EXPERIMENT_ID)
@click.option("--experiment-id", envvar=tracking._EXPERIMENT_ID_ENV_VAR, type=click.STRING,
help="ID of the experiment under which to launch the run.")
# TODO: Add tracking server argument once we have it working.
@click.option("--mode", "-m", metavar="MODE",
help="Execution mode to use for run. Supported values: 'local' (runs project "
Expand Down
3 changes: 1 addition & 2 deletions mlflow/entities/experiment.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ class Experiment(_MLflowObject):
"""
Experiment object.
"""
DEFAULT_EXPERIMENT_ID = 0
DEFAULT_EXPERIMENT_NAME = "Default"

def __init__(self, experiment_id, name, artifact_location, lifecycle_stage):
Expand All @@ -18,7 +17,7 @@ def __init__(self, experiment_id, name, artifact_location, lifecycle_stage):

@property
def experiment_id(self):
"""Integer ID of the experiment."""
"""String ID of the experiment."""
return self._experiment_id

@property
Expand Down
2 changes: 1 addition & 1 deletion mlflow/entities/run_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def run_uuid(self):

@property
def experiment_id(self):
"""Integer ID of the experiment for the current run."""
"""String ID of the experiment for the current run."""
return self._experiment_id

@property
Expand Down
2 changes: 1 addition & 1 deletion mlflow/experiments.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from mlflow.entities import ViewType
from mlflow.tracking import _get_store

EXPERIMENT_ID = click.argument("experiment_id", type=click.INT)
EXPERIMENT_ID = click.argument("experiment_id", type=click.STRING)


@click.group("experiments")
Expand Down
12 changes: 6 additions & 6 deletions mlflow/java/client/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ and [Service.java domain objects](src/main/java/org/mlflow/api/proto/mlflow/Serv
```
Run getRun(String runUuid)
RunInfo createRun()
RunInfo createRun(long experimentId)
RunInfo createRun(long experimentId, String appName)
RunInfo createRun(String experimentId)
RunInfo createRun(String experimentId, String appName)
RunInfo createRun(CreateRun request)
List<RunInfo> listRunInfos(long experimentId)
List<RunInfo> listRunInfos(String experimentId)
List<Experiment> listExperiments()
GetExperiment.Response getExperiment(long experimentId)
GetExperiment.Response getExperiment(String experimentId)
Optional<Experiment> getExperimentByName(String experimentName)
long createExperiment(String experimentName)
Expand Down Expand Up @@ -110,7 +110,7 @@ public class QuickStartDriver {
System.out.println("====== createExperiment");
String expName = "Exp_" + System.currentTimeMillis();
long expId = client.createExperiment(expName);
String expId = client.createExperiment(expName);
System.out.println("createExperiment: expId=" + expId);
System.out.println("====== getExperiment");
Expand All @@ -133,7 +133,7 @@ public class QuickStartDriver {
System.out.println("getExperimentByName: " + exp3);
}
void createRun(MlflowClient client, long expId) {
void createRun(MlflowClient client, String expId) {
System.out.println("====== createRun");
// Create run
Expand Down
Loading

0 comments on commit 0b871a4

Please sign in to comment.