Enable `--serve-artifacts` by default by harupy · Pull Request #6502 · mlflow/mlflow

harupy · 2022-08-18T05:37:06Z

Signed-off-by: harupy [email protected]

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Enable --serve-artifacts by default when running a tracking server.

How is this patch tested?

I have written tests (not required for typo or doc fix) and confirmed the proposed feature/bug-fix/change works.

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.

Click the Details link on the Preview docs check.
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.)

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

dbczumar

Two small pieces of feedback:

When I run with --backend-store-uri sqlite:///mydb.sqlite --no-serve-artifacts and forget to specify `--default-artifact-root, I see:

mlflow server --backend-store-uri sqlite:///mydb.sqlite --no-serve-artifacts
Option 'default-artifact-root' is required when backend store is not local file based.
Usage: mlflow server [OPTIONS]
Try 'mlflow server --help' for help.

Error: Option 'default-artifact-root' is required when backend store is not local file based.

Can we update this to say that 'default-artifact-root' is required when backend store is not local file based and artifact serving is disabled?

Do we need to update docs, such as https://mlflow.org/docs/latest/tracking.html#using-the-tracking-server-for-proxied-artifact-access, to indicate that proxied artifact access is enabled by default and also talk about how the --no-serve-artifacts option can be used to opt out?

mlflow/utils/cli_args.py

harupy · 2022-08-19T07:30:04Z

Can we update this to say that 'default-artifact-root' is required when backend store is not local file based and artifact serving is disabled?

Got it, thanks for the catch!

Do we need to update docs, such as https://mlflow.org/docs/latest/tracking.html#using-the-tracking-server-for-proxied-artifact-access, to indicate that proxied artifact access is enabled by default and also talk about how the --no-serve-artifacts option can be used to opt out?

We do :) I'll update them.

Signed-off-by: harupy <[email protected]>

dbczumar

LGTM once docs are updated; thanks @harupy !

Signed-off-by: harupy <[email protected]>

docs/source/tracking.rst

Signed-off-by: harupy <[email protected]>

harupy · 2022-08-22T04:08:46Z

docs/source/tracking.rst

+.. code-block:: bash
+    :caption: Command to run the tracking server in this configuration
+
+    mlflow server --backend-store-uri file::///path/to/mlruns --no-serve-artifacts


Separated the command from the image to make it easier to edit and copy.

Fantastic!

Suggested change

.. code-block:: bash

:caption: Command to run the tracking server in this configuration

mlflow server --backend-store-uri file::///path/to/mlruns --no-serve-artifacts

.. code-block:: bash

:caption: Command to run the tracking server in this configuration

mlflow server --backend-store-uri file:///path/to/mlruns --no-serve-artifacts

Thanks for the catch!

dbczumar · 2022-08-23T00:19:36Z

docs/source/tracking.rst

+.. code-block:: bash
+    :caption: Command to run the tracking server in this configuration
+
+    mlflow server --backend-store-uri postgresql:://URI --default-artifact-root s3://bucket_name --host remote_host --no-serve-artifacts


I think we should remove the double colons in these examples, right?

Suggested change

mlflow server --backend-store-uri postgresql:://URI --default-artifact-root s3://bucket_name --host remote_host --no-serve-artifacts

mlflow server --backend-store-uri postgresql://URI --default-artifact-root s3://bucket_name --host remote_host --no-serve-artifacts

Can we also replace URI with a realistic example?

dbczumar

LGTM once remaining comments are addressed! Thanks @harupy !

Signed-off-by: harupy <[email protected]>

Enable --serve-artifacts by default

d7a137f

Signed-off-by: harupy <[email protected]>

github-actions bot added area/tracking Tracking service, tracking client APIs, autologging rn/feature Mention under Features in Changelogs. labels Aug 18, 2022

harupy added 2 commits August 18, 2022 16:24

remove --serve-artifacts

1f89334

Signed-off-by: harupy <[email protected]>

more updates

111a676

Signed-off-by: harupy <[email protected]>

WeichenXu123 approved these changes Aug 18, 2022

View reviewed changes

fix failed tests

af520f7

Signed-off-by: harupy <[email protected]>

dbczumar reviewed Aug 19, 2022

View reviewed changes

mlflow/utils/cli_args.py Show resolved Hide resolved

harupy added 2 commits August 19, 2022 16:35

update default_artifact_root error message

75dd93d

Signed-off-by: harupy <[email protected]>

add how to disable artifact serving

6a6af2f

Signed-off-by: harupy <[email protected]>

dbczumar approved these changes Aug 20, 2022

View reviewed changes

harupy added 2 commits August 22, 2022 12:12

update docs

5d0daf5

Signed-off-by: harupy <[email protected]>

fix captions

172544c

Signed-off-by: harupy <[email protected]>

harupy commented Aug 22, 2022

View reviewed changes

docs/source/tracking.rst Show resolved Hide resolved

harupy and others added 4 commits August 22, 2022 12:36

add missing caption

13e2f13

Signed-off-by: harupy <[email protected]>

crop images

b6967eb

Signed-off-by: harupy <[email protected]>

crop again

4e83cb8

Signed-off-by: harupy <[email protected]>

add missing caption

026828d

Signed-off-by: harupy <[email protected]>

harupy requested a review from dbczumar August 22, 2022 04:08

harupy commented Aug 22, 2022

View reviewed changes

dbczumar reviewed Aug 23, 2022

View reviewed changes

dbczumar approved these changes Aug 23, 2022

View reviewed changes

harupy and others added 6 commits August 23, 2022 10:06

address comments

7a2e134

Signed-off-by: harupy <[email protected]>

adjust image width

a175703

Signed-off-by: harupy <[email protected]>

crop images

4d805a7

Signed-off-by: harupy <[email protected]>

adjust image width

9a69ffd

Signed-off-by: harupy <[email protected]>

add serve_artifacts

6f4d8d3

Signed-off-by: harupy <[email protected]>

update R docs

2f838fa

Signed-off-by: harupy <[email protected]>

harupy merged commit 9f9252d into mlflow:branch-2.0 Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `--serve-artifacts` by default#6502

Enable `--serve-artifacts` by default#6502
harupy merged 18 commits intomlflow:branch-2.0from
harupy:enable-serve-artifacts-by-default

harupy commented Aug 18, 2022

Uh oh!

dbczumar left a comment

Uh oh!

Uh oh!

harupy commented Aug 19, 2022

Uh oh!

dbczumar left a comment

Uh oh!

Uh oh!

harupy Aug 22, 2022

Uh oh!

dbczumar Aug 23, 2022

Uh oh!

harupy Aug 23, 2022

Uh oh!

dbczumar Aug 23, 2022

Uh oh!

dbczumar Aug 23, 2022

Uh oh!

harupy Aug 23, 2022

Uh oh!

dbczumar left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	mlflow server --backend-store-uri postgresql:://URI --default-artifact-root s3://bucket_name --host remote_host --no-serve-artifacts
	mlflow server --backend-store-uri postgresql://URI --default-artifact-root s3://bucket_name --host remote_host --no-serve-artifacts

Conversation

harupy commented Aug 18, 2022

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!

dbczumar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

harupy commented Aug 19, 2022

Uh oh!

dbczumar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

harupy Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

dbczumar Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

harupy Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

dbczumar Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

dbczumar Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

harupy Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

dbczumar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants