Fix: "Source Run" field in Model Version UI page points to wrong run model artifact in Run UI page #8156

WeichenXu123 · 2023-04-03T12:14:21Z

Related Issues/PRs

#8150

What changes are proposed in this pull request?

Fix: "Source Run" field in Model Version UI page points to wrong run model artifact in Run UI page

Closes #8150

How is this patch tested?

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

manually test code:

import mlflow

import mlflow.sklearn
from sklearn.datasets import load_iris
from sklearn import tree

iris = load_iris()
sk_model = tree.DecisionTreeClassifier()
sk_model = sk_model.fit(iris.data, iris.target)

with mlflow.start_run() as run:
    run_id = run.info.run_id
    print(run_id)
    mlflow.sklearn.log_model(sk_model, "model1")
    mlflow.sklearn.log_model(sk_model, "model2")

    mlflow.register_model(f"runs:/{run_id}/model1", "model01")
    mlflow.register_model(f"runs:/{run_id}/model2", "model02")

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

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

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

mlflow-automation · 2023-04-03T12:14:38Z

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

More info

Ignore this comment if this PR does not change the documentation.
It takes a few minutes for the preview to be available.
The preview is updated when a new commit is pushed to this PR.
This comment was created by https://github.com/mlflow/mlflow/actions/runs/4676502406.

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

dbczumar · 2023-04-04T00:41:49Z

mlflow/server/js/src/model-registry/components/ModelVersionView.js

@@ -316,8 +316,15 @@ export class ModelVersionViewImpl extends React.Component {
        </a>
      );
    } else if (runInfo) {
+      const artifactPath = modelSource.substring(modelSource.indexOf('/artifacts/') + 11);


@dbczumar Test the case where source run is defined but source lives outside the run (strange case, but could happen)

@dbczumar Any updates for this comment ? :)

In latest code, if model source is empty or parsing model source failed, it will still jump to the run page. So for some edge case the page won't be broken.

mlflow/server/js/src/model-registry/components/ModelVersionView.js

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

mlflow/server/js/src/model-registry/components/ModelVersionView.js

mlflow/server/js/src/experiment-tracking/routes.js

mlflow/server/js/src/model-registry/components/ModelVersionView.js

harupy · 2023-04-04T12:43:34Z

mlflow/server/js/src/model-registry/components/ModelVersionView.js

+      if (!modelSource) {
+        throw new Error('model source is missing.');
+      }
+      const artifactPath = modelSource.match(/.+\/artifacts\/(.+)$/)[1];


This line seems to assume that modelSource always ends with /artifacts/.... What if it doesn't?

What's the cases it does not ends with /artifacts/... ?

I think the source string is

[file|s3|...]:/path/to/mlruns/{experiment_id}/{run_id}/artifacts/{artifact_path}

or

mlflow-artifacts:/{experiment_id}/{run_id}/artifacts/{artifact_path}

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

mlflow/server/js/src/model-registry/utils/VersionUtil.ts

hubertzub-db

lgtm with two nits left

mlflow/server/js/src/model-registry/components/ModelVersionView.js

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

WeichenXu123 · 2023-04-10T12:41:39Z

CC @dbczumar @harupy Do you have more comments for this PR ? Thank you!

harupy · 2023-04-11T02:08:50Z

mlflow/server/js/src/model-registry/utils/VersionUtils.ts

