initpy and test_transformers_model_export #10538

KonakanchiSwathi · 2023-11-29T17:35:52Z

🛠 DevTools 🛠

Install mlflow from this PR

pip install git+https://github.com/mlflow/mlflow.git@refs/pull/10538/merge

Checkout with GitHub CLI

gh pr checkout 10538

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

How is this PR tested?

Existing unit/integration tests
New unit/integration tests
Manual tests

Does this PR require documentation update?

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.

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/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/breaking-change - The PR will be mentioned in the "Breaking Changes" 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

github-actions · 2023-11-29T17:36:14Z

Documentation preview for 3973ffb 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/7290974839.

Signed-off-by: swathi <konakanchi.swathi@gmail.com>

KonakanchiSwathi · 2023-11-29T18:52:20Z

@BenWilson2 @serena-ruan Please review

BenWilson2 · 2023-12-04T20:07:01Z

tests/transformers/test_transformers_model_export.py

+    assert len(predictions) != 0
+
+
+@pytest.mark.skipif(RUNNING_IN_GITHUB_ACTIONS, reason=GITHUB_ACTIONS_SKIP_REASON)


I think this line was added by mistake. Can you remove this please?

@BenWilson2 Please review

BenWilson2 · 2023-12-04T20:09:20Z

Looking good! Once that typo is fixed, we can merge! :D

removed line @pytest.mark.skipif(RUNNING_IN_GITHUB_ACTIONS, reason=GITHUB_ACTIONS_SKIP_REASON)

…/KonakanchiSwathi/mlflow into AddImageclassification_newbranch

removed line @pytest.mark.skipif(RUNNING_IN_GITHUB_ACTIONS, reason=GITHUB_ACTIONS_SKIP_REASON)

…/KonakanchiSwathi/mlflow into AddImageclassification_newbranch

BenWilson2 · 2023-12-14T17:55:20Z

mlflow/transformers/__init__.py

@@ -2669,7 +2733,7 @@ def decode_audio(encoded):
    @staticmethod
    def _validate_str_input_uri_or_file(input_str):
        """
-        Validation of blob references to audio files, if a string is input to the ``predict``
+        Validation of blob references to audio/image files, if a string is input to the ``predict``


Suggested change

Validation of blob references to audio/image files, if a string is input to the ``predict``

Validation of blob references to either audio or image files, if a string is input to the ``predict``

Let's not use shorthand in docstrings

BenWilson2 · 2023-12-14T17:59:37Z

tests/transformers/test_transformers_model_export.py

+    with model_path.joinpath("requirements.txt").open() as file:
+        requirements = file.read()


Suggested change

with model_path.joinpath("requirements.txt").open() as file:

requirements = file.read()

requirements = model_path.joinpath("requirements.txt").read_text()

BenWilson2 · 2023-12-14T18:01:25Z

tests/transformers/test_transformers_model_export.py

+    with model_path.joinpath("model_card.md").open(encoding="utf-8") as file:
+        card_text = file.read()


Suggested change

with model_path.joinpath("model_card.md").open(encoding="utf-8") as file:

card_text = file.read()

card_text = model_path.joinpath("model_card.md").read_text(encoding="utf-8")

BenWilson2 · 2023-12-14T18:03:25Z

tests/transformers/test_transformers_model_export.py

@@ -1341,6 +1363,46 @@ def test_qa_pipeline_pyfunc_load_and_infer(small_qa_pipeline, model_path, infere
    assert all(isinstance(element, str) for element in inference)


+def read_image(filename):
+    image_path = os.path.join(pathlib.Path(__file__).parent.parent, "datasets", filename)


Why are we mixing pathlib.Path and os.path.join() ? Just use pathlib to traverse the paths and read the bytes of the file directly.

BenWilson2 · 2023-12-14T18:04:36Z

tests/transformers/test_transformers_model_export.py

+    "inference_payload",
+    [
+        image_url,
+        os.path.join(pathlib.Path(__file__).parent.parent, "datasets", "cat.png"),


Does an absolute path reference from the repo root not work here? Why is this evaluation path parsing logic in the parameter?

BenWilson2 · 2023-12-14T18:07:59Z

tests/transformers/test_transformers_model_export.py

+    "inference_payload",
+    [
+        [os.path.join(pathlib.Path(__file__).parent.parent, "datasets", "cat.png")],
+        [image_url, image_url],


Why two instances here?

wanted to try the list with more than one, but changed it one instance

BenWilson2 · 2023-12-14T18:09:25Z

tests/transformers/test_transformers_model_export.py

+    ],
+)
+def test_vision_pipeline_pyfunc_predict(small_vision_model, inference_payload):
+    if inference_payload == "base64":


Let's not embed complex logic like this that mutates the parameter based on string matching. Just create another test explicitly for this condition.

well, moved this logic inside because if we do this as an input parameter a lengthy base64 string is being printed in the test suite. now we have to duplicate three tests if we want to create a new test for base64.

BenWilson2 · 2023-12-14T18:10:46Z

tests/transformers/test_transformers_model_export.py

+    transformers_loaded_model = mlflow.transformers.load_model(model_uri)
+    expected_predictions = transformers_loaded_model.predict(inference_payload)
+
+    assert list(predictions.to_dict("records")[0].values()) == expected_predictions[0]


Why are we only validating the first entry here?

fixed, thanks

Signed-off-by: Madhu <madhukesav02@gmail.com>

…/KonakanchiSwathi/mlflow into AddImageclassification_newbranch

MadhuM02 · 2023-12-19T02:31:08Z

Hi @serena-ruan , @BenWilson2 Thanks for reviewing the PR and giving us valuable feedback. IT's been quite some time since this effort has started. can we sort of connect and close this one out??

tests/transformers/test_transformers_model_export.py

serena-ruan · 2023-12-20T10:24:39Z

tests/transformers/test_transformers_model_export.py

+        mlflow.transformers.log_model(
+            transformers_model=small_vision_model,
+            artifact_path=artifact_path,
+            signature=infer_signature(
+                image_url,
+                mlflow.transformers.generate_signature_output(small_vision_model, image_url),
+                params=parameters,
+            ),
+        )
+        model_uri = mlflow.get_artifact_uri(artifact_path)


Similar here, let's use model_info.model_uri

serena-ruan

Left few nits. LGTM otherwise. Let's wait for Ben to take another look.

Signed-off-by: swathi <konakanchi.swathi@gmail.com> Co-authored-by: Serena Ruan <82044803+serena-ruan@users.noreply.github.com> Signed-off-by: Konakanchi Swathi <98085410+KonakanchiSwathi@users.noreply.github.com>

Signed-off-by: Konakanchi Swathi <98085410+KonakanchiSwathi@users.noreply.github.com>

…i.swathi@gmail.com>

mlflow/transformers/__init__.py

BenWilson2

LGTM once the test fix (string concatenation typo with a trailing comma) is applied

Signed-off-by: swathi <konakanchi.swathi@gmail.com> Co-authored-by: Ben Wilson <39283302+BenWilson2@users.noreply.github.com> Signed-off-by: Konakanchi Swathi <98085410+KonakanchiSwathi@users.noreply.github.com>

MadhuM02 · 2024-01-18T07:56:28Z

Hi @BenWilson2/ @serena-ruan, is there a tentative date for next mlflow pypi release??

initpy and test_transformers_model_export

cfebd44

Signed-off-by: swathi <konakanchi.swathi@gmail.com>

KonakanchiSwathi force-pushed the AddImageclassification_newbranch branch from b9b4b2f to cfebd44 Compare November 29, 2023 18:16

BenWilson2 reviewed Dec 4, 2023

View reviewed changes

KonakanchiSwathi and others added 9 commits December 6, 2023 09:40

Update test_transformers_model_export.py

1b899c2

removed line @pytest.mark.skipif(RUNNING_IN_GITHUB_ACTIONS, reason=GITHUB_ACTIONS_SKIP_REASON)

Signed-off-by:swathi <konakanchi.swathi@gmail.com>

82d011d

Signed-off-by:swathi <konakanchi.swathi@gmail.com>

97ae2f4

Merge branch 'AddImageclassification_newbranch' of https://github.com…

515c9f2

…/KonakanchiSwathi/mlflow into AddImageclassification_newbranch

Merge branch 'AddImageclassification_newbranch' of https://github.com…

fc6ce63

…/KonakanchiSwathi/mlflow into AddImageclassification_newbranch

Merge branch 'AddImageclassification_newbranch' of https://github.com…

155fd99

…/KonakanchiSwathi/mlflow into AddImageclassification_newbranch

Signed-off-by: swathi <konakanchi.swathi@gmail.com>

b8c51c2

removed line @pytest.mark.skipif(RUNNING_IN_GITHUB_ACTIONS, reason=GITHUB_ACTIONS_SKIP_REASON)

Signed-off-by:swathi <konakanchi.swathi@gmail.com>

9710901

Merge branch 'AddImageclassification_newbranch' of https://github.com…

167d836

…/KonakanchiSwathi/mlflow into AddImageclassification_newbranch

BenWilson2 reviewed Dec 14, 2023

View reviewed changes

Madhu0205 and others added 4 commits December 15, 2023 07:27

update

e5e89bd

Signed-off-by: Madhu <madhukesav02@gmail.com>

Merge branch 'AddImageclassification_newbranch' of https://github.com…

7c555d3

…/KonakanchiSwathi/mlflow into AddImageclassification_newbranch

Signed-off-by: swathi <konakanchi.swathi@gmail.com>

ab9fee7

Signed-off-by: swathi <konakanchi.swathi@gmail.com>

4dbbaf8

KonakanchiSwathi requested a review from BenWilson2 December 18, 2023 02:18

serena-ruan reviewed Dec 20, 2023

View reviewed changes

tests/transformers/test_transformers_model_export.py Outdated Show resolved Hide resolved

serena-ruan reviewed Dec 20, 2023

View reviewed changes

serena-ruan approved these changes Dec 20, 2023

View reviewed changes

KonakanchiSwathi and others added 5 commits December 21, 2023 10:57

Update tests/transformers/test_transformers_model_export.py

a309e45

Signed-off-by: swathi <konakanchi.swathi@gmail.com> Co-authored-by: Serena Ruan <82044803+serena-ruan@users.noreply.github.com> Signed-off-by: Konakanchi Swathi <98085410+KonakanchiSwathi@users.noreply.github.com>

Merge branch 'master' into AddImageclassification_newbranch

be250a6

Signed-off-by: Konakanchi Swathi <98085410+KonakanchiSwathi@users.noreply.github.com>

Added model_uri=model_info.model_uri Signed-off-by: swathi <konakanch…

a44da12

…i.swathi@gmail.com>

Signed-off-by: swathi <konakanchi.swathi@gmail.com>

0b5ab75

Signed-off-by: swathi <konakanchi.swathi@gmail.com>

e73a548

BenWilson2 reviewed Dec 21, 2023

View reviewed changes

mlflow/transformers/__init__.py Outdated Show resolved Hide resolved

BenWilson2 approved these changes Dec 21, 2023

View reviewed changes

Update mlflow/transformers/__init__.py

2c54870

Signed-off-by: swathi <konakanchi.swathi@gmail.com> Co-authored-by: Ben Wilson <39283302+BenWilson2@users.noreply.github.com> Signed-off-by: Konakanchi Swathi <98085410+KonakanchiSwathi@users.noreply.github.com>

KonakanchiSwathi requested review from BenWilson2 and serena-ruan December 21, 2023 16:47

Signed-off-by: "v-swathikon konakanchi.swathi@gmail.com"

3973ffb

BenWilson2 merged commit b929a3e into mlflow:master Dec 21, 2023
59 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initpy and test_transformers_model_export #10538

initpy and test_transformers_model_export #10538

KonakanchiSwathi commented Nov 29, 2023 •

edited

github-actions bot commented Nov 29, 2023 •

edited

KonakanchiSwathi commented Nov 29, 2023

BenWilson2 Dec 4, 2023

KonakanchiSwathi Dec 6, 2023

KonakanchiSwathi Dec 6, 2023

BenWilson2 commented Dec 4, 2023

BenWilson2 Dec 14, 2023

BenWilson2 Dec 14, 2023

BenWilson2 Dec 14, 2023 •

edited

BenWilson2 Dec 14, 2023

MadhuM02 Dec 15, 2023

BenWilson2 Dec 14, 2023

BenWilson2 Dec 14, 2023

MadhuM02 Dec 15, 2023

BenWilson2 Dec 14, 2023

MadhuM02 Dec 15, 2023

BenWilson2 Dec 14, 2023

MadhuM02 Dec 15, 2023

MadhuM02 commented Dec 19, 2023

serena-ruan Dec 20, 2023

serena-ruan left a comment

BenWilson2 left a comment

MadhuM02 commented Jan 18, 2024

		assert len(predictions) != 0


		@pytest.mark.skipif(RUNNING_IN_GITHUB_ACTIONS, reason=GITHUB_ACTIONS_SKIP_REASON)

	Validation of blob references to audio/image files, if a string is input to the ``predict``
	Validation of blob references to either audio or image files, if a string is input to the ``predict``

		with model_path.joinpath("requirements.txt").open() as file:
		requirements = file.read()

	with model_path.joinpath("requirements.txt").open() as file:
	requirements = file.read()
	requirements = model_path.joinpath("requirements.txt").read_text()

		with model_path.joinpath("model_card.md").open(encoding="utf-8") as file:
		card_text = file.read()

	with model_path.joinpath("model_card.md").open(encoding="utf-8") as file:
	card_text = file.read()
	card_text = model_path.joinpath("model_card.md").read_text(encoding="utf-8")

initpy and test_transformers_model_export #10538

initpy and test_transformers_model_export #10538

Conversation

KonakanchiSwathi commented Nov 29, 2023 • edited

Install mlflow from this PR

Checkout with GitHub CLI

Related Issues/PRs

What changes are proposed in this pull request?

How is this PR tested?

Does this PR require documentation update?

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:

github-actions bot commented Nov 29, 2023 • edited

KonakanchiSwathi commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenWilson2 commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenWilson2 Dec 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadhuM02 commented Dec 19, 2023

Choose a reason for hiding this comment

serena-ruan left a comment

Choose a reason for hiding this comment

BenWilson2 left a comment

Choose a reason for hiding this comment

MadhuM02 commented Jan 18, 2024

KonakanchiSwathi commented Nov 29, 2023 •

edited

github-actions bot commented Nov 29, 2023 •

edited

BenWilson2 Dec 14, 2023 •

edited