First implementation of DockerCompiler #194

GeorgesLorre · 2023-06-09T12:24:36Z

Known TODO's:

support for build in docker compose
update documentation
update examples to include dockercompiler
update pipeline.py and move compile functionality to a KubeflowCompiler

PhilippeMoussalli

I Really like the implementation! looks quite clean ✨

Left a few minor comments, excited to see where this is heading :)

PhilippeMoussalli · 2023-06-09T13:14:11Z

fondant/compiler.py

+    """Compiler that creates a docker-compose spec from a pipeline."""
+
+    def compile(self, package_path: str = "docker-compose.yml") -> None:
+        """Compile a pipeline to docker-compose spec that is saved on the package_path."""


Suggested change

"""Compile a pipeline to docker-compose spec that is saved on the package_path."""

"""Compile a pipeline to docker-compose spec and save it to a specified package_path."""

PhilippeMoussalli · 2023-06-09T13:20:19Z

fondant/compiler.py

+        """
+        base_path = Path(self.pipeline.base_path)
+        # check if base path is an existing local folder
+        if base_path.exists():


what if the path is local but does not exist? it will assume it's remote In this case

Yes, even if it is not remote if it does not exist locally we can't mount it.

alright but maybe we can improve the error handling or try creating a new local path? now it assumes that if it's local and that it does not exist that it's remote (you assign it the base path)

PhilippeMoussalli · 2023-06-09T13:23:27Z

fondant/compiler.py

+        """
+        services = {}
+        for component_name, component in self.pipeline._graph.items():
+            safe_component_name = self._safe_component_name(component_name)


might be worth moving the safe name functionality into the pipeline when building the graph since we might need to do the renaming across different runners

Yes, will probably happen once I implement the KubeflowCompiler

PhilippeMoussalli · 2023-06-09T13:30:10Z

fondant/compiler.py

+        command = ["--metadata", json.dumps(metadata)]
+
+        # add in and out manifest paths to command
+        command.extend(["--output_manifest_path", f"{self.path}/manifest.txt"])


would maybe give a unique identifier to the manifest or write it within the component folder to avoid overriding it

Would be ideal if we can reuse the same path logic across compilers. @PhilippeMoussalli I'm having a hard time figuring out where the output_manifest_path is currently built when running with kubeflow. Can you point me to it?

The path of the output manifest path is created internally and stored in kubernetes with minio. When deploying kubeflow we specify a bucket and blob storage (in our case soy-audio-379412_kfp-artifacts/artifacts) where kubeflow will write the artifacts. The specific write folder is handled internally within kubeflow and cannot be modified afaik
(e.g soy-audio-379412_kfp-artifacts/artifacts/controlnet-pipeline-4pmng/2023/05/10/controlnet-pipeline-4pmng-1483856016/retrieve-images-output_manifest_path.tgz)

I will look more into this once I write the KubeflowCompiler which will hopefully clarify this

PhilippeMoussalli · 2023-06-09T13:30:19Z

fondant/compiler.py

+        depends_on = {}
+        if dependencies:
+            # there is only an input manifest if the component has dependencies
+            command.extend(["--input_manifest_path", f"{self.path}/manifest.txt"])


RobbeSneyders

Thanks @GeorgesLorre! Really nice to have this. Left some comments 🙂

RobbeSneyders · 2023-06-13T08:58:36Z

fondant/compiler.py

+class Compiler(ABC):
+    """Abstract base class for a compiler."""
+
+    def __init__(self, pipeline: Pipeline):


Why is the pipeline passed as an initialization argument instead of to the compile method. From a logical perspective, I wouldn't expect a compiler instance to be specific to a single pipeline.

RobbeSneyders · 2023-06-13T08:59:06Z

fondant/compiler.py

+        self.pipeline = pipeline
+
+    @abstractmethod
+    def compile(self):


This probably needs *args, **kwargs if we change the signature in the subclasses.

RobbeSneyders · 2023-06-13T09:03:24Z

fondant/compiler.py

