Migrate to KfpV2 #477

GeorgesLorre · 2023-09-29T09:19:56Z

No description provided.

adresses: #417

PR that adds option to set hardware accelerators to KFP V2 (nodepool Notes: * Each component was previously configured as a sub-pipeline because of an error in detecting a certain field in the V2 spec (`exec-<sanitizied_component_name`) which forces to kubeflow to compile component to a pipeline spec instead of a component spec [link](https://github.com/kubeflow/pipelines/blob/ad058b5321f5fe74bbd10845778e6627fb9aa5cb/sdk/python/kfp/dsl/structures.py#L462). This was resolved by adding the same custom method used by KFP to format the component name to Fondant. This makes it possible to assign resources to each component since KFP does not allow assigning resources on a pipeline level * Setting up and running nodepools is still to be tested on native KFP

PR that fixes the v2 default /optional types. KFP v2 has two different optional types: - Optional values that default to None -> defined by setting `isOptional` to True and not passing the default value - Optional values that default to another values -> defined by setting the value in `defaultValue` and having `isOptional` False or empty. This PR adjusts the componentSpec accordingly. Another nice win is that we do not have to define bools/dicts/lists as strings in the component spec Few things no note: * Due to strict type. Mixing different data types is not allowed -> we had to change the type of `input_partition_rows` to int. Which means it does not accept the value `disable` anymore. Need to fix this in a seperate PR * 'inf` as float is not allowed

RobbeSneyders

Thanks @GeorgesLorre.

Mostly looks good to me! Having a bit of a hard time understanding some of the argument parsing / converting, but the argument usage in the tests seems logical.

Left some comments.

examples/pipelines/datacomp/pipeline.py

pyproject.toml

.github/workflows/pipeline.yaml

src/fondant/executor.py

src/fondant/compiler.py

src/fondant/component_spec.py

RobbeSneyders · 2023-10-03T09:42:51Z

src/fondant/component_spec.py

-    ) -> "KubeflowComponentSpec":
-        """Create a Kubeflow component spec from a Fondant component spec."""
+    def from_fondant_component_spec(cls, fondant_component: ComponentSpec):
+        """Generate a Kubeflow component spec from a ComponentOp."""


Suggested change

"""Generate a Kubeflow component spec from a ComponentOp."""

"""Generate a Kubeflow component spec from a Fondant component spec."""

PhilippeMoussalli

Thanks Robbe! Left a few comments. Can we maybe merge this one into another branch and then tackle all other issues on other branches or would you prefer to merge directly to main?

PhilippeMoussalli · 2023-10-10T14:44:36Z

components/load_from_hf_hub/fondant_component.yaml

  n_rows_to_load:
    description: Optional argument that defines the number of rows to load. Useful for testing pipeline runs on a small scale
    type: int
-    default: None
+    default: -1
  index_column:
    description: Column to set index to in the load component, if not specified a default globally unique index will be set
    type: str


Does it make sense to later on revert back to None and handle the conversion internally? It could be something like setting an arbitrary value if it's None in the fondant component and parsing it back to None again when the component is run

I'm not sure if it's such a bad thing that the default needs to match the argument type. We would also need an arbitrary value for each type to make this work.

Before we would assign a value as an optional argument if the default is set to None. Now the notion of optional arguments is missing and optional arguments are checked implicitly against arbitrary values defined in the default which can be a bit confusing.

Was more thinking from the perspective of the end user. So if they would assign a default to None, we would handle it as optional and return it as None to the end user. Implementing this is still not very clear to me but we could just assign arbitrary known values. For example, if the users sets an list argument to None, we would pass it as [] and the convert it back to None during parsing

PhilippeMoussalli · 2023-10-10T14:46:46Z

components/load_from_hf_hub/src/main.py

-                 image_column_names: t.Optional[list],
-                 n_rows_to_load: t.Optional[int],
-                 index_column: t.Optional[str],
+                 image_column_names: list,


Can we keep them as optional since they have a default or does optional only apply when the default is None by convention?

Optional means that a None value can be passed in or set as a default. This is no longer the case now.

I still think we should have optional argument (see comment above)

docs/components/component_spec.md

examples/pipelines/datacomp/pipeline.py

…olving

RobbeSneyders · 2023-10-10T15:36:23Z

.pre-commit-config.yaml

+#        entry: python scripts/component_readme/generate_readme.py
+#        files: ^components/.*/fondant_component.yaml
+#        additional_dependencies: ["fondant"]


I had to deactivate this since it installs the latest version of fondant on PyPI, which breaks on the changes we made to the component spec schema. We can re-enable this later.

PhilippeMoussalli · 2023-10-11T07:25:48Z

examples/pipelines/datacomp/pipeline.py


 # add ops to pipeline
 pipeline.add_op(load_from_hub_op)
-# pipeline.add_op(download_images_op, dependencies=load_from_hub_op)


Can you uncomment here?

PhilippeMoussalli

Thanks Robbe, left one comment regarding arguments but I think we can discuss it separately

RobbeSneyders

Thanks, let's merge!

GeorgesLorre changed the title ~~Feature/kfp v2~~ KfpV2 and vertex Sep 29, 2023

GeorgesLorre changed the title ~~KfpV2 and vertex~~ KfpV2 and Vertex Sep 29, 2023

GeorgesLorre requested a review from RobbeSneyders September 29, 2023 09:25

GeorgesLorre force-pushed the feature/kfp-v2 branch 4 times, most recently from ae4d1c2 to 146e728 Compare October 2, 2023 13:49

GeorgesLorre and others added 9 commits October 2, 2023 16:30

Feature/vertex compiler (#411)

17451c5

Add vertex runner (#429)

52102a5

adresses: #417

Make ComponentSpec the base for arg building

03b1c3b

Make ComponentSpec the base for arg building

6faf90c

Feature/no artifacts (#444)

99016d7

Add more default/optional argument logic

12b74ae

Add cluser_type to default args

32a8c04

GeorgesLorre force-pushed the feature/kfp-v2 branch from 146e728 to 32a8c04 Compare October 2, 2023 14:50

Fix ruff error

e49867b

RobbeSneyders reviewed Oct 3, 2023

View reviewed changes

PhilippeMoussalli mentioned this pull request Oct 3, 2023

Remove kfp specific code from executor #376

Closed

RobbeSneyders added 3 commits October 9, 2023 22:40

Fix isOptional and defaultValue conversion

cf179f1

Update runner to use KfP v2 API

55fc4fe

Change input_partition_rows to accept -1 as default

fa14ea0

RobbeSneyders force-pushed the feature/kfp-v2 branch from 29e27c8 to fa14ea0 Compare October 9, 2023 21:55

RobbeSneyders and others added 6 commits October 10, 2023 00:12

Update load_from_hf_hub defaults

cd82bae

Update download_images defaults

987054b

Merge branch 'main' into feature/kfp-v2

44a7d4d

Merge branch 'main' into feature/kfp-v2

74f7099

re-enable cache

347eec4

Fix tests

c89998c

RobbeSneyders added 7 commits October 10, 2023 15:38

Update datacomp pipeline

1815d02

Merge remote-tracking branch 'origin/feature/kfp-v2' into feature/kfp-v2

081422e

Remove python version upper bound

931df56

Re-add test suite for Python 3.11

4342aa8

Add Python 3.12 upper bound

a789c03

Add gcp dependencies to vertex extra

2108894

Address PR comments

b778f5e

PhilippeMoussalli reviewed Oct 10, 2023

View reviewed changes

RobbeSneyders added 6 commits October 10, 2023 16:54

Add Python 3.12 trove classifier to pyproject.toml

c7e3a9f

Lower python upper bound to 3.11 again to prevent slow dependency res…

c6601eb

…olving

Remove 3.11 test suite

d32dffc

Address PR comments

fef720e

Changes based on self-review

3cb3a27

Update component defaults for kfpv2

35dad13

RobbeSneyders mentioned this pull request Oct 10, 2023

Update commoncrawl pipeline #508

Closed

RobbeSneyders reviewed Oct 10, 2023

View reviewed changes

Fix tests for kfp 2.3.0

c7ab7d5

RobbeSneyders marked this pull request as ready for review October 10, 2023 16:21

PhilippeMoussalli reviewed Oct 11, 2023

View reviewed changes

PhilippeMoussalli approved these changes Oct 11, 2023

View reviewed changes

disable kfpv2 default caching

0c6d403

RobbeSneyders approved these changes Oct 11, 2023

View reviewed changes

RobbeSneyders merged commit 6b46324 into main Oct 11, 2023
4 checks passed

RobbeSneyders deleted the feature/kfp-v2 branch October 11, 2023 09:50

RobbeSneyders changed the title ~~KfpV2 and Vertex~~ Migrate to KfpV2 Oct 11, 2023

This was referenced Oct 11, 2023

Run KfP v2 pipeline on cluster #494

Closed

Set defaults with matching types in all reusable and example components #505

Closed

Fix caching with KfP v2 #506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to KfpV2 #477

Migrate to KfpV2 #477

GeorgesLorre commented Sep 29, 2023

RobbeSneyders left a comment

RobbeSneyders Oct 3, 2023

PhilippeMoussalli left a comment

PhilippeMoussalli Oct 10, 2023

RobbeSneyders Oct 10, 2023

PhilippeMoussalli Oct 11, 2023

PhilippeMoussalli Oct 10, 2023

RobbeSneyders Oct 10, 2023

PhilippeMoussalli Oct 11, 2023

RobbeSneyders Oct 10, 2023

PhilippeMoussalli Oct 11, 2023

PhilippeMoussalli left a comment

RobbeSneyders left a comment

	"""Generate a Kubeflow component spec from a ComponentOp."""
	"""Generate a Kubeflow component spec from a Fondant component spec."""

Migrate to KfpV2 #477

Migrate to KfpV2 #477

Conversation

GeorgesLorre commented Sep 29, 2023

RobbeSneyders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippeMoussalli 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

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

PhilippeMoussalli left a comment

Choose a reason for hiding this comment

RobbeSneyders left a comment

Choose a reason for hiding this comment