Add argo variable checking when compile to Tekton. #80

Tomcli · 2020-04-02T21:43:22Z

Which issue is resolved by this Pull Request:
Related #78

Description of your changes:
Currently this is the equivalent variable matching i found.
argo -> tekton
{{inputs.parameters.%s}} -> $(inputs.params.%s)
{{outputs.parameters.%s}} -> $(results.%s.path)

Since we don't have other equivalent matching for the rest of the argo variables, we will prompt a warning when those argo variables are used. KFP also don't recommend to use argo variables because it requires specific knowledge on Argo pipeline.

Out of all the kfp examples, only a very few of them are using Argo variables. e.g. https://github.com/kubeflow/pipelines/blob/master/samples/contrib/nvidia-resnet/pipeline/src/pipeline.py
In this case we will prompt the following warning:

Applying KFP-Tekton compiler patch
KFP-Tekton Compiler 0.0.1
WARNING:root:These Argo variables are not supported in Tekton Pipeline: {{workflow.name}}

The other places that use Argo variables are during artifact storing and pipeline placeholder. We could create our own tasks to avoid using Argo specific variables on these features as mentioned in #78.

Environment tested:

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

kubeflow-bot · 2020-04-02T21:43:29Z

This change is

ckadner · 2020-04-03T00:57:14Z

sdk/python/kfp_tekton/compiler/compiler.py

+
+    unsupported_vars = re.findall(r"{{[^ \t\n:,;{}]+}}", workflow_dump)
+    if unsupported_vars:
+      logging.warning('These Argo variables are not supported in Tekton Pipeline: %s' % ", ".join(str(v) for v in set(unsupported_vars)))


I am assuming that if one of the Argo specific variables is not supported, then the resulting Tekton pipeline does not behave in the way the author of the Python DSL script intended it to behave. In which case I think should be a little stronger here and raise TypeError or raise ValueError or even raise AttributeError. No?

Let me see is there other places that rely on this. If not then I can change it to raise and exit.

I changed to raise error now.

sdk/python/kfp_tekton/compiler/compiler.py

ckadner · 2020-04-03T01:01:59Z

Hi @Tomcli,
do you have a list of all the valriable expressions which cannot be translated to Tekton?

And if/when you do, are there issues tracking whether those might be in the works by the Tekton team?

Tomcli · 2020-04-03T01:07:53Z

The list is basically this whole page minus the two I implemented above.
https://github.com/argoproj/argo/blob/master/docs/variables.md

A lot of these variables might not even make sense in Tekton, and most kfp pipelines try to avoid using variables that only Argo could understand.

Tomcli · 2020-04-03T01:10:22Z

Some variables such as task name is already been requested in issue such as tektoncd/pipeline#2294. We can also open another issue on Tekton for this to track what variables can we support.

Tomcli · 2020-04-03T01:21:03Z

I opened a related issue at Tekton tektoncd/pipeline#2322

ckadner

great, just a tiny update on the comment wording "prompt warning" vs "raise error"

ckadner · 2020-04-03T20:34:00Z

sdk/python/kfp_tekton/compiler/compiler.py

-    # Use regex to replace all the Argo variables to Tekton variables.
-    # For variables that are unique to Argo, we prompt a warning to warn users on the non-supported variables.
+    # Use regex to replace all the Argo variables to Tekton variables. For variables that are unique to Argo,
+    # we prompt a warning to warn users on the non-supported variables. Here is the list of Argo variables.


"# we raise an Error to alert users about the unsupported" ... :-)

thanks christian.

ckadner · 2020-04-03T20:34:13Z

sdk/python/kfp_tekton/compiler/compiler.py

