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.

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

[ML-31719] Added lineage identifier header to the UC CreateModelVersion REST API call #8541

Merged
merged 32 commits into from
Jun 1, 2023

Conversation

kriscon-db
Copy link
Collaborator

@kriscon-db kriscon-db commented May 26, 2023

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Enabling extra_headers to be used in CreateModelVersion calls and passing through the new lineage ID HTTP header that the UC-MR service will use to propagate lineage information through to ManagedCatalog

How is this patch tested?

  • Added an additional unit test around _get_notebook_id
  • Fixed the GCP unit test to mock the notebook id accordingly and validate that all HTTP request calls have the expected headers
  • Manually tested on a dogfood notebook to verify that the new header is being included in the REST API call to the UC-MR service
Screenshot 2023-05-29 at 8 54 23 PM
  • Existing unit/integration tests
  • New unit/integration tests
  • Manual tests (describe details, including test results, below)

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.

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

  • 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

@mlflow-automation
Copy link
Collaborator

mlflow-automation commented May 26, 2023

Documentation preview for 6958ca6 will be available here when this CircleCI job completes successfully.

More info

@kriscon-db kriscon-db changed the title Added lineage identifier header to the UC CreateModelVersion REST API call [WIP] Added lineage identifier header to the UC CreateModelVersion REST API call May 26, 2023
@github-actions
Copy link

@kriscon-db Thank you for the contribution! Could you fix the following issue(s)?

⚠ DCO check

The DCO check failed. Please sign off your commit(s) by following the instructions here. See https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.md#sign-your-work for more details.

@kriscon-db kriscon-db requested a review from smurching May 26, 2023 16:58
@github-actions github-actions bot added area/model-registry Model registry, model registry APIs, and the fluent client calls for model registry rn/none List under Small Changes in Changelogs. labels May 26, 2023
@kriscon-db kriscon-db changed the title [WIP] Added lineage identifier header to the UC CreateModelVersion REST API call [ML-31719] Added lineage identifier header to the UC CreateModelVersion REST API call May 30, 2023
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 for mlflow/utils/rest_utils.py. Let's block on @smurching's approval for the UC-specific changes?

@kriscon-db kriscon-db requested a review from smurching May 30, 2023 20:06
Copy link
Collaborator

@smurching smurching left a comment

Choose a reason for hiding this comment

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

Had just one comment, otherwise LGTM!

@kriscon-db
Copy link
Collaborator Author

Rerequesting a review @dbczumar because I added in new protobuf definitions and wanted another pair of eyes on the way im serializing the protobuf message for the HTTP header.

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.

re-LGTMing. Thanks @kriscon-db !

@kriscon-db kriscon-db requested a review from smurching June 1, 2023 18:30
Copy link
Collaborator

@smurching smurching left a comment

Choose a reason for hiding this comment

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

LGTM!

@BenWilson2 BenWilson2 merged commit 92859f3 into mlflow:master Jun 1, 2023
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/model-registry Model registry, model registry APIs, and the fluent client calls for model registry rn/none List under Small Changes in Changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants