Use testing mode to raise errors during automatic signature inference #8866

jerrylian-db · 2023-06-27T22:55:18Z

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

The automatic signature inference feature swallows failures. However, this makes it difficult to for contributors to debug during testing. This PR uses the MLFLOW_TESTING environment variable to disable failure swallowing of this feature in MLflow tests.

How is this patch tested?

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

I manually added an exception to the signature inference code and verified that the exception was raised in an MLflow test. I manually removed the monkeypatch in that test suite, and the exception was no longer raised (instead the test failed because there was no inferred signature, as expected).

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.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)

What component(s), interfaces, languages, and integrations does this PR affect?

Components

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

Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

mlflow-automation · 2023-06-27T22:55:35Z

Documentation preview for b21e104 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/5418137000.

harupy · 2023-06-28T06:23:49Z

@jerrylian-db Can we use MLFLOW_TESTING instead? See #8869

Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

jerrylian-db · 2023-06-28T15:29:26Z

@harupy great idea! Done :)

Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

harupy · 2023-06-28T23:33:12Z

tests/gluon/test_gluon_model_export.py

+@pytest.fixture(autouse=True)
+def set_envs(monkeypatch):
+    monkeypatch.setenvs(
+        {
+            "MLFLOW_TESTING": "true",
+        }
+    )


We can remove this because the top-level conftest.py already has it:

mlflow/conftest.py

Lines 144 to 148 in 152063e

@pytest.fixture(scope="session", autouse=True)

def enable_mlflow_testing():

with pytest.MonkeyPatch.context() as mp:

mp.setenv(_MLFLOW_TESTING.name, "TRUE")

yield

Huh, so we can remove this from all individual test suites? If I remove it from the test suites, will running those test suites on their own have this environment variable set?

Yes :) You can test it to confirm.

mlflow/spark.py

harupy

LGTM once #8866 (comment) is addressed!

Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

tests/models/test_model_input_examples.py

Co-authored-by: Harutaka Kawamura <hkawamura0130@gmail.com> Signed-off-by: Jerry Liang <66143562+jerrylian-db@users.noreply.github.com>

…mlflow#8866) * implement it Signed-off-by: Jerry Liang <jerry.liang@databricks.com> * change environment variable Signed-off-by: Jerry Liang <jerry.liang@databricks.com> * fix test Signed-off-by: Jerry Liang <jerry.liang@databricks.com> * adapt feedback Signed-off-by: Jerry Liang <jerry.liang@databricks.com> --------- Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

implement it

7a49868

Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

jerrylian-db self-assigned this Jun 27, 2023

github-actions bot added area/models MLmodel format, model serialization/deserialization, flavors rn/none List under Small Changes in Changelogs. labels Jun 27, 2023

jerrylian-db changed the title ~~Create testing mode for signature automatic inference that raises errors when set to true~~ Create testing mode for automatic signature inference that raises errors when set to true Jun 27, 2023

jerrylian-db requested a review from harupy June 27, 2023 23:26

change environment variable

2d8518b

Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

jerrylian-db changed the title ~~Create testing mode for automatic signature inference that raises errors when set to true~~ Use testing mode to raise errors during automatic signature inference Jun 28, 2023

jerrylian-db added 2 commits June 28, 2023 08:30

Merge branch 'master' into signature_testing_mode

08cb4b2

fix test

9668468

Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

harupy reviewed Jun 28, 2023

View reviewed changes

mlflow/spark.py Outdated Show resolved Hide resolved

harupy approved these changes Jun 29, 2023

View reviewed changes

adapt feedback

abf556b

Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

jerrylian-db enabled auto-merge (squash) June 29, 2023 05:58

jerrylian-db disabled auto-merge June 29, 2023 06:24

Merge branch 'master' into signature_testing_mode

7a3237d

jerrylian-db enabled auto-merge (squash) June 29, 2023 17:33

jerrylian-db added 2 commits June 29, 2023 14:27

fix

c1e595b

Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

nit

eb2bf28

Signed-off-by: Jerry Liang <jerry.liang@databricks.com>

harupy reviewed Jun 30, 2023

View reviewed changes

tests/models/test_model_input_examples.py Outdated Show resolved Hide resolved

Update tests/models/test_model_input_examples.py

b21e104

Co-authored-by: Harutaka Kawamura <hkawamura0130@gmail.com> Signed-off-by: Jerry Liang <66143562+jerrylian-db@users.noreply.github.com>

jerrylian-db merged commit 5f6542a into master Jun 30, 2023

jerrylian-db deleted the signature_testing_mode branch July 11, 2023 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use testing mode to raise errors during automatic signature inference #8866

Use testing mode to raise errors during automatic signature inference #8866

jerrylian-db commented Jun 27, 2023 •

edited

Loading

mlflow-automation commented Jun 27, 2023 •

edited

Loading

harupy commented Jun 28, 2023

jerrylian-db commented Jun 28, 2023

harupy Jun 28, 2023

jerrylian-db Jun 29, 2023 •

edited

Loading

harupy Jun 29, 2023 •

edited

Loading

harupy left a comment

	@pytest.fixture(scope="session", autouse=True)
	def enable_mlflow_testing():
	with pytest.MonkeyPatch.context() as mp:
	mp.setenv(_MLFLOW_TESTING.name, "TRUE")
	yield

Use testing mode to raise errors during automatic signature inference #8866

Use testing mode to raise errors during automatic signature inference #8866

Conversation

jerrylian-db commented Jun 27, 2023 • edited Loading

Related Issues/PRs

What changes are proposed in this pull request?

How is this patch tested?

Does this PR change the documentation?

Release Notes

Is this a user-facing change?

What component(s), interfaces, languages, and integrations does this PR affect?

How should the PR be classified in the release notes? Choose one:

mlflow-automation commented Jun 27, 2023 • edited Loading

harupy commented Jun 28, 2023

jerrylian-db commented Jun 28, 2023

harupy Jun 28, 2023

Choose a reason for hiding this comment

jerrylian-db Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

harupy Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

harupy left a comment

Choose a reason for hiding this comment

jerrylian-db commented Jun 27, 2023 •

edited

Loading

mlflow-automation commented Jun 27, 2023 •

edited

Loading

jerrylian-db Jun 29, 2023 •

edited

Loading

harupy Jun 29, 2023 •

edited

Loading