Enable mlflow-artifacts scheme as wrapper around http artifact scheme by BenWilson2 · Pull Request #5070 · mlflow/mlflow

BenWilson2 · 2021-11-15T19:41:14Z

Signed-off-by: Ben Wilson benjamin.wilson@databricks.com

What changes are proposed in this pull request?

Support mlflow-artifacts: scheme for proxied mlflow server artifact handling functionality.
The implementation is a wrapper around the HTTP artifact repository scheme.
This supports both a 'basic scheme' (user leaving the resolving server address out of the uri) and approaches that retain the host (and/or port) for replacing the scheme with the originating registered uri for the artifact server.

How is this patch tested?

Unit tests

Does this PR change the documentation?

No. You can skip the rest of this section.
Yes. Make sure the changed pages / sections render correctly by following the steps below.

Check the status of the ci/circleci: build_doc check. If it's successful, proceed to the
next step, otherwise fix it.
Click Details on the right to open the job page of CircleCI.
Click the Artifacts tab.
Click docs/build/html/index.html.
Find the changed pages / sections and make sure they render correctly.

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.

Support for mlflow-artifacts scheme added (i.e., mlflow.create_experiment(name="my-experiment", artifact_location="mlflow-artifacts:/root/models/my-experiment-models/model-1"). Defining host, port, or both is also supported.

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: Ben Wilson <benjamin.wilson@databricks.com>

dbczumar · 2021-11-17T20:06:20Z

+
+    def __init__(self, artifact_uri):
+        tracking_uri = get_tracking_uri()
+        resolved_artifacts_uri = artifact_uri.replace("mlflow-artifacts:", f"{tracking_uri}")


We should only perform this replacement if the URI doesn't specify a host. We also need to handle the case where the URI has one or three slashes after the scheme.

Right now, if the URI specifies a host (e.g. mlflow-artifacts://myhostname:5000), the following behavior occurs:

In [11]: MlflowArtifactsRepository("mlflow-artifacts://foo:5050/my/artifact/path").artifact_uri Out[11]: 'http://localhost:5000//foo:5050/my/artifact/path'

Similarly, if a hostname-less mlflow-artifacts URI is specified with one or three slashes after the scheme, the following behaviors occur:

In [14]: mlflow.set_tracking_uri("http://localhost:5000/") In [15]: MlflowArtifactsRepository("mlflow-artifacts:/my/artifact/path").artifact_uri Out[15]: 'http://localhost:5000//my/artifact/path' In [16]: MlflowArtifactsRepository("mlflow-artifacts:///my/artifact/path").artifact_uri Out[16]: 'http://localhost:5000////my/artifact/path'

While the extra slashes in the path shouldn't be harmful, it would be nice to remove them.

To clarify re:hostnames - when a user supplies an artifact URI of the form mlflow-artifacts://<hostname>/<path> or mlflow-artifacts://<hostname>:<port>/<path>, we should resolve the URI to http://<hostname>/api/2.0/mlflow-artifacts/artifacts/<path> or http://<hostname>:<port>/api/2.0/mlflow-artifacts/artifacts/<path> respectively.

Can we add test cases for each of the above scenarios as well?

Some... (not so minor) changes to handle resolving all of the different ways that this uri can be configured. I have a validation check in here for raising an Exception if only a port is passed; I'd really like an opinion on whether this is needed or if it's a silly thing to check for.

Also, should I take those private functions and make them class methods? I don't see those functions being particularly useful outside of this sub class of HttpArtifactRepository.
@dbczumar @harupy any thoughts / advice on these?

I have a validation check in here for raising an Exception if only a port is passed.

This sounds good to me!

should I take those private functions and make them class methods?

I don't have a strong preference between private functions and class methods. The current code looks ok to me.

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy · 2021-11-19T00:45:05Z

+        uri = MlflowArtifactsRepository(  # pylint: disable=unused-variable
+            failing_condition
+        ).artifact_uri


Suggested change

uri = MlflowArtifactsRepository( # pylint: disable=unused-variable

failing_condition

).artifact_uri

MlflowArtifactsRepository(failing_condition)

nit

changed to just the class instantiation

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy · 2021-11-19T10:17:33Z

+
+    @classmethod
+    def resolve_uri(cls, artifact_uri):
+        tracking_uri = get_tracking_uri()


Can we raise an exception if the scheme of tracking_uri is not http or https (e.g. sqlite:///foo/bar)?

great idea :) added! I also simplified the parsing logic and temporarily set the tracking uri in the test function to validate against a valid server address (then reverted the global variable to not affect other tests with an inadvertent side-effect)

Thanks for the update! A tracking URI looks like http://localhost:5000 and doesn't contain /api/2.0/mlflow-artifacts/artifacts.

updated the tests and cleaned all of that up

@BenWilson2 Sorry I wasn't clear. The resolved artifact URI needs to have /api/2.0/mlflow-artifacts/artifacts (e.g. http://localhost:5000/api/2.0/mlflow-artifacts/artifacts/path/to/dir).

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

…i parsing Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

…-artifact-scheme-ml-18423 Updating to new version of black Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

BenWilson2 · 2021-11-19T22:14:53Z

I installed the new version of black after merging master into this branch. Black on local (21.10b0) suggests no changes, but the lint test is failing in CI. @harupy have you seen this since the merge of the black update?

harupy · 2021-11-20T04:10:26Z

@BenWilson2

Does black --version print out 21.10b0?

black --version

harupy · 2021-11-20T04:16:59Z

+            f"mlflow-artifacts://{base_path}/redundant",
+            f"{tracking_uri.scheme}://{tracking_uri.netloc}/{base_api_uri}{base_path}/redundant",
+        ),
+        ("mlflow-artifacts:/", uri_temp,),


Suggested change

("mlflow-artifacts:/", uri_temp,),

("mlflow-artifacts:/", uri_temp,),

@BenWilson2 Removing the command here should fix the lint issue.

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy · 2021-11-22T15:20:19Z

+        print(f"\nuri_parse: {uri_parse}")
+        print(f"track_parse: {track_parse}")


Suggested change

print(f"\nuri_parse: {uri_parse}")

print(f"track_parse: {track_parse}")

deleted my debug ;)

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy

LGTM!

Enable mlflow-artifacts scheme as wrapper around http artifact scheme

9e00fb6

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

BenWilson2 requested review from dbczumar and harupy November 15, 2021 19:41

github-actions Bot added area/artifacts Artifact stores and artifact logging rn/feature Mention under Features in Changelogs. labels Nov 15, 2021

dbczumar reviewed Nov 15, 2021

View reviewed changes

Comment thread mlflow/store/artifact/mlflow_artifacts_repo.py Outdated

BenWilson2 added 2 commits November 15, 2021 16:26

simplify scheme resolution for missing host and port replacement

3463351

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

Add artifact storage path testing params

395bfd9

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

BenWilson2 requested a review from dbczumar November 17, 2021 15:08

dbczumar reviewed Nov 17, 2021

View reviewed changes

Handle uri parsing and test different permutations

b6032a5

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy reviewed Nov 18, 2021

View reviewed changes

Comment thread mlflow/store/artifact/mlflow_artifacts_repo.py Outdated

BenWilson2 requested a review from dbczumar November 18, 2021 03:14

close the session in http_artifact_repo only if it is active

243a62f

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy reviewed Nov 18, 2021

View reviewed changes

Comment thread mlflow/store/artifact/mlflow_artifacts_repo.py Outdated

harupy reviewed Nov 18, 2021

View reviewed changes

Comment thread tests/store/artifact/test_mlflow_artifact_repo.py Outdated

legibility for test conditions

fa3bfa4

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy reviewed Nov 19, 2021

View reviewed changes

Comment thread mlflow/store/artifact/mlflow_artifacts_repo.py Outdated

harupy reviewed Nov 19, 2021

View reviewed changes

lint fixes

c4c4787

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy reviewed Nov 19, 2021

View reviewed changes

BenWilson2 added 3 commits November 19, 2021 13:35

Add tracking uri scheme exception and simplify parsing logic

3b22de9

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

Overrides of local tracking uri in test suite and adjust logic for ur…

cf038ed

…i parsing Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

Merge branch 'master' of https://github.com/mlflow/mlflow into mlflow…

082c7ce

…-artifact-scheme-ml-18423 Updating to new version of black Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy reviewed Nov 20, 2021

View reviewed changes

lint

b8e2af9

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy reviewed Nov 22, 2021

View reviewed changes

Comment thread tests/store/artifact/test_mlflow_artifact_repo.py Outdated

harupy reviewed Nov 22, 2021

View reviewed changes

BenWilson2 added 2 commits November 22, 2021 17:40

Simplify tests

e5f8db1

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

Fix the logic

01fb162

Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy approved these changes Nov 24, 2021

View reviewed changes

BenWilson2 merged commit 776831a into master Nov 24, 2021

BenWilson2 deleted the mlflow-artifact-scheme-ml-18423 branch November 24, 2021 15:27

harupy mentioned this pull request Nov 29, 2021

Add server option for serving only artifacts and proxied serving mode #5045

Merged

29 tasks

	("mlflow-artifacts:/", uri_temp,),
	("mlflow-artifacts:/", uri_temp,),

		print(f"\nuri_parse: {uri_parse}")
		print(f"track_parse: {track_parse}")

Conversation

BenWilson2 commented Nov 15, 2021

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!

Uh oh!

dbczumar Nov 17, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

harupy Nov 18, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy Nov 22, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

harupy Nov 22, 2021 • 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 commented Nov 19, 2021

Uh oh!

harupy commented Nov 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

3 participants

dbczumar Nov 17, 2021 •

edited

Loading

harupy Nov 18, 2021 •

edited

Loading

harupy Nov 22, 2021 •

edited

Loading

harupy Nov 22, 2021 •

edited

Loading