SDK/Components - Reworked the component model structures. #642

Ark-kun · 2019-01-07T08:48:56Z

Rewrote parsing, type checking and serialization code.
Improved the graph component structures.
Added most of the needed k8s structures.
Added model validation (input/output existence etc).
Added task cycle detection and topological sorting to GraphSpec.
All container component tests now work.
Added some graph component tests.

This change is

Ark-kun · 2019-01-07T08:49:45Z

/assign @qimingj @gaoning777

qimingj

Sending a few comments now. Will finish reviewing the rest later.

sdk/python/kfp/components/_structures.py

sdk/python/kfp/components/_components.py

sdk/python/kfp/components/_structures.py

Rewrote parsing, type checking and serialization code. Improved the graph component structures. Added most of the needed k8s structures. Added model validation (input/output existence etc). Added task cycle detection and topological sorting to GraphSpec. All container component tests now work. Added some graph component tests.

qimingj

@Ark-kun, I have a few high level questions and need your help to clarify.

Also, the change is fairly big. It includes some separable features such as K8s wrapper, ModelBase class, etc. It would be easier for reviewers if it is separated into multiple changes.

sdk/python/kfp/components/_structures.py

sdk/python/kfp/components/modelbase.py

sdk/python/kfp/components/structures/kubernetes/v1.py

sdk/python/kfp/components/_structures.py

Ark-kun · 2019-01-08T08:51:50Z

Also, the change is fairly big. It includes some separable features such as K8s wrapper, ModelBase class, etc. It would be easier for reviewers if it is separated into multiple changes.

ModelBase

I've refactored/extracted the boilerplate serialization/deserialization code into that base class. All object model structures depend on it, so it must be included.

K8s wrapper

The k8s structures are used by the Task class. I got lots of comments from our teams about the need for full kubernetes support, so it made sense to add those structures too. I could separate this one, but I did not want the tasks to lack volumeMount support that is essential for all our sample pipelines.

etc

There are also code changes to use the new structures and unit tests. Without those small code changes in component_builder the code will just be broken.

As for the unit tests, I consider them to be a positive addition and they also demonstrate the usage for the classes that were not properly used and tested before.

I might have omitted the validation code that fixed the previously skipped component tests though.

I'll try making the PRs smaller in future.

Ark-kun · 2019-01-09T00:53:56Z

/test kubeflow-pipeline-e2e-test

sdk/python/kfp/components/modelbase.py

qimingj · 2019-01-09T01:49:17Z

A few high level comments:

I am concerned about doing Python arguments type checking, serialization and deserialization. It feels like a low level Python work and by itself can be a big project. I am not sure we need to own those code in the long run. I think we might be able to leverage an existing libraries at least for part of the work:

Schema validation against serialized yaml/json instead of doing type checking in code.
Once type checking is moved out, it might be possible to simplify the serialization and deserialization and even use an existing library which supports custom serialization.