@@ -209,7 +209,7 @@ def _create_pipeline_workflow(self, args, pipeline, op_transformers=None, pipeli

    unsupported_vars = re.findall(r"{{[^ \t\n:,;{}]+}}", workflow_dump)
    if unsupported_vars:
-      logging.warning('These Argo variables are not supported in Tekton Pipeline: %s' % ", ".join(str(v) for v in set(unsupported_vars)))
+      raise ValueError('These Argo variables are not supported in Tekton Pipeline: %s' % ", ".join(str(v) for v in set(unsupported_vars)))


ckadner

Great. Thanks @Tomcli !

/lgtm
/assign @animeshsingh

animeshsingh · 2020-04-03T22:28:28Z

/approve

k8s-ci-robot · 2020-04-03T22:28:35Z

[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

Add iris pipeline example

Tomcli added 4 commits April 2, 2020 09:37

add initial parsing

88d5801

Merge remote-tracking branch 'upstream/master' into vars

c873005

add regex mapping for the equivalent tekton variables

dd319ac

revert old changes

a653e2d

k8s-ci-robot requested review from fenglixa and jinchihe April 2, 2020 21:43

k8s-ci-robot added the size/L label Apr 2, 2020

Tomcli force-pushed the vars branch from 3bb8f3f to 5152179 Compare April 2, 2020 21:44

fix typos

029f169

Tomcli force-pushed the vars branch from 5152179 to 029f169 Compare April 2, 2020 21:45

ckadner requested changes Apr 3, 2020

View reviewed changes

ckadner mentioned this pull request Apr 3, 2020

Compiler work items #18

Closed

27 tasks

raise error when unsupported Argo vars are used

fcf1ab1

ckadner requested changes Apr 3, 2020

View reviewed changes

fix comment

d6368e7

ckadner approved these changes Apr 3, 2020

View reviewed changes

k8s-ci-robot assigned animeshsingh and ckadner Apr 3, 2020

k8s-ci-robot added the lgtm label Apr 3, 2020

ckadner removed their assignment Apr 3, 2020

k8s-ci-robot added the approved label Apr 3, 2020

k8s-ci-robot merged commit 3328818 into kubeflow:master Apr 3, 2020

Tomcli deleted the vars branch May 20, 2020 17:20

gmfrasca pushed a commit to gmfrasca/data-science-pipelines-tekton that referenced this pull request May 16, 2023

Merge pull request kubeflow#80 from strangiato/iris-example

72fc9bb

Add iris pipeline example

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add argo variable checking when compile to Tekton. #80

Add argo variable checking when compile to Tekton. #80

Tomcli commented Apr 2, 2020

kubeflow-bot commented Apr 2, 2020

ckadner Apr 3, 2020

Tomcli Apr 3, 2020

Tomcli Apr 3, 2020

ckadner commented Apr 3, 2020

Tomcli commented Apr 3, 2020 •

edited

Loading

Tomcli commented Apr 3, 2020 •

edited

Loading

Tomcli commented Apr 3, 2020

ckadner left a comment

ckadner Apr 3, 2020

Tomcli Apr 3, 2020

ckadner Apr 3, 2020

ckadner left a comment

animeshsingh commented Apr 3, 2020

k8s-ci-robot commented Apr 3, 2020

Add argo variable checking when compile to Tekton. #80

Add argo variable checking when compile to Tekton. #80

Conversation

Tomcli commented Apr 2, 2020

kubeflow-bot commented Apr 2, 2020

ckadner Apr 3, 2020

Choose a reason for hiding this comment

Tomcli Apr 3, 2020

Choose a reason for hiding this comment

Tomcli Apr 3, 2020

Choose a reason for hiding this comment

ckadner commented Apr 3, 2020

Tomcli commented Apr 3, 2020 • edited Loading

Tomcli commented Apr 3, 2020 • edited Loading

Tomcli commented Apr 3, 2020

ckadner left a comment

Choose a reason for hiding this comment

ckadner Apr 3, 2020

Choose a reason for hiding this comment

Tomcli Apr 3, 2020

Choose a reason for hiding this comment

ckadner Apr 3, 2020

Choose a reason for hiding this comment

ckadner left a comment

Choose a reason for hiding this comment

animeshsingh commented Apr 3, 2020

k8s-ci-robot commented Apr 3, 2020

Tomcli commented Apr 3, 2020 •

edited

Loading

Tomcli commented Apr 3, 2020 •

edited

Loading