Repository: mlflow/mlflow
Pull request: Fix for parameter parsing when parameter contains '='
## What changes are proposed in this pull request?

Fixes #3346 (a regression introduced by #3061) where project parameters whose values contain the `=` character were not being parsed properly.

## How is this patch tested?

Added unit test coverage to `test_projects_cli`

## Release Notes

Fixed a regression causing parameter values containing `=` to be parsed improperly.

### Is this a user-facing change?

- [ ] No. You can skip the rest of this section.
- [X] Yes. Give a description of this change to be included in the release notes for MLflow users.

### What component(s), interfaces, languages, and integrations does this PR affect?
Components 
- [ ] `area/artifacts`: Artifact stores and artifact logging
- [ ] `area/build`: Build and test infrastructure for MLflow
- [ ] `area/docs`: MLflow documentation pages
- [ ] `area/examples`: Example code
- [ ] `area/model-registry`: Model Registry service, APIs, and the fluent client calls for Model Registry
- [ ] `area/models`: MLmodel format, model serialization/deserialization, flavors
- [X] `area/projects`: MLproject format, project running backends
- [ ] `area/scoring`: Local serving, model deployment tools, spark UDFs
- [ ] `area/server-infra`: MLflow server, JavaScript dev server
- [ ] `area/tracking`: Tracking Service, tracking client APIs, autologging

Interface 
- [ ] `area/uiux`: Front-end, user experience, JavaScript, plotting
- [ ] `area/docker`: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
- [ ] `area/sqlalchemy`: Use of SQLAlchemy in the Tracking Service or Model Registry
- [ ] `area/windows`: Windows support

Language 
- [ ] `language/r`: R APIs and clients
- [ ] `language/java`: Java APIs and clients
- [ ] `language/new`: Proposals for new client languages

Integrations
- [ ] `integrations/azure`: Azure and Azure ML integrations
- [ ] `integrations/sagemaker`: SageMaker integrations
- [ ] `integrations/databricks`: Databricks integrations

### How should the PR be classified in the release notes? Choose one:

- [ ] `rn/breaking-change` - The PR will be mentioned in the "Breaking Changes" section
- [ ] `rn/none` - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
- [ ] `rn/feature` - A new user-facing feature worth mentioning in the release notes
- [X] `rn/bug-fix` - A user-facing bug fix worth mentioning in the release notes
- [ ] `rn/documentation` - A user-facing documentation change worth mentioning in the release notes


Problem statement:
[BUG] mlflow run won't accept parameters' value containing "="
### Willingness to contribute
The MLflow Community encourages bug fix contributions. Would you or another member of your organization be willing to contribute a fix for this bug to the MLflow code base?

- [x] Yes. I can contribute a fix for this bug independently.
- [ ] Yes. I would be willing to contribute a fix for this bug with guidance from the MLflow community.
- [ ] No. I cannot contribute a bug fix at this time.

### System information
- **Have I written custom code (as opposed to using a stock example script provided in MLflow)**: Yes I do
- **OS Platform and Distribution (e.g., Linux Ubuntu 16.04)**: Manjaro Linux 20.1
- **MLflow installed from (source or binary)**: binary
- **MLflow version (run ``mlflow --version``)**: mlflow, version 1.10.0
- **Python version**: Python 3.7.7
- **npm version, if running the dev UI**: N/A
- **Exact command to reproduce**: `mlflow run . -P equation="1 + 2 = 3"`

### Describe the problem
Command `mlflow run` doesn't parse parameters correctly. It fails when the value of a parameter contains "=".

### Code to reproduce issue
Here I have an `MLproject` file
```yaml
entry_points:
  main:
    parameters:
      equation: string
    command: python equation-parser.py {equation}
```
and a python code `equation-parser.py`.
```python
import sys

print('got equation:', sys.argv[1])
```

Then I run the following commands
```sh
$ mlflow run . -P equation="1 + 2 = 3"
Invalid format for -P parameter: 'equation=1 + 2 = 3'. Use -P name=value.
$ mlflow run . -P "equation=1 + 2 = 3"
Invalid format for -P parameter: 'equation=1 + 2 = 3'. Use -P name=value.
```

### Other info / logs
N/A

### What component(s), interfaces, languages, and integrations does this bug affect?
Components 
- [ ] `area/artifacts`: Artifact stores and artifact logging
- [ ] `area/build`: Build and test infrastructure for MLflow
- [ ] `area/docs`: MLflow documentation pages
- [ ] `area/examples`: Example code
- [ ] `area/model-registry`: Model Registry service, APIs, and the fluent client calls for Model Registry
- [ ] `area/models`: MLmodel format, model serialization/deserialization, flavors
- [x] `area/projects`: MLproject format, project running backends
- [ ] `area/scoring`: Local serving, model deployment tools, spark UDFs
- [ ] `area/server-infra`: MLflow server, JavaScript dev server
- [ ] `area/tracking`: Tracking Service, tracking client APIs, autologging

Interface 
- [ ] `area/uiux`: Front-end, user experience, JavaScript, plotting
- [ ] `area/docker`: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
- [ ] `area/sqlalchemy`: Use of SQLAlchemy in the Tracking Service or Model Registry
- [ ] `area/windows`: Windows support

Language 
- [ ] `language/r`: R APIs and clients
- [ ] `language/java`: Java APIs and clients
- [ ] `language/new`: Proposals for new client languages

Integrations
- [ ] `integrations/azure`: Azure and Azure ML integrations
- [ ] `integrations/sagemaker`: SageMaker integrations
- [ ] `integrations/databricks`: Databricks integrations