+class DockerCompiler(Compiler):
+    """Compiler that creates a docker-compose spec from a pipeline."""
+
+    def compile(self, package_path: str = "docker-compose.yml") -> None:


Where does the name package_path come from? i.e. what's the package it refers to?

RobbeSneyders · 2023-06-13T09:06:18Z

fondant/compiler.py

+        return {"version": "3.8", "services": services}
+
+    def compose_service(
+        self, component_op: ComponentOp, dependencies: t.List[str]


Suggested change

self, component_op: ComponentOp, dependencies: t.List[str]

self, component_op: ComponentOp, *, dependencies: t.List[str]

RobbeSneyders · 2023-06-13T09:17:50Z

fondant/compiler.py

+                "source": str(base_path),
+                "target": f"/{base_path.stem}",
+            }
+            self.path = f"/{base_path.stem}"


Pylint would throw an attribute-defined-outside-init here :)

It's a bit hard to follow currently. Can we move the _patch_path() call to the _generate_spec method and pass them to compose_service? This is the only place they're used.

So something like:

def _generate_spec(self) -> dict: ... path, volume = self._patch_path() for component_name, component in self.pipeline._graph.items(): ... services[safe_component_name] = self.compose_service( component["fondant_component_op"], dependencies=component["dependencies"], path=path, volume=volume, )

RobbeSneyders · 2023-06-13T09:25:59Z

fondant/compiler.py

+        command = ["--metadata", json.dumps(metadata)]
+
+        # add in and out manifest paths to command
+        command.extend(["--output_manifest_path", f"{self.path}/manifest.txt"])


Would be ideal if we can reuse the same path logic across compilers. @PhilippeMoussalli I'm having a hard time figuring out where the output_manifest_path is currently built when running with kubeflow. Can you point me to it?

RobbeSneyders · 2023-06-13T09:29:10Z

tests/test_compiler.py

+        pipeline.add_op(component_op, dependencies=prev_comp)
+        prev_comp = component_op
+
+    pipeline.compile()


It's a bit strange that we compile the pipeline here and then pass it to a compiler again. Will this stay the case or will this change?

This should change. I did not touch the current kubeflow compiling. Once we have a dedicated KubefowCompiler we can clean up the pipeline.py code.

RobbeSneyders · 2023-06-13T09:31:02Z

tests/test_compiler.py

+            assert src.read() == truth.read()
+
+
+def test_docker_local_path(pipeline, tmp_path_factory):


Is there a reason to call the internals in this test and the one below, or can we just call the compile method as you did above and check the output? I think that would be a bit nicer as the tests then don't depend on the internal implementation.

Yes we can, downside is that I need to then store more files to compare against

PhilippeMoussalli · 2023-06-13T14:59:10Z

docs/pipeline.md

