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

TEP-0090: Matrix [Problem Statement] #532

Merged
merged 1 commit into from
Nov 12, 2021
Merged

Conversation

jerop
Copy link
Member

@jerop jerop commented Oct 6, 2021

https://github.com/jerop/community/blob/looping/teps/0090-matrix.md

This change adds the problem statement for Matrix. It scopes the
problem, describes the use cases, and identifies the goals, the
requirements for the solution, and related work in other continuous
delivery systems.

Today, users cannot supply varying Parameters to execute a
PipelineTask, that is, fan out a PipelineTasks.

To solve this problem, this TEP aims to enable executing the same
PipelineTask with different combinations of Parameters specified
in a matrix. TaskRuns or Runs will be created with variables
substituted with each combination of the Parameters in the matrix.

This matrix construct will enable users to specify concise but
powerful Pipelines. Moreover, it would improve the composability,
scalability, flexibility and reusability of Tekton Pipelines.

References:

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2021
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 6, 2021
@vdemeester vdemeester added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Oct 11, 2021
@jerop
Copy link
Member Author

jerop commented Oct 11, 2021

/assign @tlawrie

@tekton-robot
Copy link
Contributor

@jerop: GitHub didn't allow me to assign the following users: tlawrie.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @tlawrie

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.

@tlawrie
Copy link
Contributor

tlawrie commented Oct 11, 2021

Hey @jerop thanks for bringing this up on the alternate WG. Is this TEP in reference to https://github.com/tektoncd/experimental/tree/main/task-loops#task-loop-extension? and is it about bringing that in as base functionality rather than via a customtask?

@bobcatfish
Copy link
Contributor

/assign

@jerop
Copy link
Member Author

jerop commented Oct 13, 2021

Hey @jerop thanks for bringing this up on the alternate WG.

hi @tlawrie 👋🏾

Is this TEP in reference to https://github.com/tektoncd/experimental/tree/main/task-loops#task-loop-extension?

yes, this is related to that custom task

and is it about bringing that in as base functionality rather than via a customtask?

it's mostly about discussing where we want the looping construct to go -- it could start with making it stable and maybe eventually adding it to the pipelines api

wrote another tep to establish the graduation requirements and path for custom tasks in #523

in this pr, we're specifically defining the problem statement - will follow up with another pr with a proposal after we agree on the goals, non-goals, use cases and requirements

i saw this issue - boomerang-io/community#23 - from your project and i'm eager to hear the realworld use cases that you're working to solve using the counter task type discussed there

@jerop jerop marked this pull request as ready for review October 13, 2021 16:24
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 13, 2021
@jerop jerop force-pushed the looping branch 3 times, most recently from 9ff168b to 91eae33 Compare October 13, 2021 18:45
@jerop jerop changed the title TEP-0090: Looping TEP-0090: Looping [Problem Statement] Oct 13, 2021
@jerop jerop force-pushed the looping branch 2 times, most recently from d2620f0 to 487c312 Compare October 13, 2021 19:26
Copy link
Contributor

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice problem statement!!

/approve

teps/0090-looping.md Outdated Show resolved Hide resolved
@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2021
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2021
@tlawrie
Copy link
Contributor

tlawrie commented Oct 18, 2021

Hi @jerop awesome. Ill look into this a bit further this week and also ask the boomerang-io/roadmap community around the use cases for looping.

I believe our main use case is around looping for a group of tasks. I.e. in a workflow loop task 3 + 4 + 5.

@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2021
@jerop jerop force-pushed the looping branch 3 times, most recently from d58c97d to d1d8a0b Compare October 18, 2021 15:32
@tlawrie
Copy link
Contributor

tlawrie commented Oct 28, 2021

Hi @jerop my vote is to still keep in the generation of x TaskRuns based on a prior TaskRuns Results if possible. That is the use case that most relates to our usage.

@jerop
Copy link
Member Author

jerop commented Oct 28, 2021

Hi @jerop my vote is to still keep in the generation of x TaskRuns based on a prior TaskRuns Results if possible. That is the use case that most relates to our usage.

