Stabilizing methods in Python Models modules

[sueann]

What changes are proposed in this pull request?

To make the methods in Models modules stable in MLflow 1.0, we want to future-proof them against any potential changes in the underlying frameworks (e.g. sklearn, spark, tensorflow). To do so, here we propose to make some methods take in keyword arguments only, by introducing a keyword_only decorator that enforces such.

The change is breaking for any current usages of the APIs, so here we recommend changing only the TensorFlow APIs which may need to change in the near future:

def log_model(tf_saved_model_dir, tf_meta_graph_tags, tf_signature_def_key, artifact_path, conda_env=None)
def save_model(tf_saved_model_dir, tf_meta_graph_tags, tf_signature_def_key, path, mlflow_model=Model(), conda_env=None)

Alternatively, we could not put any restrictions and create new modules (e.g. tensorflow2) in the future as needed.

If we go with the keyword_only decorator route, the following also may merit the decorator if we think the requirement for a sample_df in mleap will change in the future:

mlflow.mleap.log_model(spark_model, sample_input, artifact_path)
mlflow.mleap.save_model(spark_model, sample_input, path, mlflow_model=Model())

but the current PR does not mark them as so.

How is this patch tested?

Existing & new unit tests

pytest --large tests/tensorflow/test_tensorflow_model_export.py

Release Notes

Is this a user-facing change?

• No. You can skip the rest of this section.
• Yes. Give a description of this change to be included in the release notes for MLflow users.

We now require the arguments of save_model and log_model methods in the tensorflow and mleap modules to be keyword arguments. This is to future-proof their usages against API changes involving the TensorFlow model specification.

What component(s) does this PR affect?

• API
• Models
• Python

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

[sueann]

[aarondav]

Cool!

[aarondav]

Error from providing only a subset:

log_model() missing 3 required positional arguments: 'tf_saved_model_dir', 'tf_signature_def_key', and 'artifact_path'

seems fine!

[sueann]

Yeah we could make them optional and put validation inside the methods for them if this turns out to be confusing, but I think it is okay for now.

[sueann]

-> requires

[sueann]

let's add keyword_only to mleap as well

[sueann]

@dbczumar I think you mentioned these might be used in sagemaker container? i didn't see them used anywhere actually - let me know if I'm missing something (hopefully the tests will tell me if I am but just in case)

[dbczumar]

add_to_model is used in mlflow.spark.save_model

[sueann]

right, I wasn't going to keyword_only add_to_model since the sample_df param is the last one. i guess we might as well... :-/