Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group UI nodes based on templateRef #11106

Closed
JPZ13 opened this issue May 19, 2023 · 10 comments · Fixed by #13511
Closed

Group UI nodes based on templateRef #11106

JPZ13 opened this issue May 19, 2023 · 10 comments · Fixed by #13511

Comments

@JPZ13
Copy link
Member

JPZ13 commented May 19, 2023

Summary

I was chatting with @mshatkhin23, and he mentioned that he'd like to have the workflow graph grouped based on the workflow template ref. Given this workflow snippet (using templates instead of refs here for simplicity):

- name: dag-tier-3
  dag:
    tasks:
      - name: task-1
        template: flakey-container
      - name: task-2
        depends: 'task-1'
        template: flakey-container
- name: dag-tier-2
  dag:
    tasks:
      - name: task-1
        template: dag-tier-3
      - name: task-2
        depends: 'task-1'
        template: dag-tier-3
- name: user-workflow
  dag:
    tasks:
      - name: task-1
        template: dag-tier-2
      - name: task-2
        depends: 'task-1'
        template: dag-tier-2

It produces the following graph:
image

We'd like to find a way to group or collapse things based on a given template. In the graph above, it's hard to know which task-1 is in user-workflow or dag-tier-2 or dag-tier-3. Ideally we:

  • collapse templateRefs from the start
  • adjust the display name so that the task name also includes which template is invoking it

Use Cases

This should help eliminate confusion when looking at the Argo Workflows graph. As workflows scale and orgs manage lots of templates, there will naturally be naming collisions that arise


Message from the maintainers:

Love this enhancement proposal? Give it a 👍. We prioritise the proposals with the most 👍.

@JPZ13 JPZ13 added the type/feature Feature request label May 19, 2023
@terrytangyuan
Copy link
Member

Great idea

@mshatkhin23
Copy link

mshatkhin23 commented May 21, 2023

Thanks for posting @JPZ13 !

Small addition to adjust the display name so that the task name also includes which template is invoking it - it would also be nice to make these tasks-in-dag-template (or steps-in-steps-template) relationships visually apparent, so that the developer can clearly discern between template layers. Threw together some mock-ups below to illustrate what I mean (clearly I'm no designer 😅). Definitely still some more thought needed around questions like how to discern between parallel tasks and template-layers, but I think you get the idea.

image
image
image

@dpeer6
Copy link
Contributor

dpeer6 commented May 22, 2023

@mshatkhin23 I love the idea.
Also any other idea/suggestion that will allow easy rendering of big or huge workflows.
If it can be based on a state (like Failed or Error) or by label/template (inner template not templateRef).

@or-shachar
Copy link
Contributor

Hey everyone!
I think that the option to group the graph is amazing! I think that grouping by template is only one option but if we go to that direction we should allow a more generic way to group.

Here's an example of a "complex" workflow that can be presented a 3 groups:
(imagine how more complex it can be if we had 15 test groups, and that the app update takes 5 jobs, and that we have 4 different layers of terraform)

- name: deployment
  dag:
    tasks:
     # First update infrastructure using terraform
      - name: terraform-plan
        template: terraform-plan
        group: infra

      - name: terraform-apply
        depends: 'task-1'
        template: terraform-apply
        group: infra
     
      # Then update app backend and frontend
      - name: update-backend
        depends: 'terraform-apply'
        template: argo-update
        group: app

     - name: update-cdn
        depends: update-backend
        template: update-cdn
        group: app

     # Then run validation
     - name: backend-e2e-A
        depends: update-backend
        template: backend-e2e
        arguments:
          parameters:
          - name: test_group
            value: "A"
        group: validation

     - name: backend-e2e-B
        depends: update-backend
        template: backend-e2e
        arguments:
          parameters:
          - name: test_group
            value: "B"
        group: validation

     - name: backend-e2e-C
        depends: update-backend
        template: backend-e2e
        arguments:
          parameters:
          - name: test_group
            value: "C"
        group: validation

     - name: frontend-tests
        depends: update-frontend
        template: frontend-tests
        group: validation

@mshatkhin23
Copy link

mshatkhin23 commented May 26, 2023

@or-shachar rly like the idea! Could we accomplish this through metadata labels/annotations you think? Although exposing as a top-level task/steps attribute does make things a bit cleaner.

We could maybe even have a group-based config field to expose style options? Although this would certainly just be a nice-to-have. For example,

groupConfig:
    - name: infra
      value: '{"color": "red", "border": "blue"}'
    - name: app
      value: ...
...

Also do you imagine group to have a meaning other than UI grouping? (nothing obvious comes to mind)

@eirisdg
Copy link

eirisdg commented Sep 14, 2023

I think is a good idea.

In this example, this should be collapsed by default:
image

I'm using empty workflows to group them, but can't collapse.

@GandophSmida

This comment was marked as duplicate.

@Adrien-D
Copy link
Member

I'd be happy to handle this one

@agilgur5 agilgur5 changed the title Group UI nodes based on workflow template ref Group UI nodes based on templateRef Jul 31, 2024
@agilgur5
Copy link
Member

agilgur5 commented Jul 31, 2024

I think that grouping by template is only one option but if we go to that direction we should allow a more generic way to group.

It would be a good bit more complicated to add something new to the spec, especially one that is UI-facing only (i.e. not used by the Controller) vs using an existing field.

  • collapse templateRefs from the start

Collapsibility I think would be easier than grouping in general, since IIRC the graph layout is entirely algorithmic, based on a Coffman-Graham Sort and not a custom layout.

Pluggable sorting/layout algorithms could potentially make sense for #6945

@Adrien-D
Copy link
Member

After taking a look on how to handle this issue, here is where I am at :

  • That could be nice to have 3 switchable distinct layouts
    • The existing one based on children props
    • A collapsing one, which would be the existing one but collapsed by templateRef

adrien-out yaml

  • A grouping/collapsing one, based on templateRef and boundaryID which would be pretty similar to one required

adrien-out yaml (1)

The only drawback I found to handle this only working on the UI, is the missing link between step depending on an other one, represented here in red arrows on this image. We don't have this information at the moment.

adrien-out yaml (1)

If you have an insight on this matter that would be really helpful, otherwise, in my opinion it might already be a good step forward and I can start working on implementing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment