Add server option for serving only artifacts and proxied serving mode by BenWilson2 · Pull Request #5045 · mlflow/mlflow

BenWilson2 · 2021-11-10T21:00:00Z

What changes are proposed in this pull request?

Add the flags:
--serve-artifact-opt to enable starting an MLflow server for artifact serving purposes
--artifacts-only to enable artifact serving as an exclusive option to an MLflow server instance, disabling all other endpoints in the tracking service (/api/2.0/mlflow/* disabled, /api/2.0/mlflow-artifacts/* enabled only).

This is a follow-on to 4946.

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.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)
Add capability to enable or disable mlflow-artifact functionality for the mlflow server, as well as the ability to enforce exclusively functionality of artifact handling for an mlflow server instance.

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>

…t-serve-artifacts Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

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

…t-serve-artifacts Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy · 2021-11-11T09:47:37Z

+    return wrapper
+
+
+def _disable_mlflow_artifacts_only(func):


Suggested change

def _disable_mlflow_artifacts_only(func):

def _disable_if_artifacts_only(func):

Can we also rename this function to indicate that the decorated endpoint is disabled if --artifacts-only is specified?

much more clear decorator name. changed

@BenWilson2 It seems it's not changed yet :)

…t-serve-artifacts Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

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

harupy · 2021-11-12T11:09:38Z

+            "false",
+            "false",


Suggested change

"false",

"false",

False,

False,

Should these be False?

It definitely should. I forgot about how Python will return a bool validation on a string of "true" or "false" the same as the bool operator.

harupy · 2021-11-12T11:09:58Z

+    "by routing these requests to the storage location that is specified by "
+    "'--artifact-destination' directly through a proxy. The default location that "
+    "these requests are served from is a local './mlartifacts' directory which can be "
+    "overridden via '--artifact-destination' arguments. "


Suggested change

"overridden via '--artifact-destination' arguments. "

"overridden via '--artifacts-destination' argument. "

nit

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

dbczumar

@BenWilson2 This is awesome! One point of feedback I noticed while manual QAing is that, when --serve-artifacts is specified, --default-artifact-root still defaults to ./mlruns. For artifact serving, it should default to an mlflow-artifacts:/ URI. Is support for mlflow-artifacts URIs coming in a separate PR? If so, let's merge that one first.

Once we change the default behavior, can we update the documentation for the --default-artifact-root option? Even for the existing behavior with file stores, the mlflow server --help output is pretty confusing and has some English syntax errors:

  --default-artifact-root URI  Local or S3 URI to store
                               artifacts, for new experiments.
                               Note that this flag does not
                               impact already-created
                               experiments. Default: Within
                               file store, if a file:/ URI is
                               provided. If a sql backend is
                               used, then this option is
                               required.

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

harupy · 2021-11-29T00:29:08Z

-        else:
-            default_artifact_root = DEFAULT_LOCAL_FILE_AND_ARTIFACT_PATH
+    default_artifact_root = resolve_default_artifact_root(
+        serve_artifacts, default_artifact_root, backend_store_uri, True


Suggested change

serve_artifacts, default_artifact_root, backend_store_uri, True

serve_artifacts, default_artifact_root, backend_store_uri, resolve_to_local=True

@BenWilson2 Can we use a keyword argument here?

harupy · 2021-11-29T02:12:42Z

+                (
+                    f"Endpoint: {request.url_rule} disabled due to the mlflow server running "
+                    "without `--serve-artifacts`. To enable artifacts server functionaltiy, "
+                    "run `mlflow server` with `--serve-artfiacts`"


Suggested change

"run `mlflow server` with `--serve-artfiacts`"

"run `mlflow server` with `--serve-artifacts`"

typo

harupy · 2021-11-29T02:13:01Z

+            return Response(
+                (
+                    f"Endpoint: {request.url_rule} disabled due to the mlflow server running "
+                    "without `--serve-artifacts`. To enable artifacts server functionaltiy, "


Suggested change

"without `--serve-artifacts`. To enable artifacts server functionaltiy, "

"without `--serve-artifacts`. To enable artifacts server functionality, "

typo

harupy · 2021-11-29T02:26:40Z

@BenWilson2 Can we merge the master branch to include changes made by #5070?

harupy · 2021-11-29T02:34:08Z

mlflow/mlflow/store/artifact/mlflow_artifacts_repo.py

Line 75 in 776831a

resolved = f"{base_url}{track_parse.path}{uri_parse.path.lstrip('/')}"

Is this line missing a slash after {track_parse.path}?

resolved = f"{base_url}{track_parse.path}/{uri_parse.path.lstrip('/')}"

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

…t-serve-artifacts Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

BenWilson2 · 2021-11-29T14:32:28Z

mlflow/mlflow/store/artifact/mlflow_artifacts_repo.py

Line 75 in 776831a

resolved = f"{base_url}{track_parse.path}{uri_parse.path.lstrip('/')}"

Is this line missing a slash after {track_parse.path}?
resolved = f"{base_url}{track_parse.path}/{uri_parse.path.lstrip('/')}" 

the parsed path element will have a trailing '/' when returned from urllib.parse.urlparse. If we include another '/', we'll have paths like this:
Expected :http://myhostname/api/2.0/mlflow-artifacts/artifacts/my/artifact/path/host
Actual :http://myhostname/api/2.0/mlflow-artifacts/artifacts//my/artifact/path/host

harupy · 2021-11-29T15:31:03Z

@BenWilson2 My suggested code was incorrect. I think we need a slash before track_parse.path in a case where tracking_uri looks like http://localhost:5000.

resolved = f"{base_url}/{track_parse.path}{uri_parse.path.lstrip('/')}"

Expected: http://myhostname/api/2.0/mlflow-artifacts/artifacts/my/artifact/path/host
Actual  : http://myhostname/api/2.0/mlflow-artifacts/artifactsmy/artifact/path/host
                                                     ^^^^^^^^^^^

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

BenWilson2 · 2021-11-29T18:10:39Z

@BenWilson2 My suggested code was incorrect. I think we need a slash before track_parse.path in a case where tracking_uri looks like http://localhost:5000.
resolved = f"{base_url}/{track_parse.path}{uri_parse.path.lstrip('/')}" 
Expected: http://myhostname/api/2.0/mlflow-artifacts/artifacts/my/artifact/path/host
Actual  : http://myhostname/api/2.0/mlflow-artifacts/artifactsmy/artifact/path/host
                                                     ^^^^^^^^^^^

Ah, I get it! Added fixes for this and added fixture params to validate in the test suite.

harupy · 2021-11-30T00:58:14Z

+@pytest.fixture(
+    scope="module", autouse=True, params=["http://localhost:5000", "http://localhost:5000/"]
+)


This doubles the number of tests. Can we just test that both http://localhost:5000 and http://localhost:5000/ can be resolved correctly?

definitely!

Created another fixture and used an explicit single test for validating alternate uri naming convention

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

harupy · 2021-11-30T02:29:52Z

            f"mlflow-artifacts://myhostname:4242{base_path}/hostport",
            f"http://myhostname:4242{base_url}{base_path}/hostport",
+            "http://myhostname:4242",


Can we parametrize this test?

@pytest.parametrize("tracking_uri", ["http://localhost:5000", "http://localhost:5000/"]) @pytest.parametrize("artifact_uri, resolved_uri", [(...), (...)]) def test_xxx_yyy(tracking_uri, artifact_uri, resolved_uri): assert MlflowArtifactsRepository.resolve_uri(tracking_uri, artifact_uri, resolved_uri) == ...

definitely. Added.

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

harupy · 2021-11-30T02:48:58Z

+        f"mlflow-artifacts://myhostname:4242{base_path}/hostport",
+        f"http://myhostname:4242{base_url}{base_path}/hostport",
+        "http://myhostname:4242",


Suggested change

f"mlflow-artifacts://myhostname:4242{base_path}/hostport",

f"http://myhostname:4242{base_url}{base_path}/hostport",

"http://myhostname:4242",

f"mlflow-artifacts://myhostname:4242{base_path}/hostport",

"http://myhostname:4242",

f"http://myhostname:4242{base_url}{base_path}/hostport",

Can we fix the order?

I like your edited way in the last comment better. Changed to that easier to read and cleaner implementation.

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

harupy · 2021-11-30T04:23:33Z

LGTM!

dbczumar · 2021-11-30T21:44:25Z

 # Also used as default location for artifacts, when not provided, in non local file based backends
 # (eg MySQL)
 DEFAULT_LOCAL_FILE_AND_ARTIFACT_PATH = "./mlruns"
+DEFAULT_ARTIFACTS_URI = "mlflow-artifacts:/"


Can we add an inline comment explaining what this is used for?

dbczumar · 2021-11-30T21:47:11Z

+    f"By default, data will be logged to the {DEFAULT_ARTIFACTS_URI} uri proxy if "
+    "the --serve-artifacts option is enabled. Otherwise, the default location will "
+    f"be {DEFAULT_LOCAL_FILE_AND_ARTIFACT_PATH}.",


Suggested change

f"By default, data will be logged to the {DEFAULT_ARTIFACTS_URI} uri proxy if "

"the --serve-artifacts option is enabled. Otherwise, the default location will "

f"be {DEFAULT_LOCAL_FILE_AND_ARTIFACT_PATH}.",

f"If the --serve-artifacts option is specified, the default artifact root is {DEFAULT_ARTIFACTS_URI}. "

f "otherwise, the default artifact root is {DEFAULT_LOCAL_FILE_AND_ARTIFACT_PATH}".

dbczumar

LGTM with 2 small docs comments. Thanks so much, @BenWilson2 !

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

BenWilson2 added 4 commits November 10, 2021 14:19

Add --serve-artifacts-opt and --artifacts-only options to mlflow server

69a26b8

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

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

a649b6f

…t-serve-artifacts Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

Update examples to show cli arguments in MLflow Artifacts

c44e62a

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

Update examples to turn on the REST API endpoints

74fa204

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

BenWilson2 requested review from dbczumar and harupy November 10, 2021 21:00

github-actions Bot added area/examples Example code area/server-infra MLflow Tracking server backend area/tracking Tracking service, tracking client APIs, autologging rn/feature Mention under Features in Changelogs. labels Nov 10, 2021

linting

d1b986d

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

harupy reviewed Nov 10, 2021

View reviewed changes

Comment thread mlflow/cli.py Outdated

harupy reviewed Nov 10, 2021

View reviewed changes

Comment thread mlflow/server/handlers.py Outdated

BenWilson2 added 2 commits November 10, 2021 22:27

PR changes and adjust test for serve-artifact option flag

7dbd469

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

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

4a0053a

…t-serve-artifacts Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

harupy reviewed Nov 11, 2021

View reviewed changes

Comment thread examples/mlflow_artifacts/README.md Outdated

Comment thread examples/mlflow_artifacts/docker-compose.yml

Comment thread mlflow/server/handlers.py

BenWilson2 added 3 commits November 11, 2021 11:33

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

e0a02fb

…t-serve-artifacts Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

PR changes

d6b2f32

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

PR updates

b0987a5

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

BenWilson2 requested a review from harupy November 11, 2021 19:19

harupy reviewed Nov 12, 2021

View reviewed changes

Comment thread tests/test_cli.py Outdated

harupy reviewed Nov 12, 2021

View reviewed changes

Comment thread examples/mlflow_artifacts/docker-compose.yml Outdated

harupy reviewed Nov 12, 2021

View reviewed changes

Comment thread examples/mlflow_artifacts/example.py

harupy reviewed Nov 12, 2021

View reviewed changes

Comment thread tests/test_cli.py Outdated

harupy reviewed Nov 12, 2021

View reviewed changes

Comment thread mlflow/utils/rest_utils.py Outdated

harupy reviewed Nov 12, 2021

View reviewed changes

Comment thread mlflow/server/handlers.py Outdated

PR feedback changes

45b9498

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

dbczumar requested changes Nov 15, 2021

View reviewed changes

BenWilson2 requested a review from dbczumar November 24, 2021 17:41

dbczumar changed the title ~~Add server option for serving only artifacts and proxied serving mode [ML-18422]~~ Add server option for serving only artifacts and proxied serving mode Nov 24, 2021

Rebase and discard formatting commits

d83c7a3

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

BenWilson2 force-pushed the support-serve-artifacts branch from 09b84eb to d83c7a3 Compare November 25, 2021 00:37

harupy reviewed Nov 29, 2021

View reviewed changes

BenWilson2 added 2 commits November 29, 2021 09:16

typos

6d5641c

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

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

1370034

…t-serve-artifacts Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>

fix parsing of uri for trailing slash

8fffe43

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

harupy reviewed Nov 30, 2021

View reviewed changes

BenWilson2 added 2 commits November 29, 2021 20:48

Test complexity reduction

374e209

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

tracking uri validation checks and test simplification

de2e78c

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

harupy reviewed Nov 30, 2021

View reviewed changes

Parameterize the uri resolution tests to aid in test debugging

d530c50

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

harupy reviewed Nov 30, 2021

View reviewed changes

Cleaner test syntax

5db552b

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

dbczumar reviewed Nov 30, 2021

View reviewed changes

dbczumar approved these changes Nov 30, 2021

View reviewed changes

Add and adjust pydoc strings

85d191c

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

BenWilson2 merged commit fb2972f into master Dec 1, 2021

BenWilson2 deleted the support-serve-artifacts branch December 1, 2021 01:44

	def _disable_mlflow_artifacts_only(func):
	def _disable_if_artifacts_only(func):

	"overridden via '--artifact-destination' arguments. "
	"overridden via '--artifacts-destination' argument. "

	serve_artifacts, default_artifact_root, backend_store_uri, True
	serve_artifacts, default_artifact_root, backend_store_uri, resolve_to_local=True

	"run `mlflow server` with `--serve-artfiacts`"
	"run `mlflow server` with `--serve-artifacts`"

	"without `--serve-artifacts`. To enable artifacts server functionaltiy, "
	"without `--serve-artifacts`. To enable artifacts server functionality, "

Conversation

BenWilson2 commented Nov 10, 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!

Uh oh!

harupy Nov 11, 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 12, 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbczumar left a comment

Choose a reason for hiding this comment

Uh oh!

harupy Nov 29, 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!

Choose a reason for hiding this comment

Uh oh!

harupy commented Nov 29, 2021

Uh oh!

harupy commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenWilson2 commented Nov 29, 2021

Uh oh!

harupy commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenWilson2 commented Nov 29, 2021

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 30, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy commented Nov 30, 2021

Uh oh!

harupy Nov 11, 2021 •

edited

Loading

harupy Nov 12, 2021 •

edited

Loading

harupy Nov 29, 2021 •

edited

Loading

harupy commented Nov 29, 2021 •

edited

Loading

harupy commented Nov 29, 2021 •

edited

Loading

harupy Nov 30, 2021 •

edited

Loading