From 04e359210f93d21a1efb93f6463db4512a90bfc7 Mon Sep 17 00:00:00 2001 From: Yash Gorana Date: Thu, 1 Feb 2024 14:32:46 +0530 Subject: [PATCH 1/6] [worker] fix multiple values for dicttuple --- .../syft/service/worker/worker_image_service.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/syft/src/syft/service/worker/worker_image_service.py b/packages/syft/src/syft/service/worker/worker_image_service.py index 13e2883d402..62500a49596 100644 --- a/packages/syft/src/syft/service/worker/worker_image_service.py +++ b/packages/syft/src/syft/service/worker/worker_image_service.py @@ -2,7 +2,6 @@ import contextlib from typing import List from typing import Optional -from typing import Tuple from typing import Union # third party @@ -204,14 +203,13 @@ def get_all( return SyftError(message=f"{result.err()}") images: List[SyftWorkerImage] = result.ok() - res: List[Tuple] = [] - for im in images: - if im.image_identifier is not None: - res.append((im.image_identifier.full_name_with_tag, im)) - else: - # FIXME: syft deployments in kubernetes results in a new image per version - # This results in "default-worker-image" key having multiple values and DictTuple() throws exception - res.append(("default-worker-image", im)) + res = {} + # if image is built then use full_name_with_tag as key + res.update( + {im.image_identifier.full_name_with_tag: im for im in images if im.is_built} + ) + # if image is not built then use uid as key + res.update({im.id: im for im in images if not im.is_built}) return DictTuple(res) From 9c4debbb1ad7cc5376960066aa6ff406e637eb1b Mon Sep 17 00:00:00 2001 From: Yash Gorana Date: Thu, 1 Feb 2024 14:33:14 +0530 Subject: [PATCH 2/6] [worker] validate dockerfile --- packages/syft/src/syft/custom_worker/config.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/syft/src/syft/custom_worker/config.py b/packages/syft/src/syft/custom_worker/config.py index 319273cc1a4..d19be1eb8ae 100644 --- a/packages/syft/src/syft/custom_worker/config.py +++ b/packages/syft/src/syft/custom_worker/config.py @@ -110,6 +110,13 @@ class DockerWorkerConfig(WorkerConfig): file_name: Optional[str] description: Optional[str] + @validator("dockerfile") + def validate_dockerfile(cls, dockerfile: str) -> str: + if not dockerfile: + raise ValueError("Dockerfile cannot be empty") + dockerfile = dockerfile.strip() + return dockerfile + @classmethod def from_path( cls, From 758014848beef5c289f5e4f361b94df7278780c6 Mon Sep 17 00:00:00 2001 From: Yash Gorana Date: Thu, 1 Feb 2024 14:34:14 +0530 Subject: [PATCH 3/6] [worker] fix pre built image config --- packages/syft/src/syft/custom_worker/config.py | 9 +++++++++ packages/syft/src/syft/service/worker/utils.py | 7 ++----- packages/syft/src/syft/service/worker/worker_image.py | 9 +++++++-- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/syft/src/syft/custom_worker/config.py b/packages/syft/src/syft/custom_worker/config.py index d19be1eb8ae..2e8846858af 100644 --- a/packages/syft/src/syft/custom_worker/config.py +++ b/packages/syft/src/syft/custom_worker/config.py @@ -104,6 +104,15 @@ def get_signature(self) -> str: return sha256(self.json(sort_keys=True).encode()).hexdigest() +@serializable() +class PrebuiltWorkerConfig(WorkerConfig): + # tag that is already built and pushed in some registry + tag: str + + def __str__(self) -> str: + return f"prebuilt tag='{self.tag}'" + + @serializable() class DockerWorkerConfig(WorkerConfig): dockerfile: str diff --git a/packages/syft/src/syft/service/worker/utils.py b/packages/syft/src/syft/service/worker/utils.py index a06ab53ef15..6a375fb889b 100644 --- a/packages/syft/src/syft/service/worker/utils.py +++ b/packages/syft/src/syft/service/worker/utils.py @@ -18,10 +18,10 @@ from ...custom_worker.builder_types import ImageBuildResult from ...custom_worker.builder_types import ImagePushResult from ...custom_worker.config import DockerWorkerConfig +from ...custom_worker.config import PrebuiltWorkerConfig from ...custom_worker.k8s import PodStatus from ...custom_worker.runner_k8s import KubernetesRunner from ...node.credentials import SyftVerifyKey -from ...types.datetime import DateTime from ...types.uid import UID from ...util.util import get_queue_address from ..response import SyftError @@ -489,16 +489,13 @@ def create_default_image( ) else: # in k8s we don't need to build the image, just the tag of backend is enough - - # a very bad and hacky way to keep the Stash's unique `config` requirment happy - worker_config = DockerWorkerConfig(dockerfile=tag) + worker_config = PrebuiltWorkerConfig(tag=tag) # create SyftWorkerImage from a pre-built image _new_image = SyftWorkerImage( config=worker_config, created_by=credentials, image_identifier=SyftWorkerImageIdentifier.from_str(tag), - built_at=DateTime.now(), ) result = image_stash.get_by_docker_config( diff --git a/packages/syft/src/syft/service/worker/worker_image.py b/packages/syft/src/syft/service/worker/worker_image.py index 337bd7c8b9b..3800694a940 100644 --- a/packages/syft/src/syft/service/worker/worker_image.py +++ b/packages/syft/src/syft/service/worker/worker_image.py @@ -2,6 +2,7 @@ from typing import Optional # relative +from ...custom_worker.config import PrebuiltWorkerConfig from ...custom_worker.config import WorkerConfig from ...node.credentials import SyftVerifyKey from ...serde.serializable import serializable @@ -31,9 +32,13 @@ class SyftWorkerImage(SyftObject): @property def is_built(self) -> bool: - """Returns True if the image has been built.""" + """Returns True if the image has been built or is prebuilt.""" - return self.built_at is not None + return self.built_at is not None or self.is_prebuilt + + @property + def is_prebuilt(self) -> bool: + return isinstance(self.config, PrebuiltWorkerConfig) @property def built_image_tag(self) -> Optional[str]: From c66c89275e7e8c0dcad6910a47878d1948e5469c Mon Sep 17 00:00:00 2001 From: Yash Gorana Date: Thu, 1 Feb 2024 14:34:26 +0530 Subject: [PATCH 4/6] [worker] fix repr and notebooks --- notebooks/api/0.8/10-container-images.ipynb | 10 +++++----- notebooks/api/0.8/11-container-images-k8s.ipynb | 8 ++++---- .../syft/src/syft/service/worker/image_identifier.py | 3 +++ packages/syft/src/syft/service/worker/worker_image.py | 8 +++++++- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/notebooks/api/0.8/10-container-images.ipynb b/notebooks/api/0.8/10-container-images.ipynb index d9d6923faae..81ba6a05d13 100644 --- a/notebooks/api/0.8/10-container-images.ipynb +++ b/notebooks/api/0.8/10-container-images.ipynb @@ -120,7 +120,7 @@ "\n", "RUN pip install pydicom\n", "\n", - "\"\"\"" + "\"\"\".strip()" ] }, { @@ -205,7 +205,7 @@ "source": [ "workerimage: SyftWorkerImage = None\n", "for image in dockerfile_list:\n", - " if image.config.dockerfile == custom_dockerfile_str:\n", + " if not image.is_prebuilt and image.config.dockerfile == custom_dockerfile_str:\n", " workerimage = image\n", " break\n", "\n", @@ -1051,7 +1051,7 @@ "FROM openmined/grid-backend:0.8.4-beta.12\n", "\n", "RUN pip install opendp\n", - "\"\"\"\n", + "\"\"\".strip()\n", "\n", "docker_config_2 = sy.DockerWorkerConfig(dockerfile=custom_dockerfile_str_2)" ] @@ -1202,7 +1202,7 @@ "FROM openmined/grid-backend:0.8.4-beta.12\n", "\n", "RUN pip install recordlinkage\n", - "\"\"\"\n", + "\"\"\".strip()\n", "\n", "docker_config_3 = sy.DockerWorkerConfig(dockerfile=custom_dockerfile_str_3)\n", "\n", @@ -1406,7 +1406,7 @@ "name": "python", "nbconvert_exporter": "python", "pygments_lexer": "ipython3", - "version": "3.11.2" + "version": "3.11.7" } }, "nbformat": 4, diff --git a/notebooks/api/0.8/11-container-images-k8s.ipynb b/notebooks/api/0.8/11-container-images-k8s.ipynb index fe3fd0ed66d..07c26a78b6a 100644 --- a/notebooks/api/0.8/11-container-images-k8s.ipynb +++ b/notebooks/api/0.8/11-container-images-k8s.ipynb @@ -126,7 +126,7 @@ "\n", "RUN pip install pydicom\n", "\n", - "\"\"\"" + "\"\"\".strip()" ] }, { @@ -205,7 +205,7 @@ " (\n", " image\n", " for image in dockerfile_list\n", - " if image.config.dockerfile == custom_dockerfile_str\n", + " if not image.is_prebuilt and image.config.dockerfile == custom_dockerfile_str\n", " ),\n", " None,\n", ")\n", @@ -766,7 +766,7 @@ "FROM {registry}/{repo}:{tag}\n", "\n", "RUN pip install opendp\n", - "\"\"\"\n", + "\"\"\".strip()\n", "\n", "docker_config_opendp = sy.DockerWorkerConfig(dockerfile=dockerfile_opendp)" ] @@ -996,7 +996,7 @@ "FROM {registry}/{repo}:{tag}\n", "\n", "RUN pip install recordlinkage\n", - "\"\"\"\n", + "\"\"\".strip()\n", "\n", "docker_config_recordlinkage = sy.DockerWorkerConfig(dockerfile=dockerfile_recordlinkage)\n", "\n", diff --git a/packages/syft/src/syft/service/worker/image_identifier.py b/packages/syft/src/syft/service/worker/image_identifier.py index d1995b36c87..531b5d3b726 100644 --- a/packages/syft/src/syft/service/worker/image_identifier.py +++ b/packages/syft/src/syft/service/worker/image_identifier.py @@ -93,5 +93,8 @@ def registry_host(self) -> str: def __hash__(self) -> int: return hash(self.repo + self.tag + str(hash(self.registry))) + def __str__(self) -> str: + return self.full_name_with_tag + def __repr__(self) -> str: return f"SyftWorkerImageIdentifier(repo={self.repo}, tag={self.tag}, registry={self.registry})" diff --git a/packages/syft/src/syft/service/worker/worker_image.py b/packages/syft/src/syft/service/worker/worker_image.py index 3800694a940..38baed1d2cb 100644 --- a/packages/syft/src/syft/service/worker/worker_image.py +++ b/packages/syft/src/syft/service/worker/worker_image.py @@ -20,7 +20,13 @@ class SyftWorkerImage(SyftObject): __attr_unique__ = ["config"] __attr_searchable__ = ["config", "image_hash", "created_by"] - __repr_attrs__ = ["image_identifier", "image_hash", "created_at", "built_at"] + __repr_attrs__ = [ + "image_identifier", + "image_hash", + "created_at", + "built_at", + "config", + ] id: UID config: WorkerConfig From f32908d360ff80c85f71d667091100bae968d6ba Mon Sep 17 00:00:00 2001 From: Yash Gorana Date: Thu, 1 Feb 2024 16:13:12 +0530 Subject: [PATCH 5/6] [tests] fix dockerfile test --- packages/syft/tests/syft/custom_worker/config_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/syft/tests/syft/custom_worker/config_test.py b/packages/syft/tests/syft/custom_worker/config_test.py index be56782154a..2295c5f8a64 100644 --- a/packages/syft/tests/syft/custom_worker/config_test.py +++ b/packages/syft/tests/syft/custom_worker/config_test.py @@ -199,7 +199,7 @@ def test_docker_worker_config(dockerfile_path: Path, method: str) -> None: else: raise ValueError(f"method must be one of {METHODS}") - assert docker_config.dockerfile == dockerfile_path.read_text() + assert docker_config.dockerfile == dockerfile_path.read_text().strip() assert docker_config.description == description new_description = description + " (syft version is 0.8.4-beta.12)" docker_config.set_description(description_text=new_description) From 31e9491777bcf233c3ac5ca82994d0c40a0ad578 Mon Sep 17 00:00:00 2001 From: Yash Gorana Date: Thu, 1 Feb 2024 18:07:25 +0530 Subject: [PATCH 6/6] [worker] use id strings for keys --- packages/syft/src/syft/custom_worker/config.py | 9 ++++++++- packages/syft/src/syft/service/worker/utils.py | 5 ++++- .../syft/src/syft/service/worker/worker_image_service.py | 7 ++++--- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/syft/src/syft/custom_worker/config.py b/packages/syft/src/syft/custom_worker/config.py index 2e8846858af..7c3867a5392 100644 --- a/packages/syft/src/syft/custom_worker/config.py +++ b/packages/syft/src/syft/custom_worker/config.py @@ -108,9 +108,16 @@ def get_signature(self) -> str: class PrebuiltWorkerConfig(WorkerConfig): # tag that is already built and pushed in some registry tag: str + description: Optional[str] def __str__(self) -> str: - return f"prebuilt tag='{self.tag}'" + if self.description: + return f"prebuilt tag='{self.tag}' description='{self.description}'" + else: + return f"prebuilt tag='{self.tag}'" + + def set_description(self, description_text: str) -> None: + self.description = description_text @serializable() diff --git a/packages/syft/src/syft/service/worker/utils.py b/packages/syft/src/syft/service/worker/utils.py index 6a375fb889b..5cf42a12883 100644 --- a/packages/syft/src/syft/service/worker/utils.py +++ b/packages/syft/src/syft/service/worker/utils.py @@ -489,7 +489,10 @@ def create_default_image( ) else: # in k8s we don't need to build the image, just the tag of backend is enough - worker_config = PrebuiltWorkerConfig(tag=tag) + worker_config = PrebuiltWorkerConfig( + tag=tag, + description="Prebuilt default worker image", + ) # create SyftWorkerImage from a pre-built image _new_image = SyftWorkerImage( diff --git a/packages/syft/src/syft/service/worker/worker_image_service.py b/packages/syft/src/syft/service/worker/worker_image_service.py index 62500a49596..78b16a67395 100644 --- a/packages/syft/src/syft/service/worker/worker_image_service.py +++ b/packages/syft/src/syft/service/worker/worker_image_service.py @@ -204,12 +204,13 @@ def get_all( images: List[SyftWorkerImage] = result.ok() res = {} - # if image is built then use full_name_with_tag as key + # if image is built index by full_name_with_tag res.update( {im.image_identifier.full_name_with_tag: im for im in images if im.is_built} ) - # if image is not built then use uid as key - res.update({im.id: im for im in images if not im.is_built}) + # and then index all images by id + # TODO: jupyter repr needs to be updated to show unique values (even if multiple keys point to same value) + res.update({im.id.to_string(): im for im in images if not im.is_built}) return DictTuple(res)