From 389e3afbf103c07d09a8d9b99c9526514b3cf7b2 Mon Sep 17 00:00:00 2001 From: Alex Wu Date: Mon, 11 Aug 2025 11:57:11 +0800 Subject: [PATCH 1/4] fix: override should_build function in noop_builder Signed-off-by: Alex Wu --- flytekit/image_spec/noop_builder.py | 14 ++++++++ .../unit/core/image_spec/test_image_spec.py | 34 +++++++++++++++++++ .../unit/core/image_spec/test_noop_builder.py | 16 +++++++++ 3 files changed, 64 insertions(+) diff --git a/flytekit/image_spec/noop_builder.py b/flytekit/image_spec/noop_builder.py index 8bbf7d89a8..004556adad 100644 --- a/flytekit/image_spec/noop_builder.py +++ b/flytekit/image_spec/noop_builder.py @@ -6,6 +6,20 @@ class NoOpBuilder(ImageSpecBuilder): builder_type = "noop" + def should_build(self, image_spec: ImageSpec) -> bool: + """ + The build_image function of NoOpBuilder uses the image_spec name as defined by the user without + checking whether the image exists in the Docker registry. Therefore, the should_build function + should always return True to trigger the build_image function. + + Args: + image_spec (ImageSpec): Image specification + + Returns: + bool: Always returns True + """ + return True + def build_image(self, image_spec: ImageSpec) -> str: if not isinstance(image_spec.base_image, str): msg = "base_image must be a string to use the noop image builder" diff --git a/tests/flytekit/unit/core/image_spec/test_image_spec.py b/tests/flytekit/unit/core/image_spec/test_image_spec.py index a94dec1c32..8d0c9e82d2 100644 --- a/tests/flytekit/unit/core/image_spec/test_image_spec.py +++ b/tests/flytekit/unit/core/image_spec/test_image_spec.py @@ -359,3 +359,37 @@ def test_with_builder_options(): "existing_builder_option_1": "existing_builder_option_value_1", "new_builder_option_1": "new_builder_option_value_1" } + +def test_noop_builder_updates_image_name_mapping(): + from flytekit.image_spec.noop_builder import NoOpBuilder + + # Clear any existing mappings to ensure clean test state + ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME.clear() + + # Register NoOpBuilder + ImageBuildEngine.register("noop", NoOpBuilder()) + + # Create an image spec with NoOpBuilder + expected_real_name = "localhost:30000/test_image:latest" + image_spec = ImageSpec( + name="test_image", + builder="noop", + base_image=expected_real_name + ) + + # Get the image name before building + img_name = image_spec.image_name() + + # Build the image + ImageBuildEngine.build(image_spec) + + # Verify that the mapping was created in _IMAGE_NAME_TO_REAL_NAME + assert img_name in ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME + + # Verify the mapping value is correct + actual_real_name = ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME[img_name] + assert actual_real_name == expected_real_name + + # Clean up + del ImageBuildEngine._REGISTRY["noop"] + ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME.clear() diff --git a/tests/flytekit/unit/core/image_spec/test_noop_builder.py b/tests/flytekit/unit/core/image_spec/test_noop_builder.py index c9812ba0dc..495395ce7f 100644 --- a/tests/flytekit/unit/core/image_spec/test_noop_builder.py +++ b/tests/flytekit/unit/core/image_spec/test_noop_builder.py @@ -22,3 +22,19 @@ def test_noop_builder_error(base_image): image_spec = ImageSpec(base_image=base_image) with pytest.raises(ValueError, match=msg): builder.build_image(image_spec) + + +def test_noop_builder_should_build(): + """Test that NoOpBuilder.should_build always returns True.""" + builder = NoOpBuilder() + + # Test with different image specs to ensure should_build always returns True + test_cases = [ + ImageSpec(base_image="localhost:30000/flytekit"), + ImageSpec(base_image="python:3.9"), + ImageSpec(base_image="custom/image:latest"), + ] + + for image_spec in test_cases: + result = builder.should_build(image_spec) + assert result is True, f"should_build should return True for {image_spec.base_image}" From a560e4811d4e31da753846fae135ae00dc9189b0 Mon Sep 17 00:00:00 2001 From: Alex Wu Date: Mon, 11 Aug 2025 13:21:43 +0800 Subject: [PATCH 2/4] lint fix Signed-off-by: Alex Wu --- .../unit/core/image_spec/test_image_spec.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/flytekit/unit/core/image_spec/test_image_spec.py b/tests/flytekit/unit/core/image_spec/test_image_spec.py index 8d0c9e82d2..e742bcbd1e 100644 --- a/tests/flytekit/unit/core/image_spec/test_image_spec.py +++ b/tests/flytekit/unit/core/image_spec/test_image_spec.py @@ -362,13 +362,13 @@ def test_with_builder_options(): def test_noop_builder_updates_image_name_mapping(): from flytekit.image_spec.noop_builder import NoOpBuilder - + # Clear any existing mappings to ensure clean test state ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME.clear() - + # Register NoOpBuilder ImageBuildEngine.register("noop", NoOpBuilder()) - + # Create an image spec with NoOpBuilder expected_real_name = "localhost:30000/test_image:latest" image_spec = ImageSpec( @@ -376,20 +376,20 @@ def test_noop_builder_updates_image_name_mapping(): builder="noop", base_image=expected_real_name ) - + # Get the image name before building img_name = image_spec.image_name() - + # Build the image ImageBuildEngine.build(image_spec) - + # Verify that the mapping was created in _IMAGE_NAME_TO_REAL_NAME assert img_name in ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME - + # Verify the mapping value is correct actual_real_name = ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME[img_name] assert actual_real_name == expected_real_name - + # Clean up del ImageBuildEngine._REGISTRY["noop"] ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME.clear() From d4425dee2a125892d7c4eaafd2b75f082d90f770 Mon Sep 17 00:00:00 2001 From: Alex Wu Date: Wed, 13 Aug 2025 16:24:27 +0800 Subject: [PATCH 3/4] fix: fix variable names to make it more readable Signed-off-by: Alex Wu --- tests/flytekit/unit/core/image_spec/test_image_spec.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/flytekit/unit/core/image_spec/test_image_spec.py b/tests/flytekit/unit/core/image_spec/test_image_spec.py index e742bcbd1e..ef56d0e5ef 100644 --- a/tests/flytekit/unit/core/image_spec/test_image_spec.py +++ b/tests/flytekit/unit/core/image_spec/test_image_spec.py @@ -370,11 +370,11 @@ def test_noop_builder_updates_image_name_mapping(): ImageBuildEngine.register("noop", NoOpBuilder()) # Create an image spec with NoOpBuilder - expected_real_name = "localhost:30000/test_image:latest" + expected_image_name = "localhost:30000/test_image:latest" image_spec = ImageSpec( name="test_image", builder="noop", - base_image=expected_real_name + base_image=expected_image_name ) # Get the image name before building @@ -388,7 +388,7 @@ def test_noop_builder_updates_image_name_mapping(): # Verify the mapping value is correct actual_real_name = ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME[img_name] - assert actual_real_name == expected_real_name + assert actual_real_name == expected_image_name # Clean up del ImageBuildEngine._REGISTRY["noop"] From a2919cb789de0c85f62611efb4d4c0fc07e43b7e Mon Sep 17 00:00:00 2001 From: Alex Wu Date: Sat, 16 Aug 2025 08:03:30 +0800 Subject: [PATCH 4/4] fix: refactor doc string and test Signed-off-by: Alex Wu --- flytekit/image_spec/noop_builder.py | 6 +++--- tests/flytekit/unit/core/image_spec/test_image_spec.py | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/flytekit/image_spec/noop_builder.py b/flytekit/image_spec/noop_builder.py index 004556adad..ee2dc0c664 100644 --- a/flytekit/image_spec/noop_builder.py +++ b/flytekit/image_spec/noop_builder.py @@ -8,9 +8,9 @@ class NoOpBuilder(ImageSpecBuilder): def should_build(self, image_spec: ImageSpec) -> bool: """ - The build_image function of NoOpBuilder uses the image_spec name as defined by the user without - checking whether the image exists in the Docker registry. Therefore, the should_build function - should always return True to trigger the build_image function. + The build_image function of NoOpBuilder does not actually build a Docker image. + Since no Docker build process occurs, we do not need to check for Docker daemon + or existing images. Therefore, should_build should always return True. Args: image_spec (ImageSpec): Image specification diff --git a/tests/flytekit/unit/core/image_spec/test_image_spec.py b/tests/flytekit/unit/core/image_spec/test_image_spec.py index ef56d0e5ef..43905874e0 100644 --- a/tests/flytekit/unit/core/image_spec/test_image_spec.py +++ b/tests/flytekit/unit/core/image_spec/test_image_spec.py @@ -364,6 +364,7 @@ def test_noop_builder_updates_image_name_mapping(): from flytekit.image_spec.noop_builder import NoOpBuilder # Clear any existing mappings to ensure clean test state + ImageBuildEngine._REGISTRY.pop("noop", None) ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME.clear() # Register NoOpBuilder @@ -391,5 +392,5 @@ def test_noop_builder_updates_image_name_mapping(): assert actual_real_name == expected_image_name # Clean up - del ImageBuildEngine._REGISTRY["noop"] + ImageBuildEngine._REGISTRY.pop("noop", None) ImageBuildEngine._IMAGE_NAME_TO_REAL_NAME.clear()