Hints from issue discussion:
@alpancs Thanks for filing this issue. 

With MLproject file: 
```
entry_points:
  main:
    parameters:
      equation: {type: string, default: "y=mx+c"}
    command: python equation-parser.py --equation {equation}
```
and the equation_parser.py:
```
import sys

if __name__ == "__main__":
   equation = sys.argv[1] if len(sys.argv)  > 1 else "y=mx+c"
   print(f"equation={equation}")
```

`mlflow run . -e main -P equation="2mx+c"`         # <- seems to work
`mlflow run . -e main`                                                     # <- seems to work
`mlflow run . -e main -P equation="y=2mx+c"`   # <- gives you the error

Invalid format for -P parameter: 'equation=y=2mx+c'. Use -P name=value.

Curious, what's the use case where you want an expression with an equality sign in the `-P <name>=<value=expression>`?

cc: @juntai-zheng @smurching is this something that `mlflow run .... -P<name>=<expression>` should support? It's conceivable that you may use an expression to calculate a parameter to be used for training.  
> Curious, what's the use case where you want an expression with an equality sign in the -P <name>=<value=expression>?

@dmatrix: for instance, I need to pass an URI that points to a table's snapshot in a data warehouse, e.g.
```
gs://some-bucket/some-db/some-table/snapshot/date=2020-08-27
```

Actually it is valid on mlflow 1.9.1, but invalid on mlflow 1.10.0.
@alpancs Thanks for pointing this out. This is a regression in 1.10.0 introduced by #3061. I've filed #3347 to fix it; this will be merged and included in the next release of MLflow (slated for release next week). For now, you can either leverage MLflow 1.9.1 or check out the PR branch and install MLflow from there (https://github.com/dbczumar/mlflow/tree/param_fix). Sorry for the inconvenience that this may have caused!

Commit message: Black formatting

Signed-off-by: Corey Zumar <corey.zumar@databricks.com>

Patch to review:
diff --git a/mlflow/cli.py b/mlflow/cli.py
index 1e85c638addfc..2e72eb89a90a7 100644
--- a/mlflow/cli.py
+++ b/mlflow/cli.py
@@ -194,14 +194,14 @@ def run(
 def _user_args_to_dict(arguments, argument_type="P"):
     user_dict = {}
     for arg in arguments:
-        split = arg.split("=")
+        split_index = arg.find("=")
         # Docker arguments such as `t` don't require a value -> set to True if specified
-        if len(split) == 1 and argument_type == "A":
-            name = split[0]
+        if split_index == -1 and argument_type == "A":
+            name = arg[:split_index]
             value = True
-        elif len(split) == 2:
-            name = split[0]
-            value = split[1]
+        elif split_index >= 0:
+            name = arg[:split_index]
+            value = arg[split_index + 1 :]
         else:
             eprint(
                 "Invalid format for -%s parameter: '%s'. "
diff --git a/tests/projects/test_projects_cli.py b/tests/projects/test_projects_cli.py
index 09a50029a5657..1478829aa7b60 100644
--- a/tests/projects/test_projects_cli.py
+++ b/tests/projects/test_projects_cli.py
@@ -23,9 +23,9 @@
 
 
 @pytest.mark.large
-def test_run_local_params():
+@pytest.mark.parametrize("name", ["friend", "friend=you", "='friend'"])
+def test_run_local_params(name):
     excitement_arg = 2
-    name = "friend"
     invoke_cli_runner(
         cli.run,
         [


Code review comments:
diff_hunk: @@ -194,14 +194,14 @@ def run(
 def _user_args_to_dict(arguments, argument_type="P"):
     user_dict = {}
     for arg in arguments:
-        split = arg.split("=")
+        split_index = arg.find("=")
         # Docker arguments such as `t` don't require a value -> set to True if specified
-        if len(split) == 1 and argument_type == "A":
-            name = split[0]
+        if split_index == -1 and argument_type == "A":
+            name = arg[:split_index]
path: mlflow/cli.py
text: @user1:
Should the name just be equal to `arg` here if there's no equals sign? It looks like we chop off the last character here, e.g:

```
>>> arg = "squid"
>>> split_index = arg.find("=")
>>> split_index
-1
>>> name = arg[:split_index]
>>> name
'squi'
```

Is that intentional?

@author:
Thanks so much for catching this. I'd never want to chop the tail off a `squid`. Fixed based on @user2 's suggestion!

@author:
Added test coverage for `-A` arguments as well to `test_projects_cli`

diff_hunk: @@ -194,14 +194,14 @@ def run(
 def _user_args_to_dict(arguments, argument_type="P"):
     user_dict = {}
     for arg in arguments:
-        split = arg.split("=")
+        split_index = arg.find("=")
         # Docker arguments such as `t` don't require a value -> set to True if specified
-        if len(split) == 1 and argument_type == "A":
-            name = split[0]
+        if split_index == -1 and argument_type == "A":
+            name = arg[:split_index]
             value = True
-        elif len(split) == 2:
-            name = split[0]
-            value = split[1]
+        elif split_index >= 0:
+            name = arg[:split_index]
+            value = arg[split_index + 1 :]
path: mlflow/cli.py
text: @user1:
Can we use `maxsplit` here ([ref](https://docs.python.org/ja/3/library/stdtypes.html?highlight=split#str.split))?

```python
for arg in arguments:
  split = arg.split("=", maxsplit=1)

  if len(split) == 1 and argument_type == "A":
    name = split[0]
    value = True
  elif len(split) == 2:
    name, value = split
  ...
```

@author:
This is awesome! I've applied `maxsplit` in place of the previous solution and added test coverage.