Feature: sidecar for ContainerOp #879

eterna2 · 2019-02-28T07:22:36Z

Motivation

I am using kubeflow pipelines to run simulation experiments, and because of the complexity of the simulation setup, it requires supporting containers which I might want to parameterize.

Status

Waiting for Review

all unit tests are working
updated to work with latest master HEAD
TODO?: more comprehensive test on generating complex template with PipelineParams everywhere.

New or updated Features

This PR adds the following features to kfp package's dsl and compiler modules:

allows PipelineParam for all valid attributes/properties (both string serialized and object instance) in dsl.ContainerOp
allows sidecars for workflow template (via dsl.ContainerOp.add_sidecar) with dsl.Sidecar class
allows parameterization of container property for argo workflow template (via dsl.ContainerOp.container)
(i.e. you can set any valid attributes/properties for a k8s V1Container object)

NOTE .
All changes are completely backward compatible. With appropriate pending deprecation warnings.

Example

from kfp import dsl
from kubernetes.client.models import V1EnvVar

@dsl.pipeline(
    name='foo',
    description='hello world')
def foo_pipeline(tag: str, pull_image_policy: str):

    # any attributes can be parameterized (both serialized string or actual PipelineParam)
    op = dsl.ContainerOp(name='foo', 
                                         image='busybox:' % tag,
                                         # pass in sidecars list
                                         sidecars=[dsl.Sidecar('print', 'busybox:latest', command='echo "hello"')],
                                         # pass in k8s container kwargs
                                         container_kwargs={'env': [V1EnvVar('foo', 'bar')]})

    # set `imagePullPolicy` property for `container` with `PipelineParam` 
    op.container.set_pull_image_policy(pull_image_policy)

    # add sidecar with parameterized image tag
    # sidecar follows the argo sidecar swagger spec
    op.add_sidecar(dsl.Sidecar('redis', 'redis:' % tag).set_image_pull_policy('Always'))

TLDR changes

This PR will make the following changes:

Created Container and Sidecar classes
which inherits from V1Container
- updated both classes with methods to update k8s properties
Updated ContainerOp
- added kwargs: container_kwargs and sidecars
- added attr _container (and getter container) which holds the Container instance
- added pendingDeprecationWarning for container properties and methods
- changed inputs to scan through all qualified attributes in ContainerOp to get all PipelineParam
Updated PipelineParam
- added pattern attr to hold regex pattern extracted from serialized string
- added method to recursively extract PipelineParam from any object
ContainerOp to template
- moved _op_to_template to its own module
- added method to recursively replace PipelineParam (and its serialized form) with appropriate input.parameters.%s
Added appropriate unit test for updates

This change is

k8s-ci-robot · 2019-02-28T07:22:49Z

Hi @eterna2. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2019-02-28T07:22:49Z

Hi @eterna2. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sdk/python/kfp/compiler/compiler.py

animeshsingh

I am trying to understand the use case more. The idea here is that in a single step, we can launch multiple sidecar containers - synchronously or asynchronously? Why not kick off instances another pipeline from a step, if the step requires replicated efforts?

eterna2 · 2019-03-01T00:51:19Z

@animeshsingh

Asynchronously. The sidecars are services rather than jobs (i.e. mq brokers, db, rest services, ...)

I am running a simulation job that is reliant on a few other services. If I provision these services as parallel steps, I would need to add additional scripts to send a termination signal to these services when the simulation job completes.

sdk/python/kfp/dsl/_container_op.py

sdk/python/kfp/compiler/compiler.py

ukclivecox · 2019-03-05T06:53:50Z

Is it worth adding an example or test to illustrate how a sidecar can be added?

eterna2 · 2019-03-05T14:53:08Z

@cliveseldon
Sure. I will add an @example as part of the pydoc comment for ContainerOp.

animeshsingh

Should we have a test example to test the functionality?

sdk/python/kfp/dsl/__init__.py

animeshsingh · 2019-03-08T03:24:27Z

sdk/python/kfp/dsl/_container_op.py

+
+
+class ContainerOp(ContainerBase):
+  """Represents an op implemented by a docker container image."""


Given docker is being phased out, may make sense to just call these 'container image'

Given docker is being phased out, may make sense to just call these 'container image'

Can you please provide some link documenting that?

