feat: update ml system related UI #12334

yoonhyejin · 2025-01-14T10:01:52Z

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

codecov · 2025-01-14T10:22:57Z

Codecov Report

Attention: Patch coverage is 42.73504% with 335 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...p/entity/mlModelGroup/profile/ModelGroupModels.tsx	39.60%	122 Missing ⚠️
...essInstance/profile/DataProcessInstanceSummary.tsx	37.86%	64 Missing ⚠️
.../src/app/entity/mlModel/profile/MLModelSummary.tsx	37.89%	59 Missing ⚠️
...atahub-web-react/src/app/shared/time/timeUtils.tsx	11.11%	32 Missing ⚠️
...b-web-react/src/app/preview/DefaultPreviewCard.tsx	69.33%	23 Missing ⚠️
.../dataProcessInstance/DataProcessInstanceEntity.tsx	32.00%	17 Missing ⚠️
...app/entity/dataProcessInstance/preview/Preview.tsx	0.00%	10 Missing ⚠️
...b/api/entities/dataprocess/dataprocess_instance.py	81.81%	6 Missing ⚠️
datahub-web-react/src/app/entity/EntityPage.tsx	0.00%	1 Missing ⚠️
...web-react/src/app/entity/mlModel/MLModelEntity.tsx	0.00%	1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (81.81%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Files with missing lines	Coverage Δ
...tahub-web-react/src/app/entity/shared/constants.ts	`94.95% <100.00%> (ø)`
datahub-web-react/src/app/entity/EntityPage.tsx	`29.03% <0.00%> (ø)`
...web-react/src/app/entity/mlModel/MLModelEntity.tsx	`40.11% <0.00%> (ø)`
...b/api/entities/dataprocess/dataprocess_instance.py	`72.14% <81.81%> (ø)`
...app/entity/dataProcessInstance/preview/Preview.tsx	`19.41% <0.00%> (ø)`
.../dataProcessInstance/DataProcessInstanceEntity.tsx	`31.38% <32.00%> (ø)`
...b-web-react/src/app/preview/DefaultPreviewCard.tsx	`88.14% <69.33%> (ø)`
...atahub-web-react/src/app/shared/time/timeUtils.tsx	`41.39% <11.11%> (ø)`
.../src/app/entity/mlModel/profile/MLModelSummary.tsx	`32.85% <37.89%> (ø)`
...essInstance/profile/DataProcessInstanceSummary.tsx	`37.86% <37.86%> (ø)`
... and 1 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ac0dc6...946854c. Read the comment docs.

yoonhyejin · 2025-01-14T23:28:57Z

datahub-web-react/src/app/entity/mlModel/profile/MLModelSummary.tsx

+            <Typography.Title level={3}>Model Details</Typography.Title>
+                <InfoItemContainer justifyContent="left">
+                    {/* TODO: should use versionProperties? */}
+                    <InfoItem title="Version">


@RyanHolstien should we use the versionProperties.version for this as well? are we going to deprecate the properties.version?

yoonhyejin · 2025-01-14T23:29:31Z

datahub-graphql-core/src/main/resources/entity.graphql

+"""
+Represents lineage information for ML entities.
+"""
+type MLModelLineageInfo {


let me know if this is valid approach @shirshanka

This looks right, just needs to have the backend mapping on the resolvers.

yoonhyejin · 2025-01-14T23:30:48Z

datahub-web-react/src/app/entity/mlModel/profile/MLModelSummary.tsx

+    };
+
+    const renderTrainingJobs = () => {
+        const lineageTrainingJobs = model?.properties?.mlModelLineageInfo?.trainingJobs || [];


let me know if this is a valid approach @shirshanka

would there be any difference between pulling properties > trainingjobs or pulling relationships > trained by?

yoonhyejin · 2025-01-14T23:32:37Z

datahub-web-react/src/app/entity/mlModel/profile/MLModelSummary.tsx

+                    </InfoItem>
+                    <InfoItem title="Aliases">
+                        <InfoItemContent>
+                            {/* use versionProperties for aliases */}


commenting this out until this get merged : #12166

…s return version set

…at/update-mlflow-ui

ryota-cloud · 2025-01-23T18:42:02Z

metadata-ingestion/examples/ml/create_ml.py

+
+
+@dataclass
+class Container:


at first glance, it took me some time to associate container to experiment. does it make sense to change class name to Experiement? unless there is reason to keep generic term "container"

yoonhyejin · 2025-01-24T05:01:29Z

datahub-web-react/src/app/entity/dataProcessInstance/preview/Preview.tsx

-// status,
-// startTime,
-{
+    duration,


@asikowitz we're adding some of the properties that are specific to data process instance in preview -- let me know if this makes sense

in v2 UI we have a different method of passing values down that I prefer. That method isn't supported in v1 so we can just do this here, but we'll have to coordinate with Chris on getting this to v2.

yoonhyejin · 2025-01-24T05:04:25Z

datahub-web-react/src/app/preview/DefaultPreviewCard.tsx

@@ -270,7 +312,9 @@ export default function DefaultPreviewCard({
        event.stopPropagation();
    };

-    const shouldShowRightColumn = (topUsers && topUsers.length > 0) || (owners && owners.length > 0);
+    const statusPillColor = status === 'SUCCESS' ? 'green' : 'red';
+    const shouldShowRightColumn =


@asikowitz added additional condition to shouldShowRightColumn -- lmk if this makes sense

yoonhyejin · 2025-01-24T05:25:18Z

metadata-ingestion-modules/airflow-plugin/tests/unit/test_airflow.py

@@ -317,7 +317,7 @@ def test_lineage_backend(mock_emit, inlets, outlets, capture_executions):
        assert all(map(lambda let: isinstance(let, Dataset), op2.outlets))

        # Check that the right things were emitted.
-        assert mock_emitter.emit.call_count == 19 if capture_executions else 11
+        assert mock_emitter.emit.call_count == 20 if capture_executions else 11


@asikowitz we're modifyng this test since we have dataplatforminstance in the mock calls

yoonhyejin · 2025-01-24T05:26:19Z

metadata-ingestion-modules/prefect-plugin/tests/unit/test_datahub_emitter.py

@@ -619,206 +619,178 @@ def test_emit_flow(
        mock_emitter.method_calls[11][1][0].entityUrn
        == "urn:li:dataProcessInstance:56231547bcc2781e0c14182ceab6c9ac"
    )
+    assert mock_emitter.method_calls[12][1][0].aspectName == "dataPlatformInstance"


@asikowitz same here -- assertion has been pushed back by 1

Hmm, this is kinda awkward to change. Maybe we could just check if the method calls has certain entries, rather than requiring a specific index to be a certain call

yoonhyejin · 2025-01-24T05:53:46Z

datahub-web-react/src/app/entity/dataProcessInstance/DataProcessInstanceEntity.tsx

                dataProduct={getDataProduct(genericProperties?.dataProduct)}
                externalUrl={data.properties?.externalUrl}
                parentContainers={data.parentContainers}
-                parentEntities={parentEntities}
+                parentEntities={parentEntities as unknown as PreviewEntity[]}


@asikowitz this was my attempt to fix the yarn type-check -- let me know if this makes sense

This should be fixed once you change the type. You can add:

import { Entity as GraphQLEntity } from '@types'; ``` and use that

asikowitz

Hi Hyejin, great work on getting this all working! This is a big PR so I left a lot of comments; it'd probably be good to break this up into a few PRs, e.g. ingestion, backend, and frontend, to be able to go through a smaller set of comments. Most comments are just about style, because we have a lot of ways of doing things, and some are more outdated / no longer recommended. Happy to talk through anything if you have any questions, and feel free to disagree with any of the comments if you have a different opinion.

asikowitz · 2025-01-16T08:20:39Z

datahub-web-react/src/app/preview/DefaultPreviewCard.tsx

@@ -380,6 +419,39 @@ export default function DefaultPreviewCard({
            </LeftColumn>
            {shouldShowRightColumn && (
                <RightColumn key="right-column">
+                                       {startTime && (


If this new code is only for data process, maybe we can put it in a separate function and describe it as such

asikowitz · 2025-01-16T08:24:05Z

datahub-web-react/src/app/shared/time/timeUtils.tsx

@@ -206,3 +206,39 @@ export function getTimeRangeDescription(startDate: moment.Moment | null, endDate

    return 'Unknown time range';
 }
+
+export function formatDuration(durationMs: number): string {


It won't exactly line up, but you could also use moment.js for this: https://momentjs.com/docs/#/durations/humanize/

asikowitz · 2025-01-16T08:24:13Z

datahub-web-react/src/app/shared/time/timeUtils.tsx

+    const hours = Math.floor(durationMs / 1000 / 3600);
+
+    if (hours === 0 && minutes === 0) {
+        return `${seconds}secs`;


Space in between?

asikowitz · 2025-01-22T17:10:42Z

...ub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/search/SearchUtils.java

@@ -88,7 +88,8 @@ private SearchUtils() {}
          EntityType.DATA_PRODUCT,
          EntityType.NOTEBOOK,
          EntityType.BUSINESS_ATTRIBUTE,
-          EntityType.SCHEMA_FIELD);
+          EntityType.SCHEMA_FIELD,
+          EntityType.DATA_PROCESS_INSTANCE);


Do we want these to appear in search?

asikowitz · 2025-01-22T17:11:47Z

...va/com/linkedin/datahub/graphql/types/dataprocessinst/mappers/DataProcessInstanceMapper.java

+      com.linkedin.datahub.graphql.generated.AuditStamp created =
+          AuditStampMapper.map(context, dataProcessInstanceProperties.getCreated());
+      properties.setCreated(created);


asikowitz · 2025-01-26T23:24:11Z

metadata-ingestion-modules/prefect-plugin/tests/unit/test_datahub_emitter.py

@@ -619,206 +619,178 @@ def test_emit_flow(
        mock_emitter.method_calls[11][1][0].entityUrn
        == "urn:li:dataProcessInstance:56231547bcc2781e0c14182ceab6c9ac"
    )
+    assert mock_emitter.method_calls[12][1][0].aspectName == "dataPlatformInstance"


Hmm, this is kinda awkward to change. Maybe we could just check if the method calls has certain entries, rather than requiring a specific index to be a certain call

asikowitz · 2025-01-26T23:24:50Z

metadata-ingestion/examples/ml/create_ml.py

+
+
+@dataclass
+class Container:


asikowitz · 2025-01-26T23:25:36Z

metadata-ingestion/src/datahub/api/entities/dataprocess/dataprocess_instance.py

@@ -71,6 +77,10 @@ class DataProcessInstance:
    _template_object: Optional[Union[DataJob, DataFlow]] = field(
        init=False, default=None, repr=False
    )
+    data_platform: Optional[str] = None


Generally we call this just platform. Also, perhaps confusing that we allow specifying both orchestrator and data_platform. I think we should only allow one, or we should have validation that only lets you set one.

asikowitz · 2025-01-26T23:29:43Z

metadata-ingestion/src/datahub/api/entities/dataprocess/dataprocess_instance.py

+        if self.data_plaform_instance is None and self.cluster is not None:
+            self.data_plaform_instance = self.cluster


I don't think cluster and data_platform_instance are the same concept

asikowitz · 2025-01-26T23:31:57Z

metadata-ingestion/src/datahub/api/entities/dataprocess/dataprocess_instance.py

+        :param clone_inlets: (bool) whether to clone datajob's inlets
+        :param clone_outlets: (bool) whether to clone datajob's outlets


Do you want these params in this method? Feels like the data process instance doesn't have much meaning without these

yoonhyejin added 3 commits January 14, 2025 18:08

[wip] update ui changes

7008a23

fix typo

6148597

add mlmodellineageinfo

bbcc2d4

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Jan 14, 2025

vercel bot deployed to Preview January 14, 2025 10:19 View deployment

yoonhyejin commented Jan 14, 2025

View reviewed changes

Merge branch 'master' into feat/update-mlflow-ui

963b060

vercel bot deployed to Preview January 15, 2025 00:03 View deployment

update mapper for lineageinfo and add uni tests

8dd3c25

vercel bot deployed to Preview January 15, 2025 14:03 View deployment

yoonhyejin added 2 commits January 16, 2025 00:14

[wip] add script for creating mlmodel

2046691

spotlessapply

a5f9a3f

vercel bot deployed to Preview January 15, 2025 15:35 View deployment

asikowitz and others added 2 commits January 15, 2025 13:16

feat(graphql/versioning): Add versioning support to graphql; mutation…

054bc1f

…s return version set

add back fragments.graphql

06bc8c1

vercel bot deployed to Preview January 16, 2025 00:24 View deployment

RyanHolstien and others added 2 commits January 15, 2025 19:08

fix tests

5a6923c

Merge remote-tracking branch 'andrew-fork/versioning-graphql' into fe…

69da53a

…at/update-mlflow-ui

vercel bot deployed to Preview January 16, 2025 04:38 View deployment

fix fragments.graphql

314b869

vercel bot deployed to Preview January 16, 2025 07:50 View deployment

[wip] fix ui bugs

43e3d07

vercel bot deployed to Preview January 16, 2025 15:50 View deployment

yoonhyejin added 3 commits January 17, 2025 11:29

fix rendering issues

e365b06

add run event in script

88db564

update script and add islinegae:True back

b62ef19

fix searchfilters cypress test

398450c

vercel bot deployed to Preview January 22, 2025 20:02 View deployment

shirshanka and others added 2 commits January 22, 2025 16:27

some golden file updates

8cdc27d

Add argparser for token (#6)

c52a462

vercel bot deployed to Preview January 23, 2025 00:59 View deployment

update goldens

b198047

vercel bot deployed to Preview January 23, 2025 15:18 View deployment

fix preview entity

e48a7a6

vercel bot deployed to Preview January 23, 2025 15:54 View deployment

ryota-cloud reviewed Jan 23, 2025

View reviewed changes

Merge branch 'master' into feat/update-mlflow-ui

426d80c

vercel bot deployed to Preview January 24, 2025 00:01 View deployment

Merge branch 'master' into feat/update-mlflow-ui

0d99464

vercel bot deployed to Preview January 24, 2025 04:09 View deployment

remove commented out code

2f4aaea

yoonhyejin commented Jan 24, 2025

View reviewed changes

vercel bot deployed to Preview January 24, 2025 05:17 View deployment

yoonhyejin added 2 commits January 24, 2025 14:19

fix empty ml model message

9f7f298

remove console logs

946854c

yoonhyejin commented Jan 24, 2025

View reviewed changes

vercel bot deployed to Preview January 24, 2025 05:56 View deployment

yoonhyejin marked this pull request as ready for review January 24, 2025 23:33

yoonhyejin changed the title ~~[WIP] feat: update mlflow UI~~ feat: update ml system related UI Jan 24, 2025

datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Jan 24, 2025

asikowitz requested changes Jan 26, 2025

View reviewed changes

datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Jan 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update ml system related UI #12334

feat: update ml system related UI #12334

yoonhyejin commented Jan 14, 2025

codecov bot commented Jan 14, 2025 •

edited

Loading

yoonhyejin Jan 14, 2025

yoonhyejin Jan 14, 2025

RyanHolstien Jan 14, 2025

yoonhyejin Jan 14, 2025

yoonhyejin Jan 14, 2025

ryota-cloud Jan 23, 2025

asikowitz Jan 26, 2025

yoonhyejin Jan 24, 2025

asikowitz Jan 26, 2025

yoonhyejin Jan 24, 2025

yoonhyejin Jan 24, 2025

yoonhyejin Jan 24, 2025

asikowitz Jan 26, 2025

yoonhyejin Jan 24, 2025

asikowitz Jan 24, 2025

asikowitz left a comment

asikowitz Jan 16, 2025

asikowitz Jan 16, 2025

asikowitz Jan 16, 2025

asikowitz Jan 22, 2025

asikowitz Jan 22, 2025

asikowitz Jan 26, 2025

asikowitz Jan 26, 2025

asikowitz Jan 26, 2025

asikowitz Jan 26, 2025

asikowitz Jan 26, 2025

		if self.data_plaform_instance is None and self.cluster is not None:
		self.data_plaform_instance = self.cluster

		:param clone_inlets: (bool) whether to clone datajob's inlets
		:param clone_outlets: (bool) whether to clone datajob's outlets

feat: update ml system related UI #12334

Are you sure you want to change the base?

feat: update ml system related UI #12334

Conversation

yoonhyejin commented Jan 14, 2025

Checklist

codecov bot commented Jan 14, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asikowitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2025 •

edited

Loading