Fix edge cases such as disable istio-inject for Tekton compiler (#119)

* update flag to disable istio-inject, also remove some pyyaml conflicts * move license header as a variable * move license header as global var
2020-04-24 17:43:28 -07:00 · 2020-04-24 17:43:28 -07:00 · 4f6a564dc0
parent f1b560eb06
commit 4f6a564dc0
29 changed files with 103 additions and 57 deletions
--- a/sdk/python/kfp_tekton/compiler/_op_to_template.py
+++ b/sdk/python/kfp_tekton/compiler/_op_to_template.py
@ -26,18 +26,6 @@ import re
 from .. import tekton_api_version


-class literal_str(str):
-    """Literal string class for pyyaml
-
-    Literal string class is used for converting string with newline into
-    yaml's literal string format with '|'. In pyyaml, literal string
-    conversion is not natively supported in the default dumper.
-    Therefore, we need to define this class as part of the dumper
-    before compiling it into yaml.
-    """
-    pass
-
-
 def _get_base_step(name: str):
    """Base image step for running bash commands.

@ -329,12 +317,11 @@ def _process_output_artifacts(outputs_dict: Dict[Text, Any],
        mounted_artifact_paths = []
        for artifact in outputs_dict['artifacts']:
            if artifact['name'] in replaced_param_list:
-                print(replaced_param_list)
                copy_artifacts_step['script'] = copy_artifacts_step['script'] + \
-                    'mc cp $(results.%s.path) storage/%s/runs/$PIPELINERUN/$PODNAME/%s' % (artifact_to_result_mapping[artifact['name']], bucket, artifact['path'].rsplit("/", 1)[1])
+                    'mc cp $(results.%s.path) storage/%s/runs/$PIPELINERUN/$PODNAME/%s\n' % (artifact_to_result_mapping[artifact['name']], bucket, artifact['path'].rsplit("/", 1)[1])
            else:
                copy_artifacts_step['script'] = copy_artifacts_step['script'] + \
-                    'mc cp %s storage/%s/runs/$PIPELINERUN/$PODNAME/%s' % (artifact['path'], bucket, artifact['path'].rsplit("/", 1)[1])
+                    'mc cp %s storage/%s/runs/$PIPELINERUN/$PODNAME/%s\n' % (artifact['path'], bucket, artifact['path'].rsplit("/", 1)[1])
                if artifact['path'].rsplit("/", 1)[0] not in mounted_artifact_paths:
                    volume_mount_step_template.append({'name': artifact['name'], 'mountPath': artifact['path'].rsplit("/", 1)[0]})
                    volume_template.append({'name': artifact['name'], 'emptyDir': {}})
@ -445,17 +432,6 @@ def _op_to_template(op: BaseOp, enable_artifacts=False):
    if processed_op.init_containers:
        template['spec']['steps'] = _prepend_steps(processed_op.init_containers, template['spec']['steps'])

-    # initial base_step and volume setup
-    base_step = {
-        'image': 'busybox',
-        'name': 'copy-results',
-        'script': '#!/bin/sh\nset -exo pipefail\n'
-    }
-    volume_mount_step_template = []
-    volume_template = []
-    mounted_param_paths = []
-    replaced_param_list = []
-
    # inputs
    input_artifact_paths = processed_op.input_artifact_paths if isinstance(processed_op, dsl.ContainerOp) else None
    artifact_arguments = processed_op.artifact_arguments if isinstance(processed_op, dsl.ContainerOp) else None
@ -475,7 +451,6 @@ def _op_to_template(op: BaseOp, enable_artifacts=False):
            mount_path = artifact['path'].rsplit("/", 1)[0]
            if mount_path not in mounted_param_paths:
                _add_mount_path(artifact['name'], artifact['path'], mount_path, volume_mount_step_template, volume_template, mounted_param_paths)
-        copy_inputs_step['script'] = literal_str(copy_inputs_step['script'])
        template['spec']['steps'] = _prepend_steps([copy_inputs_step], template['spec']['steps'])
        _update_volumes(template, volume_mount_step_template, volume_template)

@ -502,11 +477,9 @@ def _op_to_template(op: BaseOp, enable_artifacts=False):
                                                        replaced_param_list,
                                                        artifact_to_result_mapping)
        if mounted_param_paths:
-            copy_results_step['script'] = literal_str(copy_results_step['script'])
            template['spec']['steps'].append(copy_results_step)
        _update_volumes(template, volume_mount_step_template, volume_template)
        if copy_artifacts_step:
-            copy_artifacts_step['script'] = literal_str(copy_artifacts_step['script'])
            template['spec']['steps'].append(copy_artifacts_step)

    # **********************************************************
--- a/sdk/python/kfp_tekton/compiler/compiler.py
+++ b/sdk/python/kfp_tekton/compiler/compiler.py
@ -23,7 +23,7 @@ import logging
 import textwrap
 from typing import Callable, Set, List, Text, Dict, Tuple, Any, Union, Optional

-from ._op_to_template import _op_to_template, literal_str
+from ._op_to_template import _op_to_template

 from kfp import dsl
 from kfp.compiler._default_transformers import add_pod_env
@ -36,18 +36,6 @@ from kfp.compiler._k8s_helper import convert_k8s_obj_to_json
 from .. import tekton_api_version


-def _literal_str_representer(dumper, data):
-  """pyyaml representer for literal yaml string dumper
-
-  Create a representer for the literal string class that converts the string
-  object with newline into yaml's literal string '|' style.
-  """
-  return dumper.represent_scalar(u'tag:yaml.org,2002:str', data, style='|')
-
-# Add the _literal_str_representer as part of the yaml dumper.
-yaml.add_representer(literal_str, _literal_str_representer)
-
-
 class TektonCompiler(Compiler) :
  """DSL Compiler to generate Tekton YAML.

@ -296,7 +284,7 @@ class TektonCompiler(Compiler) :
                      valueFrom: '%s'
              """ % (value[0], value[1]))
              output_values += output_value
-            tp['value'] = literal_str(output_values)
+            tp['value'] = output_values


  def _workflow_with_pipelinerun(self, task_refs, pipeline, pipeline_template, workflow):
@ -497,7 +485,11 @@ class TektonCompiler(Compiler) :
      'apiVersion': tekton_api_version,
      'kind': 'Pipeline',
      'metadata': {
-        'name': pipeline.name or 'Pipeline'
+        'name': pipeline.name or 'Pipeline',
+        'annotations': {
+          # Disable Istio inject since Tekton cannot run with Istio sidecar
+          'sidecar.istio.io/inject': 'false'
+        }
      },
      'spec': {
        'params': params,
--- a/sdk/python/tests/compiler/compiler_tests.py
+++ b/sdk/python/tests/compiler/compiler_tests.py
@ -18,6 +18,7 @@ import tempfile
 import unittest
 import yaml
 import re
+import textwrap

 from kfp_tekton import compiler

@ -26,6 +27,24 @@ from kfp_tekton import compiler
 # in order to generate new "golden" YAML files
 GENERATE_GOLDEN_YAML = False

+# License header for Kubeflow project
+LICENSE_HEADER = textwrap.dedent("""\
+# Copyright 2020 kubeflow.org
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+""")
+

 class TestTektonCompiler(unittest.TestCase):

@ -236,6 +255,8 @@ class TestTektonCompiler(unittest.TestCase):
        compiled = list(yaml.safe_load_all(f))
      if GENERATE_GOLDEN_YAML:
        with open(golden_yaml_file, 'w') as f:
+          f.write(LICENSE_HEADER)
+        with open(golden_yaml_file, 'a+') as f:
          yaml.dump_all(compiled, f, default_flow_style=False)
      else:
        with open(golden_yaml_file, 'r') as f:
--- a/sdk/python/tests/compiler/testdata/affinity.yaml
+++ b/sdk/python/tests/compiler/testdata/affinity.yaml
@ -29,6 +29,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "A pipeline with affinity",
      "name": "affinity"}'
+    sidecar.istio.io/inject: 'false'
  name: affinity
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/artifact_location.yaml
+++ b/sdk/python/tests/compiler/testdata/artifact_location.yaml
@ -57,10 +57,14 @@ spec:
          name: $(inputs.params.secret_name)
    image: minio/mc
    name: copy-artifacts
-    script: |-
-      #!/usr/bin/env sh
-      mc config host add storage http://minio-service.$(inputs.params.namespace):9000 $AWS_ACCESS_KEY_ID $AWS_SECRET_ACCESS_KEY
+    script: '#!/usr/bin/env sh
+
+      mc config host add storage http://minio-service.$(inputs.params.namespace):9000
+      $AWS_ACCESS_KEY_ID $AWS_SECRET_ACCESS_KEY
+
      mc cp $(results.output.path) storage/$(inputs.params.bucket)/runs/$PIPELINERUN/$PODNAME/output.txt
+
+      '
 ---
 apiVersion: tekton.dev/v1beta1
 kind: Pipeline
@ -74,6 +78,7 @@ metadata:
      true, "type": "String"}, {"default": "kubeflow", "name": "namespace", "optional":
      true, "type": "String"}, {"default": "mlpipeline", "name": "bucket", "optional":
      true, "type": "String"}], "name": "custom_artifact_location_pipeline"}'
+    sidecar.istio.io/inject: 'false'
  name: custom-artifact-location-pipeline
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/condition.yaml
+++ b/sdk/python/tests/compiler/testdata/condition.yaml
@ -136,6 +136,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "shows how to use dsl.Condition.",
      "name": "pipeline flip coin"}'
+    sidecar.istio.io/inject: 'false'
  name: pipeline-flip-coin
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/hidden_output_file.yaml
+++ b/sdk/python/tests/compiler/testdata/hidden_output_file.yaml
@ -34,10 +34,13 @@ spec:
    name: download-file
  - image: busybox
    name: copy-results
-    script: |
-      #!/bin/sh
+    script: '#!/bin/sh
+
      set -exo pipefail
+
      cp /tmp/results.txt $(results.data.path);
+
+      '
  volumes:
  - emptyDir: {}
    name: data
@ -65,6 +68,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "Run a script that passes
      file to a non configurable path", "name": "Hidden output file pipeline"}'
+    sidecar.istio.io/inject: 'false'
  name: hidden-output-file-pipeline
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/imagepullsecrets.yaml
+++ b/sdk/python/tests/compiler/testdata/imagepullsecrets.yaml
@ -39,6 +39,7 @@ metadata:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "Get Most Frequent Word
      and Save to GCS", "inputs": [{"default": "This is testing", "name": "message",
      "optional": true}], "name": "Save Most Frequent"}'
+    sidecar.istio.io/inject: 'false'
  name: save-most-frequent
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/init_container.yaml
+++ b/sdk/python/tests/compiler/testdata/init_container.yaml
@ -35,6 +35,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "A pipeline with init container.",
      "name": "InitContainer"}'
+    sidecar.istio.io/inject: 'false'
  name: initcontainer
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/input_artifact_raw_value.yaml
+++ b/sdk/python/tests/compiler/testdata/input_artifact_raw_value.yaml
@ -24,10 +24,13 @@ spec:
  steps:
  - image: busybox
    name: copy-inputs
-    script: |
-      #!/bin/sh
+    script: '#!/bin/sh
+
      set -exo pipefail
+
      echo -n "Constant artifact value" > /tmp/inputs/text/data
+
+      '
  - command:
    - cat
    - /tmp/inputs/text/data
@ -49,10 +52,13 @@ spec:
  steps:
  - image: busybox
    name: copy-inputs
-    script: |
-      #!/bin/sh
+    script: '#!/bin/sh
+
      set -exo pipefail
+
      echo -n "Constant artifact value" > /tmp/inputs/text/data
+
+      '
  - command:
    - cat
    - /tmp/inputs/text/data
@ -74,10 +80,13 @@ spec:
  steps:
  - image: busybox
    name: copy-inputs
-    script: |
-      #!/bin/sh
+    script: '#!/bin/sh
+
      set -exo pipefail
+
      echo -n "hard-coded artifact value" > /tmp/inputs/text/data
+
+      '
  - command:
    - cat
    - /tmp/inputs/text/data
@ -99,11 +108,15 @@ spec:
  steps:
  - image: busybox
    name: copy-inputs
-    script: |
-      #!/bin/sh
+    script: '#!/bin/sh
+
      set -exo pipefail
+
      echo -n "Text from a file with hard-coded artifact value
+
      " > /tmp/inputs/text/data
+
+      '
  - command:
    - cat
    - /tmp/inputs/text/data
@ -120,6 +133,7 @@ metadata:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "Pipeline shows how to
      define artifact inputs and pass raw artifacts to them.", "name": "Pipeline with
      artifact input raw argument value."}'
+    sidecar.istio.io/inject: 'false'
  name: pipeline-with-artifact-input-raw-argument-value
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/katib.yaml
+++ b/sdk/python/tests/compiler/testdata/katib.yaml
@ -97,6 +97,7 @@ metadata:
      {"default": "60", "name": "experimentTimeoutMinutes", "optional": true}, {"default":
      "True", "name": "deleteAfterDone", "optional": true}], "name": "Launch katib
      experiment"}'
+    sidecar.istio.io/inject: 'false'
  name: launch-katib-experiment
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/loop_static.yaml
+++ b/sdk/python/tests/compiler/testdata/loop_static.yaml
@ -67,6 +67,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "10", "name": "my_pipe_param",
      "optional": true}], "name": "my-pipeline"}'
+    sidecar.istio.io/inject: 'false'
  name: my-pipeline
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/node_selector.yaml
+++ b/sdk/python/tests/compiler/testdata/node_selector.yaml
@ -29,6 +29,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "A pipeline with Node Selector",
      "name": "node_selector"}'
+    sidecar.istio.io/inject: 'false'
  name: node-selector
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/parallel_join.yaml
+++ b/sdk/python/tests/compiler/testdata/parallel_join.yaml
@ -81,6 +81,7 @@ metadata:
      in parallel and prints the concatenated result.", "inputs": [{"default": "gs://ml-pipeline-playground/shakespeare1.txt",
      "name": "url1", "optional": true}, {"default": "gs://ml-pipeline-playground/shakespeare2.txt",
      "name": "url2", "optional": true}], "name": "Parallel pipeline"}'
+    sidecar.istio.io/inject: 'false'
  name: parallel-pipeline
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/parallel_join_pipelinerun.yaml
+++ b/sdk/python/tests/compiler/testdata/parallel_join_pipelinerun.yaml
@ -81,6 +81,7 @@ metadata:
      in parallel and prints the concatenated result.", "inputs": [{"default": "gs://ml-pipeline-playground/shakespeare1.txt",
      "name": "url1", "optional": true}, {"default": "gs://ml-pipeline-playground/shakespeare2.txt",
      "name": "url2", "optional": true}], "name": "Parallel pipeline"}'
+    sidecar.istio.io/inject: 'false'
  name: parallel-pipeline
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/parallel_join_with_argo_vars.yaml
+++ b/sdk/python/tests/compiler/testdata/parallel_join_with_argo_vars.yaml
@ -1,3 +1,17 @@
+# Copyright 2020 kubeflow.org
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
 apiVersion: tekton.dev/v1beta1
 kind: Task
 metadata:
@ -68,6 +82,7 @@ metadata:
      [{"default": "gs://ml-pipeline-playground/shakespeare1.txt", "name": "url1",
      "optional": true}, {"default": "gs://ml-pipeline-playground/shakespeare2.txt",
      "name": "url2", "optional": true}], "name": "Parallel pipeline with argo vars"}'
+    sidecar.istio.io/inject: 'false'
  name: parallel-pipeline-with-argo-vars
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/pipeline_transformers.yaml
+++ b/sdk/python/tests/compiler/testdata/pipeline_transformers.yaml
@ -51,6 +51,7 @@ metadata:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "The pipeline shows how
      to apply functions to all ops in the pipeline by pipeline transformers", "name":
      "Pipeline transformer"}'
+    sidecar.istio.io/inject: 'false'
  name: pipeline-transformer
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/pipelineparams.yaml
+++ b/sdk/python/tests/compiler/testdata/pipelineparams.yaml
@ -64,6 +64,7 @@ metadata:
      pipeline params.", "inputs": [{"default": "latest", "name": "tag", "optional":
      true, "type": "String"}, {"default": "10", "name": "sleep_ms", "optional": true,
      "type": "Integer"}], "name": "PipelineParams"}'
+    sidecar.istio.io/inject: 'false'
  name: pipelineparams
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/resourceop_basic.yaml
+++ b/sdk/python/tests/compiler/testdata/resourceop_basic.yaml
@ -73,6 +73,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "A Basic Example on ResourceOp
      Usage.", "name": "ResourceOp Basic"}'
+    sidecar.istio.io/inject: 'false'
  name: resourceop-basic
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/retry.yaml
+++ b/sdk/python/tests/compiler/testdata/retry.yaml
@ -51,6 +51,7 @@ metadata:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "The pipeline includes
      two steps which fail randomly. It shows how to use ContainerOp(...).set_retry(...).",
      "name": "Retry random failures"}'
+    sidecar.istio.io/inject: 'false'
  name: retry-random-failures
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/sequential.yaml
+++ b/sdk/python/tests/compiler/testdata/sequential.yaml
@ -55,6 +55,7 @@ metadata:
      steps.", "inputs": [{"default": "gs://ml-pipeline-playground/shakespeare1.txt",
      "name": "url", "optional": true}, {"default": "/tmp/results.txt", "name": "path",
      "optional": true}], "name": "Sequential pipeline"}'
+    sidecar.istio.io/inject: 'false'
  name: sequential-pipeline
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/sidecar.yaml
+++ b/sdk/python/tests/compiler/testdata/sidecar.yaml
@ -56,6 +56,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "A pipeline with sidecars.",
      "name": "Sidecar"}'
+    sidecar.istio.io/inject: 'false'
  name: sidecar
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/timeout.yaml
+++ b/sdk/python/tests/compiler/testdata/timeout.yaml
@ -48,6 +48,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "shows how to use ContainerOp
      set_timeout().", "name": "pipeline includes two steps which fail randomly."}'
+    sidecar.istio.io/inject: 'false'
  name: pipeline-includes-two-steps-which-fail-randomly
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/timeout_pipelinerun.yaml
+++ b/sdk/python/tests/compiler/testdata/timeout_pipelinerun.yaml
@ -48,6 +48,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "shows how to use ContainerOp
      set_timeout().", "name": "pipeline includes two steps which fail randomly."}'
+    sidecar.istio.io/inject: 'false'
  name: pipeline-includes-two-steps-which-fail-randomly
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/tolerations.yaml
+++ b/sdk/python/tests/compiler/testdata/tolerations.yaml
@ -35,6 +35,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "A pipeline with tolerations",
      "name": "tolerations"}'
+    sidecar.istio.io/inject: 'false'
  name: tolerations
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/volume.yaml
+++ b/sdk/python/tests/compiler/testdata/volume.yaml
@ -63,6 +63,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "A pipeline with volume.",
      "name": "Volume"}'
+    sidecar.istio.io/inject: 'false'
  name: volume
 spec:
  params: []
--- a/sdk/python/tests/compiler/testdata/volume_op.yaml
+++ b/sdk/python/tests/compiler/testdata/volume_op.yaml
@ -102,6 +102,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"description": "A Basic Example on VolumeOp
      Usage.", "inputs": [{"name": "size"}], "name": "VolumeOp Basic"}'
+    sidecar.istio.io/inject: 'false'
  name: volumeop-basic
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/volume_snapshot_op.yaml
+++ b/sdk/python/tests/compiler/testdata/volume_snapshot_op.yaml
@ -355,6 +355,7 @@ metadata:
      the design doc. Please enable the\n    volume snapshot feature gate in order
      to run this pipeline.", "inputs": [{"name": "url"}], "name": "VolumeSnapshotOp
      Sequential"}'
+    sidecar.istio.io/inject: 'false'
  name: volumesnapshotop-sequential
 spec:
  params:
--- a/sdk/python/tests/compiler/testdata/withitem_nested.yaml
+++ b/sdk/python/tests/compiler/testdata/withitem_nested.yaml
@ -86,6 +86,7 @@ metadata:
  annotations:
    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "10", "name": "my_pipe_param",
      "optional": true, "type": "Integer"}], "name": "my-pipeline"}'
+    sidecar.istio.io/inject: 'false'
  name: my-pipeline
 spec:
  params: