Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

set_signature API to set the signature for model artifacts #8476

Merged
merged 12 commits into from
May 20, 2023
Merged

set_signature API to set the signature for model artifacts #8476

merged 12 commits into from
May 20, 2023

Conversation

jerrylian-db
Copy link
Collaborator

@jerrylian-db jerrylian-db commented May 19, 2023 •
edited

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

This PR introduces a set_signature API that makes it possible to set signatures for model artifacts. These model artifacts can be locally saved models, logged run model artifacts, or artifacts in any of the supported artifact stores. The function respects the immutability of model registry artifacts by disallowing model artifacts specified with a model:/ scheme URI.

How is this patch tested?

  • Existing unit/integration tests
  • New unit/integration tests
  • Manual tests (describe details, including test results, below)

I tested the set_signature API in a Databricks notebook. Here are the steps I took:

  1. Installed my feature branch using %pip install git+https://github.com/mlflow/mlflow@add_signature
  2. Logged a model with an MLflow signature. As shown below:
Screenshot 2023-05-19 at 10 40 23 AM 3. Set the signature of the model via the command `set_signature("runs:/{}/{}".format(run.info.run_id, "sklearn-model"), signature)`. Checked the run artifacts UI again and saw this: Screenshot 2023-05-19 at 10 41 34 AM

Does this PR change the documentation?

  • No. You can skip the rest of this section.
  • Yes. Make sure the changed pages / sections render correctly in the documentation preview.
Screenshot 2023-05-19 at 10 22 06 AM

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.

Introduces a mlflow.models.set_signature() API to set signatures for model 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/recipes: Recipes, Recipe APIs, Recipe configs, Recipe 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

@jerrylian-db
implement add_signature function
f15a690 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db
add unit tests
ab9b058 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db
iterate based on feedback
3875680 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db jerrylian-db self-assigned this May 19, 2023
@jerrylian-db
improve docs
020c8b3 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db
remove imports and add test
3abcdbb 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db jerrylian-db changed the title [WIP] set_signature API to set the signature for model artifacts set_signature API to set the signature for model artifacts May 19, 2023
@jerrylian-db
Merge branch 'master' into add_signature
45c45a6 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
dbczumar
dbczumar reviewed May 19, 2023
View reviewed changes
mlflow/models/signature.py Outdated
Comment on lines 299 to 304
assert isinstance(
signature, ModelSignature
), "The signature argument must be a ModelSignature object"
assert not ModelsArtifactRepository.is_models_uri(
model_uri
), "Model URIs with the `models:/` scheme are not supported."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we turn these into MlflowException with error code INVALID_PARAMETER_VALUE?

dbczumar
dbczumar reviewed May 19, 2023
View reviewed changes
mlflow/models/signature.py Outdated
Comment on lines 299 to 304
assert isinstance(
signature, ModelSignature
), "The signature argument must be a ModelSignature object"
assert not ModelsArtifactRepository.is_models_uri(
model_uri
), "Model URIs with the `models:/` scheme are not supported."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we update the error message to recommend that users create a new model version? It's always nice when I see an error message that tells me what to do next :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In a follow-up PR, I'll link to documentation that describes how to do this.

dbczumar
dbczumar reviewed May 19, 2023
View reviewed changes
mlflow/models/signature.py Outdated
Comment on lines 260 to 261
fail. Furthermore, as model registry artifacts are read-only, model artifacts located in the
model registry and represented by ``models:/`` URI schemes are not compatible with this API.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we recommend that users create a new model version in order to update the signature? Might be nice to include a link to some docs about how to do that (we may have to add those docs as part of this PR too, or feel free to inline as a Python code example)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add a new doc section in the Model Signatures doc in a follow up PR.

dbczumar
dbczumar reviewed May 19, 2023
View reviewed changes
tests/tracking/test_artifact_utils.py
out.write(artifact_text)

with mlflow.start_run() as run:
mlflow.log_metric("coolness", 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Coolness, indeed :D

dbczumar
dbczumar approved these changes May 19, 2023
View reviewed changes
Copy link
Collaborator

@dbczumar dbczumar left a comment

Choose a reason for hiding this comment

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

LGTM with nitty docs comments. Impl looks wonderful! Thanks @jerrylian-db !

@mlflow-automation
Copy link
Collaborator

mlflow-automation commented May 19, 2023 •
edited

Documentation preview for 092bfb5 will be available here when this CircleCI job completes successfully.

More info

@github-actions github-actions bot added area/models MLmodel format, model serialization/deserialization, flavors rn/feature Mention under Features in Changelogs. labels May 19, 2023
@jerrylian-db
adapt feedback
14195db 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db
change doc string
1865989 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db
change doc string
cd9f237 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db jerrylian-db enabled auto-merge (squash) May 19, 2023 22:06
@jerrylian-db
fix lint
9cb33ec 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db
fix lint
10e0f8b 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db
fix lint
092bfb5 
Signed-off-by: Jerry Liang <jerry.liang@databricks.com>
@jerrylian-db jerrylian-db merged commit 959e8d9 into master May 20, 2023
26 checks passed
@jerrylian-db jerrylian-db mentioned this pull request May 22, 2023
Merged
33 tasks
@jerrylian-db jerrylian-db deleted the add_signature branch May 26, 2023 17:21
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

Assignees

@jerrylian-db jerrylian-db

Labels
area/models MLmodel format, model serialization/deserialization, flavors rn/feature Mention under Features in Changelogs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jerrylian-db @mlflow-automation @dbczumar