Feature/vertex compiler #411

GeorgesLorre · 2023-09-06T11:48:26Z

No description provided.

src/fondant/component_spec.py

GeorgesLorre · 2023-09-06T11:52:44Z

pyproject.toml

@@ -41,7 +41,7 @@ classifiers = [
 ]

 [tool.poetry.dependencies]
-python = ">= 3.8"
+python = ">= 3.8 < 3.12"


kfp(v2) only supports up to python 3.12

src/fondant/compiler.py

GeorgesLorre · 2023-09-06T11:58:27Z

src/fondant/component_spec.py

-                    "type": "String",
+    def from_fondant_component_spec(cls, fondant_component: ComponentSpec):
+        """Generate a Kubeflow component spec from a ComponentOp."""
+        input_definitions = {


split now between artifacts and parameters

PhilippeMoussalli

Thanks Geroges! Really nice work, looking clean ✨

Still a bit concerned on how this will end up integrating with KFP. Just out of curiosity, what would break with your current implementation if you switch back to v1 and import from v2?

PhilippeMoussalli · 2023-09-06T12:01:24Z

src/fondant/component_spec.py

-    "bool": "Boolean",
-    "dict": "JsonObject",
-    "list": "JsonArray",
+    "str": "STRING",


Perhaps we can import this dict and use it?

https://github.com/kubeflow/pipelines/blob/2b05ec867fad84e24fe73ef7515e3b5849297e79/sdk/python/kfp/dsl/types/type_utils.py#L56

We can maybe do the same for V1 as well instead of defining our own custom one
https://github.com/kubeflow/pipelines/blob/2b05ec867fad84e24fe73ef7515e3b5849297e79/sdk/python/kfp/dsl/types/type_utils.py#L86

FYI I just stumbled on this when trying to look out for the v2 types since I couldn't find them in the docs

I'll add it as a task

PhilippeMoussalli · 2023-09-06T12:09:26Z

src/fondant/compiler.py

+
+                else:
+                    component_task = kubeflow_component_op(
+                        metadata=metadata,


Why not follow the same approach as the KFP compiler and set the input manifest as optional?

PhilippeMoussalli · 2023-09-06T12:10:49Z

src/fondant/compiler.py

+
+                component_op = component["fondant_component_op"]
+                # convert ComponentOp to Kubeflow component
+                kubeflow_component_op = self.kfp.components.load_component_from_text(


It seems like this is the only difference between the two compiler (the SDK changed for this function). Can we perhaps reuse the same function instead of defining it twice. Seems like there is quite a bit of overlap

PhilippeMoussalli · 2023-09-06T12:21:15Z

src/fondant/executor.py

-        if is_kubeflow_output:
-            # Save to the expected base path directory
-            safe_component_name = self.spec.name.replace(" ", "_").lower()
-            save_path_base_path = (


Are we then no longer saving the manifest relative to the basepath? We need it for data exploration
#368 (comment)

Added as task

GeorgesLorre · 2023-09-07T15:18:36Z

Thanks Geroges! Really nice work, looking clean ✨

Still a bit concerned on how this will end up integrating with KFP. Just out of curiosity, what would break with your current implementation if you switch back to v1 and import from v2?

I'll add it as a task

PhilippeMoussalli · 2023-09-07T15:26:59Z

src/fondant/components/embedding_based_laion_retrieval/fondant_component.yaml

@@ -0,0 +1,31 @@
+name: LAION retrieval


Why are different component spcs added?

PhilippeMoussalli · 2023-09-07T15:28:03Z

tests/test_compiler.py

-            VALID_PIPELINE / "kubeflow_pipeline.yml",
-        ) as truth:
-            assert yaml.safe_load(src) == yaml.safe_load(truth)
+# @pytest.mark.usefixtures("_freeze_time")


Can you re-introduce this again?

ahh ok never mind it's because of the new API, would tackle it in another ticket

yes indeed I don't want to keep adding to this PR.

added as a task: #393

PhilippeMoussalli · 2023-09-07T15:30:21Z

src/fondant/component_spec.py

-                    description=info["description"],
-                    type=info["type"],
-                    default=info["default"] if "default" in info else None,
+        args = {}


A lof of repeated code between input and output arguments, can be one put in one function

Agreed but this will probably be removed once we use the component spec as base for building the arguments and not the KubeflowComponentSpec.

#393

PhilippeMoussalli · 2023-09-07T15:35:04Z

src/fondant/compiler.py

-                )
-
-                # Set image pull policy to always
-                component_task.container.set_image_pull_policy("Always")


might be nice to keep the image pull policy (unless it's another function in v2 in which case we can tackle it separately or create a task for it)

added as a task: #393

PhilippeMoussalli · 2023-09-07T15:37:11Z

tests/test_compiler.py

@@ -83,7 +83,7 @@ def now(cls):
 @pytest.fixture(params=TEST_PIPELINES)
 def setup_pipeline(request, tmp_path, monkeypatch):
    pipeline = Pipeline(
-        pipeline_name="test_pipeline",
+        pipeline_name="testpipeline",


any special reason for this name change?

mrchtr · 2023-09-08T06:46:42Z

src/fondant/component_spec.py

+                                "--output_manifest_path",
+                                "{{$.outputs.artifacts['output_manifest_path'].uri}}",
+                            ],
+                            "command": ["python3", "main.py"],


Shouldn't we use here fondant execute main instead?

ah really ? Where do we do that ? I just took over the way we did it for kfpv1.

We changed while resolving this: #362

See comment above

PhilippeMoussalli · 2023-09-11T08:06:27Z

src/fondant/component_spec.py

-            "implementation": {
-                "container": {
-                    "image": fondant_component.image,
-                    "command": [


I think this is the command that you replaced, we're using this now for the components based on this PR https://github.com/ml6team/fondant/commits/main?before=82b5131ac8e72c620917d65c31c80b0b903e4d83+35&branch=main&qualified_name=refs%2Fheads%2Fmain

Can you replace and check if all still runs fine?

PhilippeMoussalli

Thanks Georges!

GeorgesLorre requested a review from PhilippeMoussalli September 6, 2023 11:48

GeorgesLorre commented Sep 6, 2023

View reviewed changes

src/fondant/component_spec.py Outdated Show resolved Hide resolved

GeorgesLorre commented Sep 6, 2023

View reviewed changes

src/fondant/compiler.py Outdated Show resolved Hide resolved

GeorgesLorre commented Sep 6, 2023

View reviewed changes

PhilippeMoussalli reviewed Sep 6, 2023

View reviewed changes

GeorgesLorre force-pushed the feature/vertex-compiler branch from 0fc188c to bc197e9 Compare September 6, 2023 14:27

GeorgesLorre marked this pull request as ready for review September 7, 2023 15:12

GeorgesLorre requested a review from mrchtr September 7, 2023 15:13

PhilippeMoussalli reviewed Sep 7, 2023

View reviewed changes

mrchtr reviewed Sep 8, 2023

View reviewed changes

GeorgesLorre added 8 commits September 11, 2023 09:54

Add first version of the vertex-compiler

96b9883

Add first version of the vertex-compiler

1fbead5

Add first version of the vertex-compiler

c531a2e

Add first version of the vertex-compiler

08aea04

Add first version of the vertex-compiler

4bfbe60

Fix argument building

9ff9cb5

Fix tests

8affd1a

Remove components

4aaf2ed

GeorgesLorre force-pushed the feature/vertex-compiler branch from cdca57b to 4aaf2ed Compare September 11, 2023 07:54

PhilippeMoussalli reviewed Sep 11, 2023

View reviewed changes

Update invocation of fondant

a97727c

PhilippeMoussalli approved these changes Sep 11, 2023

View reviewed changes

GeorgesLorre merged commit b1f430c into feature/kfp-v2 Sep 11, 2023

GeorgesLorre deleted the feature/vertex-compiler branch September 11, 2023 11:13

GeorgesLorre added a commit that referenced this pull request Sep 29, 2023

Feature/vertex compiler (#411)

b0d72ad

GeorgesLorre added a commit that referenced this pull request Oct 2, 2023

Feature/vertex compiler (#411)

a39cdbc

GeorgesLorre added a commit that referenced this pull request Oct 2, 2023

Feature/vertex compiler (#411)

17451c5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/vertex compiler #411

Feature/vertex compiler #411

GeorgesLorre commented Sep 6, 2023

GeorgesLorre Sep 6, 2023

GeorgesLorre Sep 6, 2023

PhilippeMoussalli left a comment

PhilippeMoussalli Sep 6, 2023

GeorgesLorre Sep 7, 2023

PhilippeMoussalli Sep 6, 2023

PhilippeMoussalli Sep 6, 2023

PhilippeMoussalli Sep 6, 2023

GeorgesLorre Sep 7, 2023

GeorgesLorre commented Sep 7, 2023

PhilippeMoussalli Sep 7, 2023

PhilippeMoussalli Sep 7, 2023

PhilippeMoussalli Sep 7, 2023

GeorgesLorre Sep 11, 2023

PhilippeMoussalli Sep 7, 2023

GeorgesLorre Sep 11, 2023

PhilippeMoussalli Sep 7, 2023

GeorgesLorre Sep 11, 2023

PhilippeMoussalli Sep 7, 2023

mrchtr Sep 8, 2023

GeorgesLorre Sep 11, 2023

mrchtr Sep 11, 2023

PhilippeMoussalli Sep 11, 2023

PhilippeMoussalli Sep 11, 2023

PhilippeMoussalli Sep 11, 2023

GeorgesLorre Sep 11, 2023

PhilippeMoussalli left a comment

Feature/vertex compiler #411

Feature/vertex compiler #411

Conversation

GeorgesLorre commented Sep 6, 2023

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

GeorgesLorre commented Sep 7, 2023

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

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