well @Ark-kun - its phased out from IBM`s official Kubernetes service
https://www.ibm.com/blogs/bluemix/2019/01/ibm-cloud-kubernetes-service-supports-containerd/

Other providers are also discussing moving away - btw GCP lauched this in beta in Nov as well
https://cloud.google.com/blog/products/containers-kubernetes/containerd-available-for-beta-testing-in-google-kubernetes-engine

texasmichelle · 2019-03-10T20:43:18Z

👍 to adding sidecar support. Thank you everyone for working on this!

… well as sidecars with Sidecar class. ContainerOp accepts PipelineParam in any valid k8 properties.

hongye-sun · 2019-03-27T18:27:12Z

/lgtm
/approve

Travis has an outage right now. Since the e2e is passing, I will give the approve label and feel free to merge after travis build is passed.

k8s-ci-robot · 2019-03-27T18:27:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongye-sun

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:

~~sdk/OWNERS~~ [hongye-sun]

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 · 2019-03-27T18:27:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongye-sun

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:

~~sdk/OWNERS~~ [hongye-sun]

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

hongye-sun · 2019-03-27T18:45:59Z

/test kubeflow-pipeline-sample-test

gaoning777 · 2019-03-27T21:51:29Z

sdk/python/kfp/dsl/_container_op.py

+    def _validate_cpu_string(self, cpu_string):
+        "Validate a given string is valid for cpu request or limit."
+
+        if isinstance(cpu_string, _pipeline_param.PipelineParam):


Could we extract this block of code as a separate internal function because it has been used by many functions here?

gaoning777 · 2019-03-27T21:55:47Z

sdk/python/kfp/dsl/_container_op.py

+    return value if isinstance(value, list) else [value]
+
+
+def create_and_append(current_list: Union[List[T], None], item: T) -> List[T]:


If this is internally used in this module, could we rename it such that it starts with an underscore?

gaoning777 · 2019-03-27T22:06:16Z

sdk/python/kfp/dsl/_container_op.py

+        self.image_pull_policy = image_pull_policy
+        return self
+
+    def add_port(self, container_port):


Maybe add a TODO to support pipelineparam for the other configurations besides CPU, memory ,etc.

It is already supported - in the compiler._op_to_template there is a recursive method to replace PipelineParam and its serialized form into argo variables.

gaoning777 · 2019-03-27T22:17:41Z

sdk/python/kfp/dsl/_container_op.py


-    if re.match(r'^[0-9]+m$', cpu_string) is not None:
-      return
+# util functions


Let's move the general utility functions into a separate module.

This is more container_op related but I think it would be helpful to define a decorator that helps for the future deprecation warnings.

hongye-sun · 2019-03-28T00:14:30Z

/test kubeflow-pipeline-sample-test

hongye-sun · 2019-03-28T16:48:27Z

/test kubeflow-pipeline-sample-test

hongye-sun · 2019-03-28T17:24:32Z

/hold cancel

hongye-sun · 2019-03-28T22:14:37Z

@eterna2

Thanks a lot for contributing to this PR. It helps to unlock many our use cases (not only just sidecar) for k8s resource orchestrations.

vicaire · 2019-03-29T01:45:31Z

+1. Thank you @eterna2

Restoring the kubeflow#900 change that was overwritten by kubeflow#879

Restoring the #900 change that was overwritten by #879

Migrate xgboost-operator repo to new test-infra

k8s-ci-robot requested review from Ark-kun, gaoning777 and hongye-sun February 28, 2019 07:22

k8s-ci-robot added size/L labels Feb 28, 2019

k8s-ci-robot added the needs-ok-to-test label Feb 28, 2019

Ark-kun reviewed Mar 1, 2019

View reviewed changes

sdk/python/kfp/compiler/compiler.py Outdated Show resolved Hide resolved

animeshsingh reviewed Mar 1, 2019

View reviewed changes

hongye-sun reviewed Mar 4, 2019

View reviewed changes

sdk/python/kfp/dsl/_container_op.py Outdated Show resolved Hide resolved

sdk/python/kfp/dsl/_container_op.py Outdated Show resolved Hide resolved

sdk/python/kfp/dsl/_container_op.py Outdated Show resolved Hide resolved

sdk/python/kfp/compiler/compiler.py Outdated Show resolved Hide resolved

hongye-sun self-assigned this Mar 5, 2019

hongye-sun mentioned this pull request Mar 5, 2019

Add Support for Docker-in-Docker in DSL (See https://github.com/argoproj/argo/tree/master/examples#docker-in-docker-using-sidecars) #759

Closed

animeshsingh reviewed Mar 8, 2019

View reviewed changes

k8s-ci-robot added size/XXL and removed size/L labels Mar 8, 2019

eterna2 added 2 commits March 8, 2019 20:50

Feature: sidecar for ContainerOp

33ed2a5

replace f-string with string format for compatibility with py3.5

c99a46d

texasmichelle mentioned this pull request Mar 10, 2019

Add SDK support for sidecars #949

Closed

eterna2 force-pushed the eterna2/kfp-containerOps-sidecar branch from 256e846 to 1f97e2f Compare March 13, 2019 11:38

ContainerOp now can be updated with any k8s V1Container attributes as…

383a4f5

… well as sidecars with Sidecar class. ContainerOp accepts PipelineParam in any valid k8 properties.

eterna2 force-pushed the eterna2/kfp-containerOps-sidecar branch from 1f97e2f to 383a4f5 Compare March 13, 2019 11:43

Merge branch 'master' into eterna2/kfp-containerOps-sidecar

472a9c5

k8s-ci-robot added the lgtm label Mar 27, 2019

k8s-ci-robot added the approved label Mar 27, 2019

gaoning777 reviewed Mar 27, 2019

View reviewed changes

hongye-sun mentioned this pull request Mar 28, 2019

Extend the DSL to implement the design of #801 #926

Merged

k8s-ci-robot removed do-not-merge/hold labels Mar 28, 2019

hongye-sun merged commit 825f64d into kubeflow:master Mar 28, 2019

elikatsis mentioned this pull request Mar 28, 2019

Extend the DSL with support for Persistent Volumes and Snapshots #801

Closed

gaoning777 mentioned this pull request Mar 29, 2019

Add image substitution feature to either DSL compiler or KFP client #487

Closed

Ark-kun added a commit to Ark-kun/pipelines that referenced this pull request Mar 29, 2019

SDK - Configure artifact name and path separately (again)

336b68c

Restoring the kubeflow#900 change that was overwritten by kubeflow#879

Ark-kun mentioned this pull request Mar 29, 2019

SDK - Configure artifact name and path separately (again) #1067

Merged

Ark-kun added a commit that referenced this pull request Apr 4, 2019

SDK - Configure artifact name and path separately (again) (#1067)

9269bac

Restoring the #900 change that was overwritten by #879

elikatsis mentioned this pull request May 9, 2019

The volumes PVC name is unreasonable after mount a pvc for pipeline #1303

Closed

elikatsis mentioned this pull request Sep 4, 2019

Support K8 resource YAML config as input for DSL components #756

Closed

eterna2 mentioned this pull request Sep 10, 2019

Adds eterna2 to kubeflow org kubeflow/internal-acls#153

Closed

Ark-kun mentioned this pull request Jul 14, 2020

fix(sdk): SDK - Avoiding deprecated ContainerOp methods #4134

Merged

2 tasks

Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023

Migrate xgboost-operator repo (kubeflow#879)

bd372c4

Migrate xgboost-operator repo to new test-infra

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023

Fix helm istio install in CI (kubeflow#879)

bda2209

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: sidecar for ContainerOp #879

Feature: sidecar for ContainerOp #879

eterna2 commented Feb 28, 2019 •

edited

Loading

k8s-ci-robot commented Feb 28, 2019

k8s-ci-robot commented Feb 28, 2019

animeshsingh left a comment

eterna2 commented Mar 1, 2019

ukclivecox commented Mar 5, 2019

eterna2 commented Mar 5, 2019

animeshsingh left a comment

animeshsingh Mar 8, 2019

Ark-kun Mar 13, 2019

animeshsingh Mar 29, 2019

texasmichelle commented Mar 10, 2019

hongye-sun commented Mar 27, 2019

k8s-ci-robot commented Mar 27, 2019

k8s-ci-robot commented Mar 27, 2019

hongye-sun commented Mar 27, 2019

gaoning777 Mar 27, 2019

gaoning777 Mar 27, 2019

gaoning777 Mar 27, 2019

eterna2 Mar 28, 2019

gaoning777 Mar 27, 2019

gaoning777 Mar 27, 2019

hongye-sun commented Mar 28, 2019

hongye-sun commented Mar 28, 2019

hongye-sun commented Mar 28, 2019

hongye-sun commented Mar 28, 2019

vicaire commented Mar 29, 2019



		class ContainerOp(ContainerBase):
		"""Represents an op implemented by a docker container image."""

		return value if isinstance(value, list) else [value]


		def create_and_append(current_list: Union[List[T], None], item: T) -> List[T]:

Feature: sidecar for ContainerOp #879

Feature: sidecar for ContainerOp #879

Conversation

eterna2 commented Feb 28, 2019 • edited Loading

Motivation

Status

New or updated Features

TLDR changes

k8s-ci-robot commented Feb 28, 2019

k8s-ci-robot commented Feb 28, 2019

animeshsingh left a comment

Choose a reason for hiding this comment

eterna2 commented Mar 1, 2019

ukclivecox commented Mar 5, 2019

eterna2 commented Mar 5, 2019

animeshsingh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

texasmichelle commented Mar 10, 2019

hongye-sun commented Mar 27, 2019

k8s-ci-robot commented Mar 27, 2019

k8s-ci-robot commented Mar 27, 2019

hongye-sun commented Mar 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongye-sun commented Mar 28, 2019

hongye-sun commented Mar 28, 2019

hongye-sun commented Mar 28, 2019

hongye-sun commented Mar 28, 2019

vicaire commented Mar 29, 2019

eterna2 commented Feb 28, 2019 •

edited

Loading