But it seems you have explored all options and nothing works to your satisfaction (e.g. existing serialization/deserialization doesn't work well with Union types).

I hope we can still reuse kubernetes package for the objects since it is a long list. But if not, given we already exposed parameters as Kubernetes objects in kubernetes python package, we need to continue supporting them.

Ark-kun · 2019-01-09T19:47:09Z

I am concerned about doing Python arguments type checking, serialization and deserialization. It feels like a low level Python work and by itself can be a big project.

Good thing I managed to make this big project a reality. Now we can benefit from a good serialization and deserialization library for Python which we control. We do not need to wait years for fixes to the existing libraries and we get all the features that we need. In future we can make this a separate OSS library.

Once type checking is moved out, it might be possible to simplify the serialization and deserialization

As I showed you offline, removing the few if ...: raise TypeError lines wont dramatically check a function that already fits a single screen. Serialization function won't shrink by a single line.

I hope we can still reuse kubernetes package for the objects since it is a long list.

Same thing. I hope that in future the kubernetes package becomes useful for this kind of job. I'm adding some fixes to the python-swagger code generator so maybe in some months there will be a slightly fixed kubernetes python package.

Ark-kun · 2019-01-09T22:12:39Z

Gentle ping.
I think I resolved all concerns about this data classes refactoring.

qimingj

/lgtm

qimingj · 2019-01-09T22:39:19Z

sdk/python/kfp/components/_structures.py


+class InputSpec(ModelBase):
+    '''Describes the component input specification'''
+    def __init__(self,


Add TODO for making it a data class in the future.

qimingj · 2019-01-09T22:42:03Z

sdk/python/kfp/components/_structures.py

+                    if output_name not in self._outputs_dict:
+                        raise TypeError('Unconfigurable output entry "{}" references non-existing output.'.format({output_name: path}))
+
+            def verify_arg(arg):


nit: functions inside a function and an if block is harder to read.

qimingj · 2019-01-09T23:00:13Z

sdk/python/tests/components/test_structure_model_base.py

+        obj41 = TestModel1.from_struct(struct41)
+        self.assertEqual(obj41.prop_4['val 4'], val4)
+        self.assertDictEqual(obj41.to_struct(), struct41)
+


nit: one space

Ark-kun · 2019-01-09T23:04:06Z

/approve

k8s-ci-robot · 2019-01-09T23:04:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

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~~ [Ark-kun]

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-01-09T23:04:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

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~~ [Ark-kun]

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

…ces (kubeflow#637)" (kubeflow#642) Partial revert of commit afd9f95. The change in the self-signed logic breaks presubmits with the following error E kubernetes.client.rest.ApiException: (409) E Reason: Conflict E HTTP response headers: HTTPHeaderDict({'Audit-Id': '85305103-054e-4795-a45e-f802de1ae4ea', 'Content-Type': 'application/json', 'Date': 'Wed, 15 Apr 2020 00:31:03 GMT', 'Content-Length': '352'}) E HTTP response body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Operation cannot be fulfilled on ingresses.extensions \"envoy-ingress\": the object has been modified; please apply your changes to the latest version and try again","reason":"Conflict","details":{"name":"envoy-ingress","group":"extensions","kind":"ingresses"},"code":409} * Only changes to util.py in the original PR are reverted.

k8s-ci-robot requested review from gaoning777 and qimingj January 7, 2019 08:48

k8s-ci-robot added the size/XXL label Jan 7, 2019

k8s-ci-robot assigned gaoning777 and qimingj Jan 7, 2019

Ark-kun force-pushed the SDK/Components---Refactored-component-model-structures branch from 478a22c to a23ad59 Compare January 7, 2019 08:52

qimingj reviewed Jan 8, 2019

View reviewed changes

sdk/python/kfp/components/_structures.py Show resolved Hide resolved

sdk/python/kfp/components/_components.py Outdated Show resolved Hide resolved

sdk/python/kfp/components/_structures.py Outdated Show resolved Hide resolved

Ark-kun and others added 6 commits January 7, 2019 18:49

Fixed incompatibilities with python <3.7

75156c3

Added __init__.py to make the Travis tests work.

f73ef23

Adding kubernetes structures to setup.py

4df3d49

Addressed PR feedback: Renamed _original_names to _serialized_names

43ce2c4

Addressed PR feedback: Reduced indentation.

d4c7ccb

Ark-kun force-pushed the SDK/Components---Refactored-component-model-structures branch from d622f60 to d4c7ccb Compare January 8, 2019 02:55

Ark-kun mentioned this pull request Jan 8, 2019

go vet in Travis tests breaks tests for unrelated PRs #646

Closed

Added descriptions for all component structures.

5913c34

qimingj reviewed Jan 8, 2019

View reviewed changes

Ark-kun and others added 2 commits January 8, 2019 00:55

Fixed a bug in ComponentSpec._post_init()

51d5e5f

Added documentation for ModelBase class and functions.

d86ec4f

qimingj reviewed Jan 9, 2019

View reviewed changes

sdk/python/kfp/components/modelbase.py Outdated Show resolved Hide resolved

sdk/python/kfp/components/modelbase.py Show resolved Hide resolved

sdk/python/kfp/components/modelbase.py Outdated Show resolved Hide resolved

sdk/python/kfp/components/modelbase.py Show resolved Hide resolved

Ark-kun added 2 commits January 9, 2019 11:36

Added __eq__/__ne__ and improved __repr__

1c2f6d1

Added ModelBase tests

1b32448

qimingj approved these changes Jan 9, 2019

View reviewed changes

k8s-ci-robot added lgtm labels Jan 9, 2019

k8s-ci-robot added approved labels Jan 9, 2019

k8s-ci-robot merged commit 83e9ffe into kubeflow:master Jan 9, 2019

Ark-kun deleted the SDK/Components---Refactored-component-model-structures branch January 21, 2019 11:16

HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024

Support custom image for any Seq (kubeflow#642)

da87237

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDK/Components - Reworked the component model structures. #642

SDK/Components - Reworked the component model structures. #642

Ark-kun commented Jan 7, 2019 •

edited

Loading

Ark-kun commented Jan 7, 2019

qimingj left a comment

qimingj left a comment

Ark-kun commented Jan 8, 2019 •

edited

Loading

Ark-kun commented Jan 9, 2019

qimingj commented Jan 9, 2019

Ark-kun commented Jan 9, 2019 •

edited

Loading

Ark-kun commented Jan 9, 2019

qimingj left a comment

qimingj Jan 9, 2019

qimingj Jan 9, 2019

qimingj Jan 9, 2019

Ark-kun commented Jan 9, 2019

k8s-ci-robot commented Jan 9, 2019

k8s-ci-robot commented Jan 9, 2019

SDK/Components - Reworked the component model structures. #642

SDK/Components - Reworked the component model structures. #642

Conversation

Ark-kun commented Jan 7, 2019 • edited Loading

Ark-kun commented Jan 7, 2019

qimingj left a comment

Choose a reason for hiding this comment

qimingj left a comment

Choose a reason for hiding this comment

Ark-kun commented Jan 8, 2019 • edited Loading

Ark-kun commented Jan 9, 2019

qimingj commented Jan 9, 2019

Ark-kun commented Jan 9, 2019 • edited Loading

Ark-kun commented Jan 9, 2019

qimingj left a comment

Choose a reason for hiding this comment

qimingj Jan 9, 2019

Choose a reason for hiding this comment

qimingj Jan 9, 2019

Choose a reason for hiding this comment

qimingj Jan 9, 2019

Choose a reason for hiding this comment

Ark-kun commented Jan 9, 2019

k8s-ci-robot commented Jan 9, 2019

k8s-ci-robot commented Jan 9, 2019

Ark-kun commented Jan 7, 2019 •

edited

Loading

Ark-kun commented Jan 8, 2019 •

edited

Loading

Ark-kun commented Jan 9, 2019 •

edited

Loading