Fix downloading MLmodel files from alias-based models:/ URIs by smurching · Pull Request #8764 · mlflow/mlflow

smurching · 2023-06-15T22:22:18Z

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Follow-up to #8728, this PR fixes downloading MLmodel files when loading models with URIs like models:/mymodel@alias. The current logic for this attempts to append the MLmodel file path to the URI and then download from an artifact URI like models:/mymodel@alias/MLmodel, which is not a valid URI and cannot be parsed.

How is this patch tested?

Updated unit tests. Also manually verified the PR fixes a bug in a DAIS demo 😅

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

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: Sid Murching <[email protected]>

smurching · 2023-06-15T22:25:21Z

mlflow/utils/model_utils.py

            )
-            ml_model_file = _download_artifact_from_uri(
-                artifact_uri=append_to_uri_path(model_uri, MLMODEL_FILE_NAME)
+            ml_model_file = get_artifact_repository(artifact_uri=model_uri).download_artifacts(


Instead of assuming that we can append /MLmodel to an artifact URI to get a valid artifact URI (which is not generally true), we instead construct an ArtifactRepository against the original artifact URI (e.g. models:/mymodels@alias) and then call download_artifacts with the actual file path (MLModel). We should make similar fixes elsewhere, I'll do that in a follow-up PR

Actually - another alternative to this could be trying to support URIs like models:/mymodel@alias/path/to/file. Technically this is possible since aliases can only contain alphanumeric characters, would reduce the set of changes needed in the immediate-term, and would allow for things like mlflow.artifacts.download_artifacts(artifact_uri="models:/mymodel@alias/path/to/file"). @harupy I saw you made a similar fix in #5921

The main downside I think is that there may be artifact store types for which appending a /path/to/file at the end of the URI isn't valid (e.g. any artifact store where there are query parameters in the URI). But we could deal with that if/when we need to

Hm, not sure my suggestion will work. The problem is that we don't restrict the set of allowed characters in registered model names in OSS. That means that a URI like models:/mymodel@myalias/1/to/file could ambiguously be interpreted as (1) targeting the file at path 1/to/file under the version of mymodel associated with the alias myalias or (2) targeting the model named mymodel@alias, version 1, file path /to/file. So unfortunately I think we can't support this style of URI for aliases and should adopt the fix in this PR to download specific files from model versions referenced by alias

It seems the safer option. Just checked cloud provider docs for object store and they really don't have many restrictions on key contents anyway.

smurching · 2023-06-15T22:26:16Z

mlflow/store/artifact/utils/models.py

 def _improper_model_uri_msg(uri):
    return (
        "Not a proper models:/ URI: %s. " % uri
-        + "Models URIs must be of the form 'models:/<model_name>/<suffix>' "


This error message doesn't render well in notebooks, since <model_name> is rendered/escaped as an HTML tag; fix it by removing the angle brackets

I didn't know that, makes sense!

lol (looks like a bug, not MLflow's)

We should probably look for other cases of this.... I had no idea that it would parse as a tag

Found another trick:

Signed-off-by: Sid Murching <[email protected]>

mlflow-automation · 2023-06-15T23:43:46Z

Documentation preview for 4bbdd07 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/5284929548.

Signed-off-by: Sid Murching <[email protected]>

BenWilson2

LGTM!

harupy

LGTM!

…8764) * Fix downloading MLmodel files from alias URIs Signed-off-by: Sid Murching <[email protected]> * Fix test Signed-off-by: Sid Murching <[email protected]> * use variable Signed-off-by: Sid Murching <[email protected]> --------- Signed-off-by: Sid Murching <[email protected]>

* Fix downloading MLmodel files from alias URIs Signed-off-by: Sid Murching <[email protected]> * Fix test Signed-off-by: Sid Murching <[email protected]> * use variable Signed-off-by: Sid Murching <[email protected]> --------- Signed-off-by: Sid Murching <[email protected]>

Fix downloading MLmodel files from alias URIs

10258a0

Signed-off-by: Sid Murching <[email protected]>

smurching commented Jun 15, 2023

View reviewed changes

Fix test

4f7e3a0

Signed-off-by: Sid Murching <[email protected]>

smurching requested review from BenWilson2, arpitjasa-db and harupy June 15, 2023 22:33

smurching added the rn/none List under Small Changes in Changelogs. label Jun 16, 2023

use variable

4bbdd07

Signed-off-by: Sid Murching <[email protected]>

BenWilson2 approved these changes Jun 16, 2023

View reviewed changes

harupy approved these changes Jun 16, 2023

View reviewed changes

smurching merged commit 6c1b84b into mlflow:master Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix downloading MLmodel files from alias-based models:/ URIs#8764

Fix downloading MLmodel files from alias-based models:/ URIs#8764
smurching merged 3 commits intomlflow:masterfrom
smurching:fix-mlmodel-file-download

smurching commented Jun 15, 2023 •

edited

Loading

Uh oh!

smurching Jun 15, 2023 •

edited

Loading

Uh oh!

smurching Jun 16, 2023

Uh oh!

smurching Jun 16, 2023 •

edited

Loading

Uh oh!

BenWilson2 Jun 16, 2023

Uh oh!

smurching Jun 15, 2023

Uh oh!

harupy Jun 15, 2023 •

edited

Loading

Uh oh!

harupy Jun 15, 2023 •

edited

Loading

Uh oh!

BenWilson2 Jun 16, 2023

Uh oh!

harupy Jun 16, 2023 •

edited

Loading

Uh oh!

mlflow-automation commented Jun 15, 2023 •

edited

Loading

Uh oh!

BenWilson2 left a comment

Uh oh!

harupy left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

smurching commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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:

Uh oh!

smurching Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smurching Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

smurching Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenWilson2 Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

smurching Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

harupy Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenWilson2 Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

harupy Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlflow-automation commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenWilson2 left a comment

Choose a reason for hiding this comment

Uh oh!

harupy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smurching commented Jun 15, 2023 •

edited

Loading

smurching Jun 15, 2023 •

edited

Loading

smurching Jun 16, 2023 •

edited

Loading

harupy Jun 15, 2023 •

edited

Loading

harupy Jun 15, 2023 •

edited

Loading

harupy Jun 16, 2023 •

edited

Loading

mlflow-automation commented Jun 15, 2023 •

edited

Loading