feat(frontend) Support Input/Output from MLMD for V2-compatible. Fix #5670

[zijianjoy]

Description of your changes:
Fix #5670

Features:

1. Shows execution name and link to execution page at the top of page.
2. Shows input/output parameters by reading MLMD custom properties input:key, output:key
3. Shows input/output artifacts with hyperlinks by reading events associated with the current execution.

Sample screenshots:

Checklist:

• The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

[Bobgy]

Leaving some nit comments after a detailed review~

[Bobgy]

/lgtm
I think looks great! I looked forward to seeing proper input/outputs tab for v2 compatible mode for a while! It'll be a great productivity improvement too.

[Bobgy]

nit for discussion: I personally prefer avoiding reduce, because it's not very intuitive, also it doesn't really improve readability much (compared to loops).

Other styles like using Object.entries() and Object.fromEntries() might make the code easier to read too.

But anyway, you can decide on this. Just saying some opinions.

[zijianjoy]

I think both approaches have similar effect/scalability, and I will gladly change to Object.entries() approach for readability.

[Bobgy]

FYI, built test image is gcr.io/ml-pipeline-test/2c5ad35de81b5f94bfc67cff0e3951de7f831d64/frontend:latest.
You can test it in your own cluster now.

[Bobgy]

Some UX comments after trying out the built image with my pipelines.

Comment 1:

When there are no input artifacts, the blank space below "Artifacts" title feels a little awkward:

I have two ideas:

• we may add a text hint there "There are no input artifacts."
• we may omit the input artifacts section altogether, (but when an execution doesn't have any inputs/outputs, we should add a hint text "there are no inputs and outputs").

I personally prefer omitting empty sections, because it makes information density higher on the tab.

Comment 2:
I feel a little uncomfortable seeing the separator between inputs and outputs, because it's a new type of UI element on KFP UI (especially the color is new, we don't use dark grey for sth that does not need strong attention.)

I also have 2 ideas:

• In KFP's design language, we can introduce a separator with lighter grey color, so it doesn't stand out so much
• or we simply don't add a separator there. I felt that having Input and Output title was enough.

I'm leaning towards not adding a separator, but I don't have a strong opinion on this one.

Comment 3:

If I'm browsing through the list and found an interesting item, then I want to know which type is it (input/output param/artifact).
I need to look at three different places:

1. artifact name
2. find its wrapping title to find out it's artifact or parameter
3. find out the wrapping area whether it's input or output (this will be even harder especially when number of output parameters is larger, then Output section title is farther away from the Output Artifacts section).

In original design, I'd only need to look at 1 & 2, because titles are inlined:

Also the titles are only two words, they are not too long.

[zijianjoy]

This is very detailed and clear comment about the UX discussion! Thank you @Bobgy for writing it!

Some UX comments after trying out the built image with my pipelines.

Comment 1:

When there are no input artifacts, the blank space below "Artifacts" title feels a little awkward

I agree with hiding the I/O param/artifact section when there is none. And showing a banner if nothing exists also makes sense. Note that loading artifacts has network delay because UI needs to query events and artifacts in async call, so artifacts sections won't show at the beginning until the query completes. I am leaning towards keeping the title so user knows that there is no artifact, rather than silently hiding the whole section. But I agree that there is some awkward space if no artifact exists. I have implemented your suggestion in this PR, we can discuss more if users feel differently.

Comment 2:
I feel a little uncomfortable seeing the separator between inputs and outputs, because it's a new type of UI element on KFP UI (especially the color is new, we don't use dark grey for sth that does not need strong attention.)

I think separator is useful because input/output is closely located on UI, which is hard to distinguish them. But I agree that we should discuss more before introducing a new UX style. I am going to keep the current UI consistent with before, and we can come up with ideas on how to better render dense information on a side panel. See screenshot:

Comment 3:
If I'm browsing through the list and found an interesting item, then I want to know which type is it (input/output param/artifact).
I need to look at three different places:

1. artifact name
2. find its wrapping title to find out it's artifact or parameter
3. find out the wrapping area whether it's input or output (this will be even harder especially when number of output parameters is larger, then Output section title is farther away from the Output Artifacts section).

In original design, I'd only need to look at 1 & 2, because titles are inlined.

Totally agree with this comment, I will make the titles consistent with before.

[Bobgy]

This is very detailed and clear comment about the UX discussion! Thank you @Bobgy for writing it!

Some UX comments after trying out the built image with my pipelines.

Comment 1:

When there are no input artifacts, the blank space below "Artifacts" title feels a little awkward

I agree with hiding the I/O param/artifact section when there is none. And showing a banner if nothing exists also makes sense. Note that loading artifacts has network delay because UI needs to query events and artifacts in async call, so artifacts sections won't show at the beginning until the query completes. I am leaning towards keeping the title so user knows that there is no artifact, rather than silently hiding the whole section. But I agree that there is some awkward space if no artifact exists. I have implemented your suggestion in this PR, we can discuss more if users feel differently.

I think you raised a good point. It would feel weird to find sth like a loading indicator and then it disappears after getting the response.

The best loading experience demo I have seen for loading multiple things at the same time is react suspense for data (it was demoed years ago, but still not released as stable. You might find the demo interesting https://youtu.be/nLF0n9SACd4 https://reactjs.org/docs/concurrent-mode-suspense.html
https://react-query.tanstack.com/guides/suspense).

Because MLMD requests are usually fast, the easiest fix now is to wait for all requests to finish, before showing anything.

And one corner case is looking at running workflow, ideally when the running node refreshes automatically, we can show stale data while fetching new data.

You can decide on the tradeoff, I suggest starting with sth simple with reasonable UX.

Comment 2:
I feel a little uncomfortable seeing the separator between inputs and outputs, because it's a new type of UI element on KFP UI (especially the color is new, we don't use dark grey for sth that does not need strong attention.)

I think separator is useful because input/output is closely located on UI, which is hard to distinguish them. But I agree that we should discuss more before introducing a new UX style. I am going to keep the current UI consistent with before, and we can come up with ideas on how to better render dense information on a side panel. See screenshot:

I felt that with the fix in comment 3, I will be able to easily distinguish them.

One easy way to improve on the separator design is to follow a pattern of an existing usage of separator (height, color, margin). It will be a lighter grey color and do not stand out so much.

[Bobgy]

/lgtm
/approve

/hold
In case you want to change anything, feel free to unhold

[google-oss-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• frontend/OWNERS [Bobgy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[zijianjoy]

I think you raised a good point. It would feel weird to find sth like a loading indicator and then it disappears after getting the response.

The best loading experience demo I have seen for loading multiple things at the same time is react suspense for data (it was demoed years ago, but still not released as stable. You might find the demo interesting https://youtu.be/nLF0n9SACd4 https://reactjs.org/docs/concurrent-mode-suspense.html
https://react-query.tanstack.com/guides/suspense).

Because MLMD requests are usually fast, the easiest fix now is to wait for all requests to finish, before showing anything.

And one corner case is looking at running workflow, ideally when the running node refreshes automatically, we can show stale data while fetching new data.

You can decide on the tradeoff, I suggest starting with sth simple with reasonable UX.

Discussed offline, we need to wait for suspense to graduate from experimental to make use of it. Thank you for sharing! Currently we use execution state as query key for parent component, so we will be able to update child component for output artifacts when the execution finishes.

Comment 2:

I felt that with the fix in comment 3, I will be able to easily distinguish them.

One easy way to improve on the separator design is to follow a pattern of an existing usage of separator (height, color, margin). It will be a lighter grey color and do not stand out so much.

I prefer to use separator which is different from table row separator, otherwise I am introducing separators with similar styles but different meanings. To avoid further confusion, I will not add separator at the moment. We can discuss more if users hope to see Input/Output differently.

[zijianjoy]

/unhold

Thank you Yuan for your detailed review!