feat(frontend): Feature reduce pipeline graph. Fixes #4924

[StefanoFioravanzo]

Description of your changes:

Add a "Transitive Reduction" mechanism to simplify the graph in the Pipeline Details and in the Run Details pages. Activating this function will remove any redundant dependencies in the graph, thus making it simpler to read in many instances.

In case the graph cannot be reduced, the toggle will be disabled. Toggling again this option will revert the graph back to its original layout, so the user can switch it on and off to compare the two views, if needed.

More details at #4924

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.

• Do you want this pull request (PR) cherry-picked into the current release branch?

  Learn more about cherry-picking updates into the release branch.

[Udiknedormin]

Isn't something similar implemented on PipelineVolume's after method (see: my issue comment)? Wouldn't it be better to reduce dependencies directly in the produced Argo yaml instead of the UI?

[Bobgy]

Thank you @StefanoFioravanzo!
Love this idea!

I prefer the generic graph simplification at UI layer.

[StefanoFioravanzo]

@Udiknedormin I answered to your question in the issue. Thanks @Bobgy !

[Bobgy]

Read through code in details, left some thoughts.

A general question: why did you decide to show simplified graph by default?
I think in smaller graphs, seeing all the dependencies can be useful because each dependency means some parameter or artifact passing happens between the two tasks.

In bigger graphs, we might let users click the simplify graph switch by themselves.

[Bobgy]

I'm not seeing why this is useful.
I think hiding the sth dynamically in the UI is generally a bad design, users will be confused, because when the switch doesn't show up. There's no clue why it doesn't.

In this specific case, I think the simplest design is enough:

• always show the simplify graph switch

[StefanoFioravanzo]

So this is used both in RunDetails and PipelineDetails pages to compare the reduced graph vs the original one. If there is a difference then the reduced one gets picked to be displayed.

[StefanoFioravanzo]

@Bobgy Thank you for the detailed review! I will go through all of your comments in a couple of weeks, after the Christmas break.

[StefanoFioravanzo]

@Bobgy I think I addressed all your comments. As for this:

why did you decide to show simplified graph by default?

I agree that in general we should leave it off as default, I changed that here 657bd6d

Also, following up on this #4925 (comment) :
Returning undefined in transitiveReduction makes typescript complain in the tests here

pipelines/frontend/src/lib/StaticGraphParser.test.ts

Line 829 in 208618b

expect(reduced.nodeCount()).toEqual(3);

saying that reduced Object is possibly 'undefined'.. That's obviously true, and I am not sure if we had better just return the unchanged graph instead of undefined or if we should handle this situation in a different way

[Bobgy]

Thanks, I'll take further look this week

[StefanoFioravanzo]

Hey @Bobgy any update here? 😃

[Bobgy]

@StefanoFioravanzo Sorry for the late reply.

Code mostly looks good, I've read through it in details.

Despite some minor nit pickings -- can you write unit tests in RunDetails.test.tsx and PipelineDetails.test.tsx to verify UI behavior after clicking on the simplify graph switch?

[Bobgy]

Personal opinion:
I think there's no need to disable the switch, when reduced graph is the same.
It's understandable -- when the graph is already simple, reducing it has no effect. We've got tooltip on the
switch too to explain this.

What do you think? anyway, this is not a blocker

[StefanoFioravanzo]

I would leave it disabled so that it's obvious that this features is not available for that particular pipeline. Otherwise users might try clicking it anyway, see that nothing changes, and wonder what is happening. I wouldn't rely on people actually reading and understanding what's in the tooltip, we can just prevent any potential misunderstanding by disabling the switch

[StefanoFioravanzo]

@Bobgy about unit tests, I have close to zero experience in writing tests for React components. So please bear with me a little bit here :) See the last commit d080cc9 where I try to test a diamond-shaped pipeline. Creating the graph and testing for its nodes and edges works. Now I want to query for the switch component, simulate its click and test that the graph is missing the edge "node1 to node4". How can I query for the switch element and click it?

[Bobgy]

@StefanoFioravanzo that's a good start,
The test is using @testing-library/react, so you can find documentation in https://testing-library.com/docs/queries/about (note that our version looks slightly outdated).

There's a section that introduces all types of query you can use,
for this case

getByText: Not useful for forms, but this is the number 1 method a user finds most non-interactive elements (like divs and spans).
getByLabelText: Only really good for form fields, but this is the number one method a user finds those elements, so it should be your top preference.

either of this sounds reasonable, because users would find your switch looking at the text on that switch.

You can use https://testing-library.com/docs/dom-testing-library/api-events library to fire the click event easily. (I believe we didn't install it, but we've decided to use @testing-library/ family, so you can just install it).

===

Here are the information,

Anyway, if you still cannot figure out in reasonable time. It's OK to log a remaining issue and leave it there. This PR is already great work!

/cc @zijianjoy to also do some review

[k8s-ci-robot]

@Bobgy: GitHub didn't allow me to request PR reviews from the following users: to, also, do, some, review.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@StefanoFioravanzo that's a good start,
The test is using @testing-library/react, so you can find documentation in https://testing-library.com/docs/queries/about (note that our version looks slightly outdated).

There's a section that introduces all types of query you can use,
for this case

getByText: Not useful for forms, but this is the number 1 method a user finds most non-interactive elements (like divs and spans).
getByLabelText: Only really good for form fields, but this is the number one method a user finds those elements, so it should be your top preference.

either of this sounds reasonable, because users would find your switch looking at the text on that switch.

You can use https://testing-library.com/docs/dom-testing-library/api-events library to fire the click event easily. (I believe we didn't install it, but we've decided to use @testing-library/ family, so you can just install it).

===

Here are the information,

Anyway, if you still cannot figure out in reasonable time. It's OK to log a remaining issue and leave it there. This PR is already great work!

/cc @zijianjoy to also do some review

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[google-oss-robot]

@Bobgy: GitHub didn't allow me to request PR reviews from the following users: review, to, also, do, some.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@StefanoFioravanzo that's a good start,
The test is using @testing-library/react, so you can find documentation in https://testing-library.com/docs/queries/about (note that our version looks slightly outdated).

There's a section that introduces all types of query you can use,
for this case

getByText: Not useful for forms, but this is the number 1 method a user finds most non-interactive elements (like divs and spans).
getByLabelText: Only really good for form fields, but this is the number one method a user finds those elements, so it should be your top preference.

either of this sounds reasonable, because users would find your switch looking at the text on that switch.

You can use https://testing-library.com/docs/dom-testing-library/api-events library to fire the click event easily. (I believe we didn't install it, but we've decided to use @testing-library/ family, so you can just install it).

===

Here are the information,

Anyway, if you still cannot figure out in reasonable time. It's OK to log a remaining issue and leave it there. This PR is already great work!

/cc @zijianjoy to also do some review

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[StefanoFioravanzo]

@Bobgy Thanks that suggestion was spot on! I updated the commit with a complete unit test for the RunDetails page. I gave a look at PipelineDetails as well, but there doesn't seem to be an existing straightforward way to test the structure of a pipeline there, since there is no mock of the Graph component and functions to compare with that. Let me know if the current testing is acceptable and maybe we can improve this in the future.

[Bobgy]

Yes, I think adding one test is enough for coverage.

[Bobgy]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, StefanoFioravanzo

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

[google-oss-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, StefanoFioravanzo

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]

Thank you Stefano for this change! It helps improve pipeline visualization a lot! 👍