Add model registration argument for autolog #5395
Conversation
| with patch("mlflow.register_model") as mock_register_model, mlflow.start_run(): | ||
| model.fit(X, y) | ||
|
|
||
| if registered_model_name is None: | ||
| mock_register_model.assert_not_called() | ||
| else: | ||
| mock_register_model.assert_called_once_with( | ||
| ANY, registered_model_name, await_registration_for=ANY | ||
| ) |
There was a problem hiding this comment.
why not check the model is registered with the specified name?
There was a problem hiding this comment.
that might requires more code to write, and register_model functionality should be covered by the model registry testing code.
There was a problem hiding this comment.
that might require more code to write.
Are you sure? Can we just call get_registered_model? Let me know if I'm missing something. And why is writing more code an issue?
MlflowClient().get_registered_model(name)seems (much) shorter (and more strict because assert_called_once_with doesn't guarantee register_model succeeded) than:
from unittest.mock import patch, ANY
with patch("mlflow.register_model") as mock_register_model:
mock_register_model.assert_called_once_with(
ANY, registered_model_name, await_registration_for=ANY
)There was a problem hiding this comment.
register_modelfunctionality should be covered by the model registry testing code.
Yes, but how do we ensure we don't pass incorrect arguments?
There was a problem hiding this comment.
updated using MlflowClient().get_registered_model(name) for test
github-actions
bot
added
area/tracking
There was a problem hiding this comment.
LGTM once https://github.com/mlflow/mlflow/pull/5395/files#r813665077 is addressed. Thanks @WeichenXu123 !
|
A weird error in CI tensorflow 2.0.0 / 2.0.4 but I cannot reproduce it in my local machine. might be CI env setup issue. |
I think it does because the failed test ( |
|
@WeichenXu123 |
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: Weichen Xu weichen.xu@databricks.com
What changes are proposed in this pull request?
Add model registration argument for autolog
How is this patch tested?
Unit tests.
Does this PR change the documentation?
ci/circleci: build_doccheck. If it's successful, proceed to thenext step, otherwise fix it.
Detailson the right to open the job page of CircleCI.Artifactstab.docs/build/html/index.html.Release Notes
Is this a user-facing change?
Add model registration argument for autolog
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/artifacts: Artifact stores and artifact loggingarea/build: Build and test infrastructure for MLflowarea/docs: MLflow documentation pagesarea/examples: Example codearea/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registryarea/models: MLmodel format, model serialization/deserialization, flavorsarea/projects: MLproject format, project running backendsarea/scoring: MLflow Model server, model deployment tools, Spark UDFsarea/server-infra: MLflow Tracking server backendarea/tracking: Tracking Service, tracking client APIs, autologgingInterface
area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Modelsarea/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registryarea/windows: Windows supportLanguage
language/r: R APIs and clientslanguage/java: Java APIs and clientslanguage/new: Proposals for new client languagesIntegrations
integrations/azure: Azure and Azure ML integrationsintegrations/sagemaker: SageMaker integrationsintegrations/databricks: Databricks integrationsHow should the PR be classified in the release notes? Choose one:
rn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notes