Fix special compilation cases for kfp samples

[drewbutlerbb4]

Fixes the testing of kfp samples for pipelines that require special compilation instructions.

Specifically these include;
compose.py
basic_no_decorator.py

[kubeflow-bot]

This change is 

[Tomcli]

If possible, can we generalize the helper functions to take the below sample pipelines as well?
https://github.com/kubeflow/pipelines/blob/master/samples/tutorials/DSL%20-%20Control%20structures/DSL%20-%20Control%20structures.py
https://github.com/kubeflow/pipelines/blob/master/samples/tutorials/Data%20passing%20in%20python%20components/Data%20passing%20in%20python%20components%20-%20Files.py

[drewbutlerbb4]

@Tomcli I believe I have addressed your comments. I can't test on either of the linked pipelines because they have functionality that kfp-tekton can't currently compile (exithandler and input artifacts). I will make another pipeline and test on that shortly.

[Tomcli]

Thanks @drewbutlerbb4, this is great. For the other 2 pipelines in the samples folder, we can add them in when exitOp and input artifacts are ready.

[Tomcli]

/lgtm

[Tomcli]

/assign @animeshsingh

[ckadner]

For the test_kfp_samples.sh script, why do we not just generate PipelineRun for all testdata scripts and only change one line:

  if dsl-compile-tekton --py "${f}" --output "${TEKTON_COMPILED_YAML_DIR}/${f##*/}.yaml" >> "${COMPILER_OUTPUTS_FILE}" 2>&1;

to

  if dsl-compile-tekton --generate-pipelinerun --py "${f}" --output "${TEKTON_COMPILED_YAML_DIR}/${f##*/}.yaml" >> "${COMPILER_OUTPUTS_FILE}" 2>&1;

There is no need or benefit to have a mix here, since we will catch the NotImplementedErrors either way.

[drewbutlerbb4]

Maybe I do not understand, but wouldn't this still fail to compile basic_no_decorator.py and compose.py?

[ckadner]

Hi Andrew, yes, I wrote that comment before understanding the full purpose of your test_util.py. But let me explain the my bigger point below

[ckadner]

should we not move these tests into our standard compiler test suite?

[ckadner]

please move the two functions test_workflow_without_decorator and test_nested_workflow into the actual compiler unit test and have the test_util.py call them and capture the output YAML into a file, like dsl-compile does

[ckadner]

Hi Andrew,

I think the code you wrote in test_util.py is good but it should really be part of our unit test suite for the compiler. In fact the KFP testdata scripts are meant to be used in the compiler unit tests where all the negative cases and special parameters are handled.

The test_kfp_samples.sh script was really just intended to show us which features still need to be implemented on the compiler and give us a kind of score card. It was not the intention to do all we can to make all tests succeed just to make sure we know what was not yet working.

We are at a time in the project where we should add all KFP unit tests in our unit test suite and use that to measure our progress.

On another tangent I have been using the test_kfp_samples.sh script locally to run the compiler over all DSL samples in the KFP repository to get a better sense of which features are actually used by KFP consumers and then use some bash scripting to dissect the compiler console output to computer a list of all errors. See this comment and below on issue #18. I wanted to make that public but it now is kind of conflicting with your change, so I admit some bias on my part :-)

[drewbutlerbb4]

Thanks for the explanation Christian I now understand your point of view better. I agree that this could be placed in the compiler tests along with some new example pipelines and that could help improve our testing.

However I am not entirely sure it shouldn't be included in the test_kfp_samples.sh as well. Based on discussions from our meetings, I believe the output of this script (as well as your new improvements) is being used as a metric to judge the progress of this project by people not working day-to-day on the kfp-tekton? Wouldn't it then be confusing to these people to see pipelines failing when they shouldn't be?

I would like to get @Tomcli opinion on your comments because, admittedly I'm not entirely sure this was the intention when I was assigned this work. That being said I'll defer to @ckadner and @Tomcli on this one.

[Tomcli]

@ckadner I agree we can also use test_util.py as part of our unit test suite for the compiler, but we should also leverage it in test_kfp_samples.sh for the metrics because nested pipelines and no decorator pipelines need special ways to compile them. If you compile them using the regular dsl-compile or dsl-compile-tekton, you will see the below 2 errors

   1 ValueError: There are multiple pipelines: ['save_most_frequent_word', 'download_save_most_frequent_word']. Please specify --function.
   1 ValueError: A function with @dsl.pipeline decorator is required in the py file.

Therefore, I asked Andrew to take those special compile cases and generalize it. This way we can update the list of nested pipelines and no decorator pipelines in the config.yaml and compile them with the required parameters.

[ckadner]

Okay, lets do both: integrate the special test handling in the report script and add 2 unit test cases :-)

[ckadner]

this should move up so we can capture the entire output of the compile run in the ${COMPILER_OUTPUTS_FILE}, not just the SUCCESS or FAILURE. The python3 command should return an exit value of 0 for success and 1 for failure

Maybe we could use a ternary stile approach:

    if ([ -z "${IS_SPECIAL}" ] \
        && dsl-compile-tekton --py "${f}" --output "${TEKTON_COMPILED_YAML_DIR}/${f##*/}.yaml" \
        || python3 -m test_util ${f} ${CONFIG_FILE} ) >> "${COMPILER_OUTPUTS_FILE}" 2>&1 ;
    then
      echo "SUCCESS: ${f##*/}" | tee -a "${COMPILER_OUTPUTS_FILE}"
    else
      echo "FAILURE: ${f##*/}" | tee -a "${COMPILER_OUTPUTS_FILE}"
    fi

...or better.. wrap the compile part into a bash function and inside that function check for IS_SPECIAL:

function compile_dsl {
  if [ -z "${IS_SPECIAL}" ]; then
    dsl-compile-tekton --py "$1" --output "$2"
  else
    python3 -m test_util $1 ${CONFIG_FILE} # need to produce YAML file output
  fi
} 

# ...

    if compile_dsl "${f}" "${TEKTON_COMPILED_YAML_DIR}/${f##*/}.yaml" >> "${COMPILER_OUTPUTS_FILE}" 2>&1 ;
    then
      echo "SUCCESS: ${f##*/}" | tee -a "${COMPILER_OUTPUTS_FILE}"
    else
      echo "FAILURE: ${f##*/}" | tee -a "${COMPILER_OUTPUTS_FILE}"
    fi

[ckadner]

please move the two functions test_workflow_without_decorator and test_nested_workflow into the actual compiler unit test and have the test_util.py call them and capture the output YAML into a file, like dsl-compile does

[ckadner]

@drewbutlerbb4 -- I realize I am requesting substantial changes to your PR, some of which are quite aspirational and could be scoped out into a separate PR. Since I am working on another PR in the test_kfp_samples.sh script anyway, I could incorporate those changes there and we can merge your PR as is. What do you think?

So, 3 stages:

1. we merge your PR as is
2. I create a PR that makes the tesk_kfp_samples.sh work with exit codes and compiler output (along with the actual changes I need for the report)
3. you or I create a PR that refactors your test_util.py in a way that the actual test cases move into our compiler unit test suite and the the test_util.py becomes only a thin shim that runs the test methods and makes sure we get the right exit code and the compiled output YAML with the similar parameters as dsl-compile does

[drewbutlerbb4]

Sure that works for me

[ckadner]

We decided to merge this PR and incorporate further changes in subsequent PRs

Thanks @drewbutlerbb4

/lgtm
/assign @animeshsingh

[animeshsingh]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: animeshsingh

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:

• OWNERS [animeshsingh]

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