+/**
+ * Extract artifact path from provided `modelSource` string
+ */
+export function extractArtifactPathFromModelSource(modelSource: string, runId: string) {


Can we add a return type?

@harupy my two cents here is that the vast chunk of DB and MLflow typescript codebase relies on implicit inferred return types (in this case string | undefined will be automatically inferred) so it's ok I guess 🤔

@hubertzub-db Makes sense!

mlflow/server/js/src/experiment-tracking/routes.js

mlflow/server/js/src/model-registry/utils/VersionUtils.ts

harupy

LGTM once the remaining comments are addressed

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

init

42a6e36

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

WeichenXu123 mentioned this pull request Apr 3, 2023

[BUG] "Source Run" field in Model Version UI page points to wrong run model artifact in Run UI page #8150

Closed

format

ec3c67c

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

dbczumar reviewed Apr 4, 2023

View reviewed changes

dbczumar requested a review from hubertzub-db April 4, 2023 00:42

hubertzub-db reviewed Apr 4, 2023

View reviewed changes

mlflow/server/js/src/model-registry/components/ModelVersionView.js Outdated Show resolved Hide resolved

mlflow/server/js/src/model-registry/components/ModelVersionView.js Outdated Show resolved Hide resolved

update

60df129

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

github-actions bot added the rn/bug-fix Mention under Bug Fixes in Changelogs. label Apr 4, 2023

update test

9ad0992

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

WeichenXu123 commented Apr 4, 2023

View reviewed changes

mlflow/server/js/src/model-registry/components/ModelVersionView.js Outdated Show resolved Hide resolved

harupy reviewed Apr 4, 2023

View reviewed changes

mlflow/server/js/src/experiment-tracking/routes.js Outdated Show resolved Hide resolved

harupy reviewed Apr 4, 2023

View reviewed changes

mlflow/server/js/src/model-registry/components/ModelVersionView.js Outdated Show resolved Hide resolved

harupy reviewed Apr 4, 2023

View reviewed changes

WeichenXu123 added 3 commits April 6, 2023 10:49

update

e35c56b

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

update

1aca1a0

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

update

398352e

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

WeichenXu123 requested a review from hubertzub-db April 6, 2023 10:26

hubertzub-db reviewed Apr 6, 2023

View reviewed changes

mlflow/server/js/src/model-registry/utils/VersionUtil.ts Outdated Show resolved Hide resolved

hubertzub-db approved these changes Apr 6, 2023

View reviewed changes

mlflow/server/js/src/model-registry/components/ModelVersionView.js Outdated Show resolved Hide resolved

WeichenXu123 added 2 commits April 6, 2023 22:57

update

a26c569

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

Merge branch 'master' into bug-8150

5355711

harupy reviewed Apr 11, 2023

View reviewed changes

mlflow/server/js/src/experiment-tracking/routes.js Outdated Show resolved Hide resolved

harupy reviewed Apr 11, 2023

View reviewed changes

mlflow/server/js/src/model-registry/utils/VersionUtils.ts Outdated Show resolved Hide resolved

harupy approved these changes Apr 11, 2023

View reviewed changes

update

cdb2c3a

Signed-off-by: Weichen Xu <weichen.xu@databricks.com>

WeichenXu123 merged commit 4728afe into mlflow:master Apr 12, 2023
25 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: "Source Run" field in Model Version UI page points to wrong run model artifact in Run UI page #8156

Fix: "Source Run" field in Model Version UI page points to wrong run model artifact in Run UI page #8156

WeichenXu123 commented Apr 3, 2023 •

edited

mlflow-automation commented Apr 3, 2023 •

edited

dbczumar Apr 4, 2023

WeichenXu123 Apr 6, 2023

WeichenXu123 Apr 12, 2023

harupy Apr 4, 2023

WeichenXu123 Apr 6, 2023 •

edited

hubertzub-db left a comment

WeichenXu123 commented Apr 10, 2023

harupy Apr 11, 2023

hubertzub-db Apr 11, 2023

harupy Apr 11, 2023

harupy left a comment

Fix: "Source Run" field in Model Version UI page points to wrong run model artifact in Run UI page #8156

Fix: "Source Run" field in Model Version UI page points to wrong run model artifact in Run UI page #8156

Conversation

WeichenXu123 commented Apr 3, 2023 • edited

Related Issues/PRs

What changes are proposed in this pull request?

How is this patch tested?

Does this PR change the documentation?

Release Notes

Is this a user-facing change?

What component(s), interfaces, languages, and integrations does this PR affect?

How should the PR be classified in the release notes? Choose one:

mlflow-automation commented Apr 3, 2023 • edited

dbczumar Apr 4, 2023

Choose a reason for hiding this comment

WeichenXu123 Apr 6, 2023

Choose a reason for hiding this comment

WeichenXu123 Apr 12, 2023

Choose a reason for hiding this comment

harupy Apr 4, 2023

Choose a reason for hiding this comment

WeichenXu123 Apr 6, 2023 • edited

Choose a reason for hiding this comment

hubertzub-db left a comment

Choose a reason for hiding this comment

WeichenXu123 commented Apr 10, 2023

harupy Apr 11, 2023

Choose a reason for hiding this comment

hubertzub-db Apr 11, 2023

Choose a reason for hiding this comment

harupy Apr 11, 2023

Choose a reason for hiding this comment

harupy left a comment

Choose a reason for hiding this comment

WeichenXu123 commented Apr 3, 2023 •

edited

mlflow-automation commented Apr 3, 2023 •

edited

WeichenXu123 Apr 6, 2023 •

edited