Add volume, volumemount and env to container op #300

IronPan · 2018-11-16T21:36:59Z

Add these fields to support mounting secret to the container. It replace gcp secret which is to specific to gcp.

I will add a gcp op instead as a syntax sugar.

This change is

IronPan · 2018-11-16T21:40:45Z

gaoning777 · 2018-11-16T21:47:29Z

sdk/python/kfp/compiler/compiler.py

@@ -140,23 +143,21 @@ def _op_to_template(self, op):
    if op.memory_request or op.cpu_request:
      template['container']['resources']['requests'] = {}
      if op.memory_request:
-        template['container']['resources']['requests']['memory'] = op.memory_request
+        template['container']['resources']['requests'][


Minor issue: It would be best if these lines are cut in the more natural places, for example after ['memory']?

ack. thanks

Ark-kun · 2018-11-16T22:19:47Z

sdk/python/kfp/compiler/compiler.py

  def _get_uncommon_ancestors(self, op_groups, op1, op2):
    """Helper function to get unique ancestors between two ops.

    For example, op1's ancestor groups are [root, G1, G2, G3, op1], op2's ancestor groups are
    [root, G1, G4, op2], then it returns a tuple ([G2, G3, op1], [G4, op2]).
    """
    both_groups = [op_groups[op1.name], op_groups[op2.name]]
-    common_groups_len = sum(1 for x in zip(*both_groups) if x==(x[0],)*len(x))
+    common_groups_len = sum(


Can you please undo the auto-format changes so that the diff is smaller and changes are more clear.

ack. thanks

qimingj · 2018-11-16T23:35:33Z

sdk/python/kfp/compiler/compiler.py

+      env_variables = []
+      for e in op.env_variables:
+        env_variables.append(self._convert_k8s_obj_to_dic(e))
+      template['container']['env'] = env_variables


can be done in one line (didn't try it so syntax might be off):

template['container']['env'] = map(self._convert_k8s_obj_to_dic, op.env_variables)

Same for volume.

done. it's much cleaner.

qimingj · 2018-11-16T23:39:29Z

sdk/python/kfp/compiler/compiler.py

+    If obj is dict, return the dict.
+    If obj is swagger model, return the properties dict.
+
+    :param obj: The data to serialize.


":param" and ":return": are these new Python style docstring? In Python it is usually:

Args:
arg1: ...
arg2: ...
Returns:
...

sdk/python/kfp/compiler/compiler.py

qimingj · 2018-11-16T23:55:51Z

sdk/python/kfp/dsl/_container_op.py

@@ -154,5 +165,20 @@ def set_cpu_limit(self, cpu):
    self._validate_cpu_string(cpu)
    self.cpu_limit = cpu

+  def set_volumes(self, volumes):
+    """Specifying what K8s volumes the container depends on"""


Add an Args section in docstring? For example:

Args:
volumes: a list of k8s volumes. For example, ...

At least users need to know what type of data to pass.

qimingj · 2018-11-17T00:04:43Z

sdk/python/tests/compiler/compiler_tests.py

-              'name': 'mlpipeline-minio-artifact',
+                           arguments=['echo %s %s | tee /tmp/message.txt' % (
+                             msg1, msg2)],
+                           file_outputs={'merged': '/tmp/message.txt'})


do you also need to cover op.set_volumes?

the volume is not specified at container level, so this test won't test it. I have a volume.py test that covers it.

qimingj · 2018-11-17T00:25:33Z

Thanks @IronPan. I added a few comments.

Ark-kun · 2018-11-17T00:31:09Z

/lgtm

qimingj

Thanks!

qimingj · 2018-11-17T01:57:30Z

sdk/python/kfp/dsl/_container_op.py

+
+    Args:
+      volume_mounts: a list of Kubernetes volume mounts
+      For detailed spec, check


It's better. It would be clearer if you also specify what type of Python object is expected. For example, does it only accept "k8s_client.V1VolumeMount"? If so, make it explicit. If you were user of this API and knows little about K8s volume mounts, you probably need more help.

cool i add the link to the definition file.

This would require loading the Kubernetes module for every usage of the API. Do you think that a string annotation might be sufficient?
E.g.

def set_volume_mounts(self, volume_mounts: 'List[kubernetes.client.models.V1VolumeMount]'):

Ark-kun · 2018-11-17T02:06:27Z

/lgtm

IronPan · 2018-11-17T02:31:13Z

/approve

k8s-ci-robot · 2018-11-17T02:31:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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~~ [IronPan]

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 · 2018-11-17T02:31:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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~~ [IronPan]

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

…low#300) * Create a cron job to regularly garbage collect test resources. * Add to cleanup_ci.py an "all" subcommand to delete all resources." * Add a batch job for one off runs. Related to: kubeflow#87 Cron job to garbage collect test resources kubeflow#249 cron job to collect Kubeflow deployments launched by E2E tests * * Add a cron job to run the cleanup every two hours. * In cleanup_ci.py; don't load the imports of the manifests We encountered an error where the manifest didn't exist. I think that may have been a collision because we had a separate script running to do the deletes. * Fix some bugs. * Deal with config being none. * Maybe activate service account.

* v1alpha2 spec for predict,explain,transform * Fix tests * Change endpoints to nouns * Fix default tests * AlibiExplainSpec to AlibiExplainerSpec * Update codegen

IronPan and others added 15 commits November 13, 2018 17:43

[WIP] change deployment platform to gcp

2dd451f

debug

d7d35bf

Merge branch 'master' of https://github.com/kubeflow/pipelines

f33cb44

revert test

38b6cdc

Merge branch 'master' of https://github.com/kubeflow/pipelines

ead9828

add volume

cfdaa36

update test

9785fda

to list

e48143c

fix

2310f21

to list

3053dcc

to list

acc5eff

to list

1beddd3

to list

adb9a74

stage

1035814

update

1bc9543

k8s-ci-robot requested review from gaoning777, qimingj and Ark-kun November 16, 2018 21:37

k8s-ci-robot added size/L labels Nov 16, 2018

update

bc77782

k8s-ci-robot assigned Ark-kun, gaoning777 and qimingj Nov 16, 2018

gaoning777 reviewed Nov 16, 2018

View reviewed changes

Ark-kun reviewed Nov 16, 2018

View reviewed changes

qimingj reviewed Nov 17, 2018

View reviewed changes

Undid style changes

15fcbac

k8s-ci-robot added lgtm labels Nov 17, 2018

address comments

2e5cd7a

k8s-ci-robot removed lgtm labels Nov 17, 2018

qimingj reviewed Nov 17, 2018

View reviewed changes

update comments

6520f3d

k8s-ci-robot added lgtm labels Nov 17, 2018

k8s-ci-robot added the approved label Nov 17, 2018

k8s-ci-robot merged commit a47eb10 into master Nov 17, 2018

IronPan mentioned this pull request Nov 26, 2018

Authentication and service account plan for Pipeline + Kubeflow #374

Closed

Ark-kun deleted the yangpa/container branch November 28, 2018 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add volume, volumemount and env to container op #300

Add volume, volumemount and env to container op #300

IronPan commented Nov 16, 2018 •

edited

Loading

IronPan commented Nov 16, 2018

gaoning777 Nov 16, 2018

IronPan Nov 17, 2018

Ark-kun Nov 16, 2018

gaoning777 Nov 16, 2018

IronPan Nov 17, 2018

qimingj Nov 16, 2018

qimingj Nov 16, 2018

IronPan Nov 17, 2018

qimingj Nov 16, 2018

IronPan Nov 17, 2018

qimingj Nov 16, 2018

IronPan Nov 17, 2018

qimingj Nov 17, 2018

IronPan Nov 17, 2018

qimingj commented Nov 17, 2018

Ark-kun commented Nov 17, 2018

qimingj left a comment

qimingj Nov 17, 2018

IronPan Nov 17, 2018

Ark-kun Nov 17, 2018

Ark-kun commented Nov 17, 2018

IronPan commented Nov 17, 2018

k8s-ci-robot commented Nov 17, 2018

k8s-ci-robot commented Nov 17, 2018

Add volume, volumemount and env to container op #300

Add volume, volumemount and env to container op #300

Conversation

IronPan commented Nov 16, 2018 • edited Loading

IronPan commented Nov 16, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qimingj commented Nov 17, 2018

Ark-kun commented Nov 17, 2018

qimingj 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

Ark-kun commented Nov 17, 2018

IronPan commented Nov 17, 2018

k8s-ci-robot commented Nov 17, 2018

k8s-ci-robot commented Nov 17, 2018

IronPan commented Nov 16, 2018 •

edited

Loading