Skip to content

Improve load_model() by adding param dst_path#4997

Merged
dbczumar merged 3 commits into
mlflow:masterfrom
liangz1:load-model
Nov 9, 2021
Merged

Improve load_model() by adding param dst_path#4997
dbczumar merged 3 commits into
mlflow:masterfrom
liangz1:load-model

Conversation

@liangz1

@liangz1 liangz1 commented Nov 3, 2021 •
edited
Loading

Copy link
Copy Markdown

Signed-off-by: Liang Zhang liang.zhang@databricks.com

What changes are proposed in this pull request?

This PR is to resolve issue #4852.

How is this patch tested?

Existing tests.

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.

You can specify the directory where model artifacts are downloaded to by using the parameter dst_path in load_model().

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/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

add param output_path to load_model function
677e1b6 
Signed-off-by: Liang Zhang <liang.zhang@databricks.com>
@github-actions github-actions Bot added area/models MLmodel format, model serialization/deserialization, flavors rn/none List under Small Changes in Changelogs. labels Nov 3, 2021
@liangz1 liangz1 requested a review from arjundc-db November 3, 2021 15:31
lint
cbb0874 
Signed-off-by: Liang Zhang <liang.zhang@databricks.com>
dbczumar
dbczumar reviewed Nov 4, 2021
View reviewed changes
Comment thread mlflow/catboost.py Outdated


def load_model(model_uri):
def load_model(model_uri, artifact_path=None):

@dbczumar dbczumar Nov 4, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think dst_path or path might be better here to distinguish from the artifact_path parameter of log_model(). This will make it clearer that artifact_path is not related to the model_uri / run.

Suggested change
def load_model(model_uri, artifact_path=None):
def load_model(model_uri, dst_path=None):

@harupy harupy Nov 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 for this change. download_artifacts also has a dst_path argument:

mlflow/mlflow/tracking/client.py

Lines 1353 to 1365 in 8bd7503

def download_artifacts(self, run_id: str, path: str, dst_path: Optional[str] = None) -> str:
"""
Download an artifact file or directory from a run to a local directory if applicable,
and return a local path for it.
:param run_id: The run to download artifacts from.
:param path: Relative source path to the desired artifact.
:param dst_path: Absolute path of the local filesystem destination directory to which to
download the specified artifacts. This directory must already exist.
If unspecified, the artifacts will either be downloaded to a new
uniquely-named directory on the local filesystem or will be returned
directly in the case of the LocalArtifactRepository.
:return: Local path of desired artifact.

@WeichenXu123 WeichenXu123 Nov 8, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+1

@jwyyy jwyyy mentioned this pull request Nov 7, 2021
Merged
27 tasks
@WeichenXu123

WeichenXu123 commented Nov 8, 2021

Copy link
Copy Markdown
Collaborator

After load_model returned, shall we delete the model file on local disk immediately ?

rename to dst_path
0606404 
Signed-off-by: Liang Zhang <liang.zhang@databricks.com>
@liangz1 liangz1 requested review from dbczumar and harupy and removed request for arjundc-db November 8, 2021 09:55
@liangz1

liangz1 commented Nov 8, 2021

Copy link
Copy Markdown
Author

@WeichenXu123

After load_model returned, shall we delete the model file on local disk immediately ?

That's a good point for optimization that could be addressed in a separate PR. We also need to confirm that for all the flavors, the load_model() method is not lazy, and will never refer to the model file after load_model is returned.

@liangz1 liangz1 changed the title Improve load_model() by adding param artifact_path Improve load_model() by adding param dst_path Nov 8, 2021
@liangz1

liangz1 commented Nov 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Author

It looks like the failure is discussed here keras-team/keras#15579. It's not related to this PR.

Cross version tests / test (keras / 2.7.0 / autologging)
Cross version tests / test (keras / 2.7.0 / models)

@liangz1 liangz1 requested a review from WeichenXu123 November 8, 2021 15:33
dbczumar
dbczumar approved these changes Nov 9, 2021
View reviewed changes

@dbczumar dbczumar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM! Thanks @liangz1 !

@dbczumar dbczumar merged commit 5b8483a into mlflow:master Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbczumar dbczumar dbczumar approved these changes

@harupy harupy Awaiting requested review from harupy

@WeichenXu123 WeichenXu123 Awaiting requested review from WeichenXu123

Assignees

No one assigned

Labels

area/models MLmodel format, model serialization/deserialization, flavors rn/none List under Small Changes in Changelogs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@liangz1 @WeichenXu123 @harupy @dbczumar