The use cases we have in the TEP right now could also use the dynamic generation using Results from previous TaskRuns (included a description of this in each of the use cases). However, we may benefit from taking an iterative approach where the initial version uses user-supplied Parameters only then we explore the dynamic generation as the next step considering the feedback that we gather. So the dynamic generation is definitely not out of the picture.

Added this item to the API WG agenda so we can discuss further on Monday (cc @imjasonh @bobcatfish @vdemeester)

@jerop
Copy link
Member Author

jerop commented Oct 28, 2021

/assign @pritidesai (thanks!)

@pritidesai
Copy link
Member

@Tomcli please review the proposal here and let us know if we are missing anything based on kubeflow use case, thanks 🙏

@pritidesai
Copy link
Member

/assign

@Tomcli
Copy link
Contributor

Tomcli commented Oct 28, 2021

thanks @pritidesai @jerop I think this is a great start to add the looping support. Looking forward to the new Matrix/looping syntax.

@jerop jerop force-pushed the looping branch 2 times, most recently from 3a65cf9 to 5c78998 Compare October 28, 2021 23:08
@jerop jerop force-pushed the looping branch 2 times, most recently from eb31ce6 to 09fbae0 Compare November 8, 2021 17:02
@jerop
Copy link
Member Author

jerop commented Nov 8, 2021

@bobcatfish @tlawrie @imjasonh @squee1945 @vdemeester @Tomcli @pritidesai thank you for the reviews and discussions at the API WG 🤗

Updated the TEP by adding the following requirements:

  • The Parameters in the matrix can use Results from previous TaskRuns or Runs to dynamically generate TaskRuns or Runs from a given PipelineTask.
  • Configuring the maximum number of TaskRuns or Runs generated in a given matrix should be supported, with a
    default value provided.

We will implement the feature iteratively, with the dynamic generation using Results coming later (will be mentioned in the proposal section in the next PR).

Also highlighting that dynamic generation of TaskRuns or Runs from a matrix means that different executions of a given Pipeline may generate different number of TaskRuns or Runs based on the size of the Results. It does not mean that there's a feed of Results that generate TaskRuns or Runs that may never stop - note that this is not possible because Tekton Pipelines is a DAG and parent PipelineTasks have to be completed (thus no longer producing Results) before a child PipelineTask is executed.

@jerop jerop force-pushed the looping branch 2 times, most recently from 1fb50a7 to 3d3dd6f Compare November 8, 2021 22:13
@bobcatfish
Copy link
Contributor

sgtm!

@tlawrie
Copy link
Contributor

tlawrie commented Nov 9, 2021

@jerop fabulous work getting it all aligned. Thank you.

Copy link
Member

@pritidesai pritidesai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @jerop for putting this together! Though I am listed as one of the authors, its all your efforts 🙏 and approving them 😄

/approve

I am very excited to design and implement this. I am a little nervous with the term combination of parameters specially when it's compared to experimental task loop (iterating over a list of items). But it can devised while designing it.

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, pritidesai, vdemeester

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:

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

This change adds the problem statement for Matrix. It scopes the
problem, describes the use cases, and identifies the goals, the
requirements for the solution, and related work in other continuous
delivery systems.

Today, users cannot supply varying `Parameters` to execute a
`PipelineTask`, that is, fan out a `PipelineTasks`.

To solve this problem, this TEP aims to enable executing the same
`PipelineTask` with different combinations of `Parameters` specified
in a `matrix`. `TaskRuns` or `Runs` will be created with variables
substituted with each combination of the `Parameters` in the `matrix`.

This `matrix` construct will enable users to specify concise but
powerful `Pipelines`. Moreover, it would improve the composability,
scalability, flexibility and reusability of *Tekton Pipelines*.

https://github.com/jerop/community/blob/looping/teps/0090-matrix.md

References:
- [Task Loops Experimental Project][task-loops]
- Issues:
  - tektoncd/pipeline#2050
  - tektoncd/pipeline#4097

[task-loops]: https://github.com/tektoncd/experimental/tree/main/task-loops
@imjasonh
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2021
@tekton-robot tekton-robot merged commit 761124f into tektoncd:main Nov 12, 2021
This was referenced Jan 9, 2022
@jerop jerop deleted the looping branch January 29, 2022 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
Status: Proposed
Development

Successfully merging this pull request may close these issues.

10 participants