Pass in sourceId tag in all cases #10464
Conversation
|
Documentation preview for 61b1e50 will be available here when this CircleCI job completes successfully. More info
|
mlflow/tracking/default_experiment/databricks_notebook_experiment_provider.py
Outdated
Show resolved
Hide resolved
| try: | ||
| experiment_id = MlflowClient().create_experiment(source_notebook_name, None, tags) | ||
| except MlflowException as e: | ||
| if e.error_code == databricks_pb2.ErrorCode.Name( | ||
| databricks_pb2.INVALID_PARAMETER_VALUE | ||
| ): | ||
| # If repo notebook experiment creation isn't enabled, fall back to | ||
| # using the notebook ID | ||
| # If determined that it is not a repo notebook | ||
| experiment_id = source_notebook_id | ||
| else: | ||
| raise e |
There was a problem hiding this comment.
@annzhang-db I thought we would get a RESOURCE_ALREADY_EXISTS exception if we call create_experiment() with a non-repo notebook path that already exists. Is that what the backend does?
If the backend does indeed return RESOURCE_ALREADY_EXISTS, I think that would break the current implementation of DatabricksNotebookExperimentProvider in this PR; have we tested this thoroughly?
There was a problem hiding this comment.
Yes, we get a RESOURCE_ALREADY_EXISTS exception if we call create_experiment() with a non-repo notebook path that already exists AND no sourceType/sourceId tags passed in. Since we are passing in the sourceId here, it will actually go into the previous case (in the backend PR, ill leave a comment there) and raise INVALID_PARAMETER_VALUE error for sourceId but no sourceType.
There was a problem hiding this comment.
So if I log a param in a non-repo notebook, then attach/detach from the cluster, then try to log a param again, won't this default experiment provider try to call create_experiment() under the hood and then fail at the user level with RESOURCE_ALREADY_EXISTS, which will break the user's workflow?
There was a problem hiding this comment.
It will call create_experiment() with a sourceId tag, which will not fail with RESOURCE_ALREADY_EXISTS. Only if create_experiment() is called without sourceId tag will it fail with RESOURCE_ALREADY_EXISTS.
There was a problem hiding this comment.
LGTM once we add an integration suite test case: https://src.dev.databricks.com/databricks/universe/-/blob/mlflow/src/test/e2e/tracking/DefaultRepoNotebookExperimentIntegrationSuite.scala. Thanks @annzhang-db
Signed-off-by: Ann Zhang <ann.zhang@databricks.com>
Signed-off-by: Ann Zhang <ann.zhang@databricks.com>
馃洜 DevTools 馃洜
Install mlflow from this PR
Checkout with GitHub CLI
Related Issues/PRs
#xxxWhat changes are proposed in this pull request?
How is this PR tested?
Does this PR require documentation update?
Release Notes
Is this a user-facing change?
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/gateway: AI Gateway service, Gateway client APIs, third-party Gateway integrationsarea/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registryarea/models: MLmodel format, model serialization/deserialization, flavorsarea/recipes: Recipes, Recipe APIs, Recipe configs, Recipe Templatesarea/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/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/breaking-change- The PR will be mentioned in the "Breaking Changes" 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