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

Add initial pipelinerun support #62

Merged
merged 3 commits into from
Mar 26, 2020
Merged

Conversation

Tomcli
Copy link
Member

@Tomcli Tomcli commented Mar 25, 2020

Add initial pipelinerun support for development purpose. This PR didn't add any breaking change to the existing compiler. Features such as pipeline timeout and workspaces will come in the following PR since those are breaking features that might require monkey patches on the pipeline dsl definition.

Example usage:

dsl-compile-tekton --py sdk/python/tests/compiler/testdata/parallel_join.py --output pipeline.yaml --generate-pipelinerun

or in Python

TektonCompiler().compile(download_and_join, __file__.replace('.py', '.yaml'), generate_pipelinerun=True)

I will also extend it to the developer guide once it's ready.

@kubeflow-bot
Copy link

This change is Reviewable

@Tomcli
Copy link
Member Author

Tomcli commented Mar 25, 2020

/assign @ckadner

@ckadner
Copy link
Member

ckadner commented Mar 25, 2020

Thanks @Tomcli, I will take a look.

@ckadner
Copy link
Member

ckadner commented Mar 26, 2020

Hi @Tomcli,

sorry for the delay. I am still thinking about this. I don't like dragging in ever more of the KFP compiler code, only to accommodate small bits of code changes. But due to the sub-optimal code structure small changes sometimes come with a long tail.

Besides that we have an unfortunate mix of inheritance with overriding classes and class methods, but we rely on monkey-patching to dynamically replace static methods. So we need to ensure all possible entry points to the compiler, like main, CLI, unit tests, direct instantiations all trigger the monkey-patch but not when the actual KFP functions are desired (i.e. using dsl-compile on a Python script that has a __main__ using the TektonCompiler).

And conceptually I prefer generating Pipelines over PipelineRuns, so maybe I just have to get over myself and become a little less opinionated 😉

I will take a little break and get back to this with fresh eyes a little later.

Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Okay, let's go ahead with this :-)

...after a few minor style changes.

Thanks @Tomcli

@@ -46,6 +46,18 @@ def my_pipeline(a: int = 1, b: str = "default value"):
```
"""

def compile(self, pipeline_func, package_path, type_check=True, pipeline_conf: dsl.PipelineConf = None, generate_pipelinerun=False):
Copy link
Member

Choose a reason for hiding this comment

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

can you move the compile function down to just above _write_workflow so that when we compare code changes with KFP all the methods line up

and break the line before the generate_pipelinerun argument to keep the line length below 120 for code style compliance and better readability

Copy link
Member Author

@Tomcli Tomcli Mar 26, 2020

Choose a reason for hiding this comment

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

Thanks @ckadner, I moved the compile function.

Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Thanks @Tomcli

@ckadner
Copy link
Member

ckadner commented Mar 26, 2020

/lgtm
/assign @animeshsingh

@animeshsingh
Copy link
Collaborator

/approve

@k8s-ci-robot
Copy link

[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:

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

@k8s-ci-robot k8s-ci-robot merged commit 5b52b79 into kubeflow:master Mar 26, 2020
@ckadner ckadner mentioned this pull request Mar 26, 2020
27 tasks
@Tomcli Tomcli deleted the pipelinerun branch May 6, 2021 22:59
gmfrasca pushed a commit to gmfrasca/data-science-pipelines-tekton that referenced this pull request Feb 10, 2023
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.

None yet

5 participants