Raise an exception only if param value changed

[AdamStelmaszczyk]

Related Issues/PRs

Similar PR, but for _log_params: #7057

What changes are proposed in this pull request?

MlflowException("Changing param values is not allowed..." shouldn't be raised if the values didn't change - but that wasn't the case as this was not checked. The change is adding an if statement checking whether the param value actually changed.

How is this patch tested?

• Existing unit/integration tests
• New unit/integration tests
• Manual tests (describe details, including test results, below)

To reproduce the issue, create the client.py file:

import mlflow
import threading

ITERATIONS = 1000

def log_param():
	for _ in range(ITERATIONS):
		mlflow.log_param('key', 'val')

mlflow.set_tracking_uri('http://localhost:5000')
with mlflow.start_run():
	t1 = threading.Thread(target=log_param)
	t2 = threading.Thread(target=log_param)
	t1.start()
	t2.start()
	t1.join()
	t2.join()

Let's also create a venv:

$ python3.8 -m venv mlflow
$ source mlflow/bin/activate
(mlflow) $ pip install mlflow==2.2.2

And run the server in that venv:

(mlflow) $ mlflow server --backend-store-uri=sqlite:///mlflow.db

Keep that console tab open. In a new one, run client.py in our venv:

$ source mlflow/bin/activate
(mlflow) $ python3.8 client.py

After some time you should see INVALID_PARAMETER_VALUE exception. If not, re-run the script. Usually it takes only a couple of runs to trigger the exception.

After applying the proposed patch, you won't see the exception.

In our system, log_param was called from kedro pipeline, specifically https://github.com/Galileo-Galilei/kedro-mlflow calls it in before_node_run hook. So sometimes we saw the whole pipeline failing because of INVALID_PARAMETER_VALUE (and param values didn't change).

The logic now will be consistent with _log_params (that log_batch calls): https://github.com/AdamStelmaszczyk/mlflow/blob/4d1653da42c33ee9e89af7ec649bd2498895377a/mlflow/store/tracking/sqlalchemy_store.py#L983 which was fixed in #7057.

Does this PR change the documentation?

• No. You can skip the rest of this section.
• Yes. Make sure the changed pages / sections render correctly in the documentation preview.

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.

SqlAlchemyStore.log_param() can now be called multiple times (and from multiple threads) with the same param values.

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
• area/recipes: Recipes, Recipe APIs, Recipe configs, Recipe Templates
• area/projects: MLproject format, project running backends
• area/scoring: MLflow Model server, model deployment tools, Spark UDFs
• area/server-infra: MLflow Tracking server backend
• area/tracking: Tracking Service, tracking client APIs, autologging

Interface

• area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
• 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
• 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

[mlflow-automation]

Documentation preview for 4d1653d will be available here when this CircleCI job completes successfully.

More info

• Ignore this comment if this PR does not change the documentation.
• It takes a few minutes for the preview to be available.
• The preview is updated when a new commit is pushed to this PR.
• This comment was created by https://github.com/mlflow/mlflow/actions/runs/4427234912.

[harupy]

@AdamStelmaszczyk Thanks for the PR! Can you add a test?

[AdamStelmaszczyk]

Unfortunately, I don't know how to test this with 100% repeatability to have an automatic test. One needs at least 2 threads performing DB inserts in exactly same time, so it's probabilistic. That's why I wrote client.py which shows the bug (exception thrown) let's say ~80% of time. So with manual testing it's fine, one can try several times client.py - but it doesn't seem suitable for an automatic test.

[harupy]

@AdamStelmaszczyk Understood, thanks for the explanation! I thought just logging the same parameter twice can reproduce the issue, but concurrent write is needed to reproduce. I was able to reproduce the issue using the attached code.

[harupy]

LGTM!

[dbczumar]

LGTM! Thanks @AdamStelmaszczyk !