Skip to content

Deprecate duplicate artifact download APIs#6537

Merged
WeichenXu123 merged 6 commits intomlflow:branch-2.0from
WeichenXu123:remove-download-artifacts
Aug 23, 2022
Merged

Deprecate duplicate artifact download APIs#6537
WeichenXu123 merged 6 commits intomlflow:branch-2.0from
WeichenXu123:remove-download-artifacts

Conversation

@WeichenXu123
Copy link
Copy Markdown
Collaborator

@WeichenXu123 WeichenXu123 commented Aug 22, 2022 •
edited
Loading

Signed-off-by: Weichen Xu [email protected]

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Deprecate duplicate artifact download APIs.

The following APIs are removed:

MlflowClient.download_artifacts

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

Deprecate duplicate artifact download API: MlflowClient.download_artifacts

What component(s), interfaces, languages, and integrations does this PR affect?

Components

  • area/artifacts: Artifact stores and artifact logging
  • area/build: Build and test infrastructure for MLflow
  • area/docs: MLflow documentation pages
  • area/examples: Example code
  • area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
  • area/models: MLmodel format, model serialization/deserialization, flavors
  • area/pipelines: Pipelines, Pipeline APIs, Pipeline configs, Pipeline Templates
  • area/projects: MLproject format, project running backends
  • area/scoring: MLflow Model server, model deployment tools, Spark UDFs
  • area/server-infra: MLflow Tracking server backend
  • area/tracking: Tracking Service, tracking client APIs, autologging

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

@WeichenXu123
init
174cac2 
Signed-off-by: Weichen Xu <[email protected]>
@WeichenXu123
update
19458aa 
Signed-off-by: Weichen Xu <[email protected]>
@WeichenXu123
Copy link
Copy Markdown
Collaborator Author

WeichenXu123 commented Aug 22, 2022 •
edited
Loading

shall we add an argument “tracking_uri” for mlflow.artifacts.download_artifacts ? Otherwise we need to globally set tracking uri (by env variable MLFLOW_TRACKING_URI or call mlflow.set_tracking_uri) before calling the API.

@dbczumar
Copy link
Copy Markdown
Collaborator

dbczumar commented Aug 22, 2022 •
edited
Loading

shall we add an argument “tracking_uri” for mlflow.artifacts.download_artifacts ? Otherwise we need to globally set tracking uri (by env variable MLFLOW_TRACKING_URI or call mlflow.set_tracking_uri) before calling the API.

Ah, that's definitely a problem. Good catch! Can we add tracking_uri to mlflow.download_artifacts()?

@WeichenXu123 WeichenXu123 changed the title Remove duplicate artifact download APIs Deprecate duplicate artifact download APIs Aug 23, 2022
@WeichenXu123
update
00a0535 
Signed-off-by: Weichen Xu <[email protected]>
@WeichenXu123
update
d29822b 
Signed-off-by: Weichen Xu <[email protected]>
@WeichenXu123 WeichenXu123 force-pushed the remove-download-artifacts branch from 2f0d27a to d29822b Compare August 23, 2022 01:19
harupy
harupy reviewed Aug 23, 2022
View reviewed changes
mlflow/artifacts/__init__.py Outdated
Comment on lines +59 to +63
if tracking_uri is not None:
with _use_tracking_uri(tracking_uri):
store = _get_store()
else:
store = _get_store()
Copy link
Copy Markdown
Member

@harupy harupy Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if tracking_uri is not None:
with _use_tracking_uri(tracking_uri):
store = _get_store()
else:
store = _get_store()
store = _get_store(tracking_uri)

Can we pass tracking_uri to _get_store?

harupy
harupy reviewed Aug 23, 2022
View reviewed changes
mlflow/tracking/client.py
"""
return self._tracking_client.list_artifacts(run_id, path)

@deprecated("mlflow.artifacts.download_artifacts", "2.0")
Copy link
Copy Markdown
Member

@harupy harupy Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning message looks incorrect. It should be mlflow.tracking.client.MflowClient.download_artifacts.

image

Copy link
Copy Markdown
Member

@harupy harupy Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can address this issue in a separate PR.

Copy link
Copy Markdown
Collaborator Author

@WeichenXu123 WeichenXu123 Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will file a follow-up PR to fix this.

Copy link
Copy Markdown
Member

@harupy harupy Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WeichenXu123 You don't need to because I already did in #6552.

harupy
harupy approved these changes Aug 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@harupy harupy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left #6537 (comment), otherwise LGTM!

@WeichenXu123
update
e33731d 
Signed-off-by: Weichen Xu <[email protected]>
@github-actions github-actions bot added area/artifacts Artifact stores and artifact logging rn/breaking-change Mention under Breaking Changes in Changelogs. labels Aug 23, 2022
@WeichenXu123
fix lint
3c0024c 
Signed-off-by: Weichen Xu <[email protected]>
@WeichenXu123 WeichenXu123 merged commit 2d4e092 into mlflow:branch-2.0 Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@harupy harupy harupy approved these changes

Assignees

No one assigned

Labels

area/artifacts Artifact stores and artifact logging rn/breaking-change Mention under Breaking Changes in Changelogs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@WeichenXu123 @dbczumar @harupy