Repository: kubeflow/kfp-tekton (language: python)

Issue:
Loop.range generates inconsistent number of iterations when start=end
/kind bug

**What steps did you take and what happened:**

Reproduce using the following code

``` python
from kfp import dsl, components
from kfp_tekton.tekton import Loop

op1_yaml = '''\
name: 'my-in-coop1'
inputs:
- {name: item, type: Integer}
- {name: param, type: Integer}
implementation:
    container:
        image: library/bash:4.4.23
        command: ['sh', '-c']
        args:
        - |
          set -e
          echo op1 "$0" "$1"
        - {inputValue: item}
        - {inputValue: param}
'''


@dsl.pipeline(name='pipeline')
def pipeline():
    with Loop.range(start=0, step=1, end=0) as item:
        op1_template = components.load_component_from_text(op1_yaml)
        op1 = op1_template(item, 10)

if __name__ == '__main__':
  from kfp_tekton.compiler import TektonCompiler as Compiler
  from kfp_tekton.compiler.pipeline_utils import TektonPipelineConf
  tekton_pipeline_conf = TektonPipelineConf()
  tekton_pipeline_conf.set_tekton_inline_spec(True)
  # tekton_pipeline_conf.set_resource_in_separate_yaml(True)
  Compiler().compile(pipeline, __file__.replace('.py', '.yaml'), tekton_pipeline_conf=tekton_pipeline_conf)
```

This generates 0 iterations.

**What did you expect to happen:**
I expect this to generate 1 iteration given that the loop `Loop.range(start=0, step=1, end=1)` generates 2 iterations.

**Additional information:**
[Miscellaneous information that will assist in solving the issue.]


**Environment:**

* Python Version (use `python --version`):
* SDK Version: 
* Tekton Version (use `tkn version`):
* Kubernetes Version (use `kubectl version`):
* OS (e.g. from `/etc/os-release`):



Discussion / hints:
/assign @ScrapCodes 

@ScrapCodes are you able to take over this issue?
Are you sure, if start = end = 0, and step = 1 then generate one iteration?
If start = 0 and end =1, step = 1, then generate two iteration seems like a bug to me. WDYT?
@Tomcli, @yhwang and @typeness 
to summarize, correct me if I am wrong,
Current behaviors:
- start = end = 0, and step = 1:
  0 iteration
- start=0, step=1, end=1:
  2 iterations

for me, I think the following behaviors make sense to me:
- start = end = 0, and step = 1:
  0 iteration
- start=0, step=1, end=1:
  1 iteration


I think what DataStage wants is not the exact behavior of Python range. It's similar to the requirement of enumerate where the index is counted from 1 instead of 0. 

@Udiknedormin start = 0 end = 0, and step = n should be always one iteration in this case right?
@Tomcli got it. but because the pipeline-loops is on the kfp-tekton repo. We need to document the behaviors for community users.
yes we should

Fix patch:
diff --git a/tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelinelooprun.go b/tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelinelooprun.go
index aba1290618..5143efba6c 100644
--- a/tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelinelooprun.go
+++ b/tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelinelooprun.go
@@ -790,18 +790,22 @@ func computeIterations(run *v1alpha1.Run, tls *pipelineloopv1alpha1.PipelineLoop
 	from := 0
 	step := 1
 	to := 0
+	fromProvided := false
+	toProvided := false
 	iterationElements := []interface{}{}
 	iterationParamStr := ""
 	iterationParamStrSeparator := ""
 	for _, p := range run.Spec.Params {
 		if p.Name == "from" {
 			from, _ = strconv.Atoi(p.Value.StringVal)
+			fromProvided = true
 		}
 		if p.Name == "step" {
 			step, _ = strconv.Atoi(p.Value.StringVal)
 		}
 		if p.Name == "to" {
 			to, _ = strconv.Atoi(p.Value.StringVal)
+			toProvided = true
 		}
 		if p.Name == tls.IterateParam {
 			if p.Value.Type == v1beta1.ParamTypeString {
@@ -872,10 +876,16 @@ func computeIterations(run *v1alpha1.Run, tls *pipelineloopv1alpha1.PipelineLoop
 			}
 		}
 	}
-	if from != to {
+	if from != to && fromProvided && toProvided {
 		if step == 0 {
 			return 0, iterationElements, fmt.Errorf("invalid values step: %d found in runs", step)
 		}
+		if (to-from < step && step > 0) || (to-from > step && step < 0) {
+			// This is a special case, to emulate "python's enumerate" behaviour see issue #935
+			numberOfIterations = 1
+			iterationElements = append(iterationElements, from)
+			return numberOfIterations, iterationElements, nil
+		}
 		if (from > to && step > 0) || (from < to && step < 0) {
 			return 0, iterationElements, fmt.Errorf("invalid values for from:%d, to:%d & step: %d found in runs", from, to, step)
 		}
@@ -892,6 +902,11 @@ func computeIterations(run *v1alpha1.Run, tls *pipelineloopv1alpha1.PipelineLoop
 			}
 		}
 	}
+	if from == to && step != 0 && fromProvided && toProvided {
+		// This is a special case, to emulate "python's enumerate" behaviour see issue #935
+		numberOfIterations = 1
+		iterationElements = append(iterationElements, from)
+	}
 	return numberOfIterations, iterationElements, nil
 }
 


Test patch:
diff --git a/tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelineloop_range_test.go b/tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelineloop_range_test.go
index 06f0305970..b284594678 100644
--- a/tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelineloop_range_test.go
+++ b/tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelineloop_range_test.go
@@ -157,10 +157,19 @@ func TestReconcilePipelineLoopRunRange(t *testing.T) {
 		from:                 "1",
 		to:                   "1",
 		step:                 "1",
-		expectedStatus:       corev1.ConditionTrue,
-		expectedReason:       pipelineloopv1alpha1.PipelineLoopRunReasonSucceeded,
-		expectedPipelineruns: []*v1beta1.PipelineRun{},
-		expectedEvents:       []string{"Normal Started ", "Normal Succeeded All PipelineRuns completed successfully"},
+		expectedStatus:       corev1.ConditionUnknown,
+		expectedReason:       pipelineloopv1alpha1.PipelineLoopRunReasonRunning,
+		expectedPipelineruns: []*v1beta1.PipelineRun{expectedPipelineRunWithRange2},
+		expectedEvents:       []string{"Normal Started ", "Normal Running Iterations completed: 0"},
+	}, {
+		name:                 "Case from 0 to 0 and non zero step increment",
+		from:                 "0",
+		step:                 "1",
+		to:                   "0",
+		expectedStatus:       corev1.ConditionUnknown,
+		expectedReason:       pipelineloopv1alpha1.PipelineLoopRunReasonRunning,
+		expectedPipelineruns: []*v1beta1.PipelineRun{expectedPipelineRunWithRange3},
+		expectedEvents:       []string{"Normal Started ", "Normal Running Iterations completed: 0"},
 	}, {
 		name:                 "Case from < to and +ve step increment",
 		from:                 "-1",
@@ -180,32 +189,32 @@ func TestReconcilePipelineLoopRunRange(t *testing.T) {
 		expectedPipelineruns: []*v1beta1.PipelineRun{},
 		expectedEvents:       []string{"Normal Started ", "Warning Failed Cannot determine number of iterations: invalid values step: 0 found in runs"},
 	}, {
-		name:                 "Case from > to and +ve increment",
+		name:                 "Case to - from < step and step > 0",
 		from:                 "1",
 		step:                 "1",
 		to:                   "-1",
-		expectedStatus:       corev1.ConditionFalse,
-		expectedReason:       pipelineloopv1alpha1.PipelineLoopRunReasonFailedValidation,
-		expectedPipelineruns: []*v1beta1.PipelineRun{},
-		expectedEvents:       []string{"Normal Started ", "Warning Failed Cannot determine number of iterations: invalid values for from:1, to:-1 & step: 1 found in runs"},
+		expectedStatus:       corev1.ConditionUnknown,
+		expectedReason:       pipelineloopv1alpha1.PipelineLoopRunReasonRunning,
+		expectedPipelineruns: []*v1beta1.PipelineRun{expectedPipelineRunWithRange2},
+		expectedEvents:       []string{"Normal Started ", "Normal Running Iterations completed: 0"},
 	}, {
-		name:                 "Case from > to and +ve step increment",
+		name:                 "Case to - from < step and step > 0",
 		from:                 "0",
 		step:                 "1",
 		to:                   "-1",
-		expectedStatus:       corev1.ConditionFalse,
-		expectedReason:       pipelineloopv1alpha1.PipelineLoopRunReasonFailedValidation,
-		expectedPipelineruns: []*v1beta1.PipelineRun{},
-		expectedEvents:       []string{"Normal Started ", "Warning Failed Cannot determine number of iterations: invalid values for from:0, to:-1 & step: 1 found in runs"},
+		expectedStatus:       corev1.ConditionUnknown,
+		expectedReason:       pipelineloopv1alpha1.PipelineLoopRunReasonRunning,
+		expectedPipelineruns: []*v1beta1.PipelineRun{expectedPipelineRunWithRange3},
+		expectedEvents:       []string{"Normal Started ", "Normal Running Iterations completed: 0"},
 	}, {
 		name:                 "Case from = to",
 		from:                 "-1",
 		step:                 "1",
 		to:                   "-1",
-		expectedStatus:       corev1.ConditionTrue,
-		expectedReason:       pipelineloopv1alpha1.PipelineLoopRunReasonSucceeded,
-		expectedPipelineruns: []*v1beta1.PipelineRun{},
-		expectedEvents:       []string{"Normal Started ", "Normal Succeeded All PipelineRuns completed successfully"},
+		expectedStatus:       corev1.ConditionUnknown,
+		expectedReason:       pipelineloopv1alpha1.PipelineLoopRunReasonRunning,
+		expectedPipelineruns: []*v1beta1.PipelineRun{expectedPipelineRunWithRange1},
+		expectedEvents:       []string{"Normal Started ", "Normal Running Iterations completed: 0"},
 	}, {
 		name:                 "Case from > to and non -ve step increment",
 		from:                 "1",