+compiler.compile()
+```
+
+The DockerCompiler will try to see if the `base_path` of the pipeline is local or remote. If local the base_path will be mounted as a bind volume on every service/component.


Suggested change

The DockerCompiler will try to see if the `base_path` of the pipeline is local or remote. If local the base_path will be mounted as a bind volume on every service/component.

The DockerCompiler will try to see if the `base_path` of the pipeline is local or remote. If local the `base_path` will be mounted as a bind volume on every service/component.

PhilippeMoussalli · 2023-06-13T15:00:48Z

fondant/compiler.py

+
+
+@dataclass
+class DockerVolume:


could you add a docstring explaining the class variables

PhilippeMoussalli · 2023-06-13T15:00:56Z

fondant/compiler.py

+
+
+@dataclass
+class MetaData:


PhilippeMoussalli

Thanks Georges :)

RobbeSneyders

Thanks @GeorgesLorre!

RobbeSneyders · 2023-06-13T21:04:56Z

docs/pipeline.md

+
+
+
+## Compiling a pipeline


We should probably explain when a user should use which compiler.

Known TODO's: - support for build in docker compose - update documentation - update examples to include dockercompiler - update pipeline.py and move compile functionality to a KubeflowCompiler

First implementation of DockerCompiler

f2f5923

GeorgesLorre requested review from RobbeSneyders, PhilippeMoussalli and NielsRogge June 9, 2023 12:24

Add basic logging

624be75

PhilippeMoussalli reviewed Jun 9, 2023

View reviewed changes

Update documentation

574dd5a

RobbeSneyders reviewed Jun 13, 2023

View reviewed changes

Clean up interface

e7be812

PhilippeMoussalli reviewed Jun 13, 2023

View reviewed changes

fondant/compiler.py

@dataclass

class DockerVolume:

Copy link

Contributor

PhilippeMoussalli Jun 13, 2023

Choose a reason for hiding this comment

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

could you add a docstring explaining the class variables

PhilippeMoussalli reviewed Jun 13, 2023

View reviewed changes

fondant/compiler.py

@dataclass

class MetaData:

Copy link

Contributor

PhilippeMoussalli Jun 13, 2023

Choose a reason for hiding this comment

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

same here

PhilippeMoussalli approved these changes Jun 13, 2023

View reviewed changes

RobbeSneyders approved these changes Jun 13, 2023

View reviewed changes

docs/pipeline.md

## Compiling a pipeline

Copy link

Member

RobbeSneyders Jun 13, 2023

Choose a reason for hiding this comment

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

We should probably explain when a user should use which compiler.

GeorgesLorre added 2 commits June 14, 2023 10:05

Add missing docstrings + rename package_path

eac9c47

Normalize docstrings

8e7f0fc

GeorgesLorre merged commit 86d001f into main Jun 14, 2023

GeorgesLorre deleted the feature/docker-compiler branch June 14, 2023 12:43

RobbeSneyders mentioned this pull request Aug 22, 2023

Create separate class for metadata #372

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First implementation of DockerCompiler #194

First implementation of DockerCompiler #194

GeorgesLorre commented Jun 9, 2023

PhilippeMoussalli left a comment

PhilippeMoussalli Jun 9, 2023

PhilippeMoussalli Jun 9, 2023

GeorgesLorre Jun 13, 2023 •

edited

Loading

PhilippeMoussalli Jun 13, 2023

PhilippeMoussalli Jun 9, 2023

GeorgesLorre Jun 13, 2023

PhilippeMoussalli Jun 9, 2023

RobbeSneyders Jun 13, 2023

PhilippeMoussalli Jun 13, 2023

GeorgesLorre Jun 13, 2023

PhilippeMoussalli Jun 9, 2023

RobbeSneyders left a comment

RobbeSneyders Jun 13, 2023

RobbeSneyders Jun 13, 2023

RobbeSneyders Jun 13, 2023

RobbeSneyders Jun 13, 2023

RobbeSneyders Jun 13, 2023

RobbeSneyders Jun 13, 2023

RobbeSneyders Jun 13, 2023

GeorgesLorre Jun 13, 2023

RobbeSneyders Jun 13, 2023

GeorgesLorre Jun 13, 2023

GeorgesLorre Jun 14, 2023

PhilippeMoussalli Jun 13, 2023

PhilippeMoussalli Jun 13, 2023

PhilippeMoussalli Jun 13, 2023

PhilippeMoussalli left a comment

RobbeSneyders left a comment

RobbeSneyders Jun 13, 2023

	"""Compile a pipeline to docker-compose spec that is saved on the package_path."""
	"""Compile a pipeline to docker-compose spec and save it to a specified package_path."""

	self, component_op: ComponentOp, dependencies: t.List[str]
	self, component_op: ComponentOp, *, dependencies: t.List[str]

		assert src.read() == truth.read()


		def test_docker_local_path(pipeline, tmp_path_factory):

	The DockerCompiler will try to see if the `base_path` of the pipeline is local or remote. If local the base_path will be mounted as a bind volume on every service/component.
	The DockerCompiler will try to see if the `base_path` of the pipeline is local or remote. If local the `base_path` will be mounted as a bind volume on every service/component.

First implementation of DockerCompiler #194

First implementation of DockerCompiler #194

Conversation

GeorgesLorre commented Jun 9, 2023

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

GeorgesLorre Jun 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

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

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

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

GeorgesLorre Jun 13, 2023 •

edited

Loading