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

feat: New variable iterationNumberParam to track current item. #898

Merged
merged 4 commits into from
Apr 1, 2022

Conversation

ScrapCodes
Copy link
Contributor

Which issue is resolved by this Pull Request:
Resolves #889

Description of your changes:
Added new variable iterationNumberParam to track current item.
Environment tested:

  • Python Version (use python --version):
  • Tekton Version (use tkn version):
  • Kubernetes Version (use kubectl version):
  • OS (e.g. from /etc/os-release):

Checklist:

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ScrapCodes

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

@yhwang
Copy link
Member

yhwang commented Mar 29, 2022

@Tomcli please correct me if I am wrong. I think the main purpose of this is to add iterationNumberParam when iterateParam is used. in this case, a user can use both iterateParam and iterationNumberParam to get the param for the current iteration as well as the current iteration number. so the iterationNumberParam should be added in the condition section here: https://github.com/kubeflow/kfp-tekton/blob/master/tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelinelooprun.go#L879

Edit:
I tried the iterateNumeric, its value represents the current iteration number already. just wondering in the from/to case, is iterateParam (which represents the current iteration param) needed?

if tls.IterationNumberParam != "" {
out = append(out, v1beta1.Param{
Name: tls.IterationNumberParam,
Value: v1beta1.ArrayOrString{Type: v1beta1.ParamTypeString, StringVal: currentIterationItem},
Copy link
Member

Choose a reason for hiding this comment

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

this makes iterationNumberParam is equivalent to iterateParam. its value should be the iteration

@Tomcli
Copy link
Member

Tomcli commented Mar 29, 2022

@Udiknedormin @wzhanw quick question, when Feng and Gang implemented the pipelineLoop controller last year, the iteration is counted from 1 to n as counter. However, the python programming enumerate() format is counted from 0 to n-1 as indices. So do you want the new iterationNumberParam Tekton variable to count from 0 or 1?

@ScrapCodes
Copy link
Contributor Author

Hi, please correct me - IterateParam may be nil, incase of numeric iteration loop. In that case, iterationNumberParam will point to a number.

@wzhanw
Copy link
Contributor

wzhanw commented Mar 30, 2022

@Tomcli I will check with Feng and Gang. I think we can use the iteration.

@yhwang
Copy link
Member

yhwang commented Mar 30, 2022

@ScrapCodes except using 0 or 1 as the initial index number, we should directly use iteration as the value of iterationNumberParam. it aligns with David's comment

@rafalbigaj
Copy link
Contributor

rafalbigaj commented Mar 31, 2022

@Tomcli @yhwang @wzhanw @ScrapCodes we need to have 2 independent parameters:

  • pointed by iterateParam or iterateNumeric - to hold loop collection item or range element.
  • pointed by iterationNumberParam - to hold iteration number starting from 1. It works consistently for loop collection or ranges.

Examples:

  1. Loop over text "a,b,c" with separator "," will generate 3 iterations with parameter values:
  • iterateParam -> "a", iterationNumberParam -> 1
  • iterateParam -> "b", iterationNumberParam -> 2
  • iterateParam -> "c", iterationNumberParam -> 3
  1. Loop over collection [1,5,19]will generate 3 iterations with parameter values:
  • iterateParam -> 1, iterationNumberParam -> 1
  • iterateParam -> 5, iterationNumberParam -> 2
  • iterateParam -> 19, iterationNumberParam -> 3
  1. Loop over range from 1 to 5 by 2 will generate 3 iterations with parameter values:
  • iterateNumeric -> 1, iterationNumberParam -> 1
  • iterateNumeric -> 3, iterationNumberParam -> 2
  • iterateNumeric -> 5, iterationNumberParam -> 3

Please let me know if any further explanation is needed.

This is an important issue with high priority. Could you know ETA for it?

BTW, wouldn't name iterationIndex work better than iterationNumberParam?

name: pipelineloop
spec:
# IterationNumberParam points to the param that stores the current loop item. Similar to IterateNumeric
# for numeric loop range.
Copy link
Contributor

Choose a reason for hiding this comment

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

IterationNumberParam points rather to collection or range index.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add the note that it's index in-order and that it starts from 1.

@ScrapCodes
Copy link
Contributor Author

ScrapCodes commented Mar 31, 2022

BTW, wouldn't name iterationIndex work better than iterationNumberParam?

Indeed!

spec:
# IterationNumberParam points to the param that stores the current loop item. Similar to IterateNumeric
# for numeric loop range.
iterationNumberParam: current-message
Copy link
Contributor

Choose a reason for hiding this comment

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

current-message is a very misleading name here. It's usage, echo "$(params.message) $(params.current-message)", doesn't make it any better.

Maybe message-index would do better? And echo "$(params.message-index)) $(params.message)", so that it's shown as:

1) I am the first one
2) I am the second one
3) I am the third one

@rafalbigaj
Copy link
Contributor

rafalbigaj commented Mar 31, 2022

@ScrapCodes I have realised after discussion with @Udiknedormin that 1-based numbering is more consistent with PipelineLoop status details. Therefore, I think we can stay with iterationNumberParam. Thanks!

@ScrapCodes
Copy link
Contributor Author

Thank you for taking a look @rafalbigaj, @Udiknedormin, @yhwang, @Tomcli and @wzhanw.
For some reason, IBM cloud is not working(for me) and thus, I could not verify by running it against a real cluster.

@yhwang
Copy link
Member

yhwang commented Mar 31, 2022

@rafalbigaj
just want to confirm again about item 3:

3. Loop over range from 1 to 5 by 2 will generate 3 iterations with parameter values:

iterateNumeric -> 1, iterationNumberParam -> 1
iterateNumeric -> 3, iterationNumberParam -> 2
iterateNumeric -> 5, iterationNumberParam -> 3

This changes the existing behavior. I guess you are aware of it and want to use the new iterationNumberParam to represent the iteration index and iterateNumeric to represent the value.

@rafalbigaj
Copy link
Contributor

@yhwang to be honest I don't really understand why we have 2 params: iterateParam and iterateNumeric. iterateNumeric was never documented.

In all cases we need access to the iteration item (element) and number (index).

@yhwang
Copy link
Member

yhwang commented Apr 1, 2022

@rafalbigaj agree with you. but for now, I suggest that we keep iterateNumeric but change its value to iteration item(element) and add iterationNumberParam for number(index). Just a reminder that iterateNumeric will change to the iteration item after this PR.

If you want to keep iterateNumeric as-is and change it in another PR please let us know.

@rafalbigaj
Copy link
Contributor

@yhwang I'm OK with that. Thanks!

@rafalbigaj
Copy link
Contributor

FYI: @wzhanw @Udiknedormin

@Tomcli
Copy link
Member

Tomcli commented Apr 1, 2022

e2e test passed for this PR, @yhwang please comment in this PR if you need any spec/behavioral changes for the SDK.
https://cloud.ibm.com/devops/pipelines/tekton/f85a45d2-36fc-45fb-a449-b01c13cf5191/runs/dc8f0240-fbb3-4148-a667-79132c15ed51/test/run-go-unittests?env_id=ibm:yp:us-south

@yhwang
Copy link
Member

yhwang commented Apr 1, 2022

@Tomcli the current implementation works well with my SDK change. But for this PR and based on Rafal's comment, we need to update the value of iterateNumeric to item value.

@Tomcli
Copy link
Member

Tomcli commented Apr 1, 2022

I personal would recommend to change the iterateNumeric in another PR. This way if the iterateNumeric is broken, we don't have to revert back the iterationNumberParam changes.

@Tomcli
Copy link
Member

Tomcli commented Apr 1, 2022

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Apr 1, 2022
@google-oss-prow google-oss-prow bot merged commit 7b692b6 into kubeflow:master Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update PipelineLoop controller to add new Tekton variable "iterationNumberParam"
6 participants