Add kfp compiler #291

GeorgesLorre · 2023-07-12T13:54:37Z

Untested -> looking for feedback on the structure not the content

PhilippeMoussalli · 2023-07-13T07:27:54Z

src/fondant/pipeline.py

-        pipeline_versions = self.client.list_pipeline_versions(pipeline_id).versions
-        return [version.id for version in pipeline_versions]
-
-    def delete_pipeline(self, pipeline_name: str):


Where do you see all the other functionalities that are not related to compiling and running? user might still want to list pipeline runs, delete pipelines, ...

Not sure if we need to wrap this functionality for every platform we want to run on. At least at this point in time, I would be ok with removing it and having the user work with the underlying platform directly.

makes sense, we'll re-add it later if we notice that it's needed

src/fondant/compiler.py

PhilippeMoussalli · 2023-07-13T07:32:27Z

src/fondant/compiler.py

+
+    def compile(
+        self,
+        pipeline: Pipeline,


I think the Pipeline is the unifying interface between all runners right? if so, would add it to the abstract class

PhilippeMoussalli

I like the overall structure and separation between the Compiler and Runner!

I think there are still some things that don't fall quite between like Client related functionalities. Not the highest priority but we should figure out where those land

RobbeSneyders

Thanks @GeorgesLorre!

The lines along you split the behavior look good to me!

src/fondant/compiler.py

RobbeSneyders · 2023-07-13T08:52:12Z

src/fondant/compiler.py

+        except ImportError:
+            raise ImportError(
+                "You need to install kfp to use the Kubeflow compiler, "
+                / "you can install it with `poetry install --extras kfp`"


Suggested change

/ "you can install it with `poetry install --extras kfp`"

/ "you can install it with `pip install fondant[kfp]`"

poetry install is only for fondant developers. Users can also use poetry, but then they'll install their own project and need to indicate the fondant extras with the fondant dependency in their pyproject.toml.

Related to the above:

https://github.com/ml6team/fondant/blob/86867a5e777c93a7ca0d982f263d66c38abc3e8f/src/fondant/import_utils.py#L24C1-L25C1

RobbeSneyders · 2023-07-13T08:53:47Z

src/fondant/runner.py

+                "You need to install kfp to use the Kubeflow compiler, "
+                / "you can install it with `poetry install --extras kfp`"


Suggested change

"You need to install kfp to use the Kubeflow compiler, "

/ "you can install it with `poetry install --extras kfp`"

"You need to install kfp to use the Kubeflow runner, "

/ "you can install it with `pip install fondant[kfp]`"

RobbeSneyders · 2023-07-13T08:55:54Z

src/fondant/pipeline.py

-        pipeline_versions = self.client.list_pipeline_versions(pipeline_id).versions
-        return [version.id for version in pipeline_versions]
-
-    def delete_pipeline(self, pipeline_name: str):


Not sure if we need to wrap this functionality for every platform we want to run on. At least at this point in time, I would be ok with removing it and having the user work with the underlying platform directly.

RobbeSneyders · 2023-07-13T08:57:10Z

src/fondant/runner.py

+    def run(cls, input_spec: str, host: str, *args, **kwargs):
+        """Run a kubeflow pipeline."""
+        cls._resolve_imports()
+        client = kfp.Client(host=host)


This probably doesn't change a lot to the current functionality, but it would make more sense to me to instantiate this in __init__().

RobbeSneyders

Thanks @GeorgesLorre!

The general direction looks good. Left some comments.

RobbeSneyders · 2023-08-07T14:48:06Z

src/fondant/compiler.py

+            self.kfp = kfp
+        except ImportError:
+            msg = """You need to install kfp to use the Kubeflow compiler,\n
+                     you can install it with `pip install --extras kfp`"""


Suggested change

you can install it with `pip install --extras kfp`"""

you can install it with `pip install fondant[pipelines]`"""

Although we probably want to split this up for the different runners so it becomes fondant[kfp].

RobbeSneyders · 2023-08-07T14:53:27Z

src/fondant/compiler.py

+        self.pipeline.sort_graph()
+        self.pipeline._validate_pipeline_definition("{{workflow.name}}")


This should be done in the DockerCompiler as well, no?

RobbeSneyders · 2023-08-07T14:54:21Z

src/fondant/pipeline.py

@@ -18,8 +17,6 @@
 from fondant.schema import validate_partition_number, validate_partition_size

 if is_kfp_available():


Ideally we can get rid of this and make this file completely runner independent.

This is only needed for setting p_volumes at the componentOp level, that was only needed when we were initially planning to download the Laion dataset locally to use it. I think we can remove both ephemeral_storage_size and p_volumes from the ComponenOp. We can always re-add them later if we notice they're needed

RobbeSneyders · 2023-08-07T14:55:18Z

src/fondant/runner.py

+    def _resolve_imports(self):
+        """Resolve imports for the Kubeflow compiler."""
+        try:
+            global kfp


Can we assign to self.kfp here as in the compiler?

RobbeSneyders · 2023-08-07T14:55:31Z

src/fondant/runner.py

+        except ImportError:
+            raise ImportError(
+                "You need to install kfp to use the Kubeflow compiler, "
+                / "you can install it with `pip install --extras kfp`",


See comment on same line above.

RobbeSneyders · 2023-08-07T14:55:47Z

src/fondant/runner.py

+        # client = kfp.Client(host=host)
+        # # TODO add logic to see if pipeline exists
+        # pipeline_spec = client.run_pipeline(
+        #     experiment_id=experiment.id,
+        #     job_name=run_name,
+        #     pipeline_package_path=pipeline.package_path,
+        # )
+
+        # pipeline_url = f"{self.host}/#/runs/details/{pipeline_spec.id}"
+        # logger.info(f"Pipeline is running at: {pipeline_url}")


Is there a reason this is still commented out?

should not have committed this, will be taken up in a separate PR addressing the kfp runner

RobbeSneyders · 2023-08-07T14:57:17Z

tests/test_pipeline.py

    """
-    Test that an exception is raised when the passed invalid argument name or type to the fondant
-    component does not match the ones specified in the fondant specifications.
+    Test that an InvalidPipelineDefinition exception is raised when attempting to compile


You're no longer compiling in the test :)

RobbeSneyders · 2023-08-07T14:57:32Z

tests/test_pipeline.py

    ],
 )
 def test_invalid_pipeline_compilation(
    default_pipeline_args,
    invalid_pipeline_example,
-    tmp_path,
 ):
    """
    Test that an InvalidPipelineDefinition exception is raised when attempting to compile


You're no longer compiling in the test :)

RobbeSneyders · 2023-08-07T14:58:04Z

tests/test_pipeline.py

@@ -169,12 +165,12 @@ def test_invalid_pipeline_dependencies(
    [
        ("example_1", ["first_component", "second_component"]),
        ("example_2", ["first_component", "second_component"]),
+        ("example_3", ["first_component", "second_component"]),
    ],
 )
 def test_invalid_pipeline_compilation(


Suggested change

def test_invalid_pipeline_compilation(

def test_invalid_pipeline_validation(

RobbeSneyders · 2023-08-07T15:48:29Z

src/fondant/components

This should not be removed

PhilippeMoussalli

Thanks Georges! Looks much cleaner already ✨

PhilippeMoussalli · 2023-08-08T11:53:26Z

src/fondant/compiler.py

+        self.kfp.compiler.Compiler().compile(wrapped_pipeline, output_path)  # type: ignore
+        logger.info("Pipeline compiled successfully")
+
+    def kf_pipeline(self):


Suggested change

def kf_pipeline(self):

def kfp_pipeline(self):

PhilippeMoussalli · 2023-08-08T11:54:27Z

src/fondant/compiler.py

+                    metadata=metadata,
+                    **component_args,
+                )
+            else:


No else statement needed, the two statements are the same, I think now they can be the same since we're passing the metadata to every components

src/fondant/compiler.py

PhilippeMoussalli · 2023-08-08T12:09:14Z

src/fondant/pipeline.py

@@ -18,8 +17,6 @@
 from fondant.schema import validate_partition_number, validate_partition_size

 if is_kfp_available():


This is only needed for setting p_volumes at the componentOp level, that was only needed when we were initially planning to download the Laion dataset locally to use it. I think we can remove both ephemeral_storage_size and p_volumes from the ComponenOp. We can always re-add them later if we notice they're needed

GeorgesLorre · 2023-08-08T13:20:25Z

@RobbeSneyders updated!

GeorgesLorre · 2023-08-08T13:37:35Z

@PhilippeMoussalli Ok I'll remove the ephemeral_storage_size and p_volumes in a new PR. This would indeed be cleaner. #342

RobbeSneyders

Thanks! Can you just update the error message for the kfp import before merging?

Move all kfp related stuff from the Pipeline to a separate kfp compiler

GeorgesLorre requested review from RobbeSneyders and PhilippeMoussalli July 12, 2023 13:54

PhilippeMoussalli reviewed Jul 13, 2023

View reviewed changes

src/fondant/compiler.py Show resolved Hide resolved

PhilippeMoussalli reviewed Jul 13, 2023

View reviewed changes

RobbeSneyders reviewed Jul 13, 2023

View reviewed changes

GeorgesLorre force-pushed the feature/kf-compiler branch from 9f3d36d to e7e98f2 Compare July 19, 2023 12:04

GeorgesLorre force-pushed the feature/kf-compiler branch from e7e98f2 to 6d26bb3 Compare August 3, 2023 13:04

RobbeSneyders reviewed Aug 7, 2023

View reviewed changes

src/fondant/components Outdated

Copy link

Member

RobbeSneyders Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be removed

PhilippeMoussalli mentioned this pull request Aug 8, 2023

[DataComp] Update pipeline name, remove DockerCompiler #340

Merged

PhilippeMoussalli reviewed Aug 8, 2023

View reviewed changes

GeorgesLorre added 5 commits August 8, 2023 14:55

Add draft structure for kfp refactor

a0838ce

Update kubeflow compiler code for new structure

0b493ba

Fix loose imports

f59b791

Update compiler and pipeline tests to match new structure

c12fd20

Make validate method for pipeline and call it from the compilers

5cc072f

GeorgesLorre force-pushed the feature/kf-compiler branch from 6d26bb3 to 5cc072f Compare August 8, 2023 12:55

Fix test after rebase

48434d1

GeorgesLorre changed the title ~~Add draft structure for kfp refactor~~ Add kfp compiler Aug 8, 2023

Rename kf to kfp

17a569d

RobbeSneyders approved these changes Aug 10, 2023

View reviewed changes

Update ImportError message

06c23eb

GeorgesLorre merged commit 262fd88 into main Aug 10, 2023

GeorgesLorre deleted the feature/kf-compiler branch August 10, 2023 08:59

Hakimovich99 pushed a commit that referenced this pull request Oct 16, 2023

Add kfp compiler (#291)

5e5957d

Move all kfp related stuff from the Pipeline to a separate kfp compiler

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kfp compiler #291

Add kfp compiler #291

GeorgesLorre commented Jul 12, 2023 •

edited

Loading

PhilippeMoussalli Jul 13, 2023

RobbeSneyders Jul 13, 2023

PhilippeMoussalli Jul 13, 2023

PhilippeMoussalli Jul 13, 2023

PhilippeMoussalli left a comment

RobbeSneyders left a comment

RobbeSneyders Jul 13, 2023

PhilippeMoussalli Jul 13, 2023 •

edited

Loading

RobbeSneyders Jul 13, 2023

RobbeSneyders Jul 13, 2023

RobbeSneyders Jul 13, 2023

RobbeSneyders left a comment

RobbeSneyders Aug 7, 2023

RobbeSneyders Aug 7, 2023

RobbeSneyders Aug 7, 2023

PhilippeMoussalli Aug 8, 2023

RobbeSneyders Aug 7, 2023

RobbeSneyders Aug 7, 2023

RobbeSneyders Aug 7, 2023

GeorgesLorre Aug 8, 2023

RobbeSneyders Aug 7, 2023

RobbeSneyders Aug 7, 2023

RobbeSneyders Aug 7, 2023

RobbeSneyders Aug 7, 2023

PhilippeMoussalli left a comment

PhilippeMoussalli Aug 8, 2023

PhilippeMoussalli Aug 8, 2023

PhilippeMoussalli Aug 8, 2023

GeorgesLorre commented Aug 8, 2023

GeorgesLorre commented Aug 8, 2023 •

edited

Loading

RobbeSneyders left a comment

	/ "you can install it with `poetry install --extras kfp`"
	/ "you can install it with `pip install fondant[kfp]`"

		"You need to install kfp to use the Kubeflow compiler, "
		/ "you can install it with `poetry install --extras kfp`"

	you can install it with `pip install --extras kfp`"""
	you can install it with `pip install fondant[pipelines]`"""

		self.pipeline.sort_graph()
		self.pipeline._validate_pipeline_definition("{{workflow.name}}")

		@@ -18,8 +17,6 @@
		from fondant.schema import validate_partition_number, validate_partition_size

		if is_kfp_available():

	def test_invalid_pipeline_compilation(
	def test_invalid_pipeline_validation(

Add kfp compiler #291

Add kfp compiler #291

Conversation

GeorgesLorre commented Jul 12, 2023 • edited Loading

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

Choose a reason for hiding this comment

PhilippeMoussalli Jul 13, 2023 • edited Loading

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

RobbeSneyders 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgesLorre commented Aug 8, 2023

GeorgesLorre commented Aug 8, 2023 • edited Loading

RobbeSneyders left a comment

Choose a reason for hiding this comment

GeorgesLorre commented Jul 12, 2023 •

edited

Loading

PhilippeMoussalli Jul 13, 2023 •

edited

Loading

GeorgesLorre commented Aug 8, 2023 •

edited

Loading