Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message for ImageSpec #2498

Merged
merged 5 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions flytekit/image_spec/image_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,11 @@
return os.environ.get(_F_IMG_ID) == self.image_name()
return True

def exist(self) -> bool:
def exist(self) -> Optional[bool]:
"""
Check if the image exists in the registry.
Return True if the image exists in the registry, False otherwise.
Return None if failed to check if the image exists due to the permission issue or other reasons.
Copy link
Member

Choose a reason for hiding this comment

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

That Python typing for exist needs to be updated to include None.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

"""
import docker
from docker.errors import APIError, ImageNotFound
Expand All @@ -121,7 +123,9 @@
except APIError as e:
if e.response.status_code == 404:
return False
return True

click.secho(f"Failed to check if the image exists with error:\n {e}", fg="red")
return None
except ImageNotFound:
return False
except Exception as e:
Expand Down Expand Up @@ -153,9 +157,8 @@
f" pip install --upgrade docker"
)

click.secho(f"Failed to check if the image exists with error : {e}", fg="red")
click.secho("Flytekit assumes that the image already exists.", fg="blue")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we ensure we handle runtime error nicely in the clean exception in pyflyte

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

return True
click.secho(f"Failed to check if the image exists with error:\n {e}", fg="red")
return None

Check warning on line 161 in flytekit/image_spec/image_spec.py

View check run for this annotation

Codecov / codecov/patch

flytekit/image_spec/image_spec.py#L160-L161

Added lines #L160 - L161 were not covered by tests

def __hash__(self):
return hash(asdict(self).__str__())
Expand Down Expand Up @@ -240,14 +243,17 @@
True if the image should be built, otherwise it returns False.
"""
img_name = image_spec.image_name()
if not image_spec.exist():
exist = image_spec.exist()
if exist is False:
click.secho(f"Image {img_name} not found. building...", fg="blue")
return True
if image_spec._is_force_push:
click.secho(f"Image {img_name} found but overwriting existing image.", fg="blue")
return True

click.secho(f"Image {img_name} found. Skip building.", fg="blue")
elif exist is True:
if image_spec._is_force_push:
click.secho(f"Overwriting existing image {img_name}.", fg="blue")
return True
click.secho(f"Image {img_name} found. Skip building.", fg="blue")

Check warning on line 254 in flytekit/image_spec/image_spec.py

View check run for this annotation

Codecov / codecov/patch

flytekit/image_spec/image_spec.py#L252-L254

Added lines #L252 - L254 were not covered by tests
else:
click.secho(f"Flytekit assumes the image {img_name} already exists.", fg="blue")
return False


Expand Down
16 changes: 14 additions & 2 deletions plugins/flytekit-envd/flytekitplugins/envd/image_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
import pathlib
import shutil
import subprocess
from dataclasses import asdict
from importlib import metadata

import click
from packaging.version import Version
from rich import print
from rich.pretty import Pretty

from flytekit.configuration import DefaultImages
from flytekit.core import context_manager
Expand Down Expand Up @@ -34,7 +37,16 @@
else:
build_command += f" --tag {image_spec.image_name()}"
envd_context_switch(image_spec.registry)
execute_command(build_command)
try:
execute_command(build_command)
except Exception as e:
click.secho("❌ Failed to build image spec:", fg="red")
print(
Pretty(

Check warning on line 45 in plugins/flytekit-envd/flytekitplugins/envd/image_builder.py

View check run for this annotation

Codecov / codecov/patch

plugins/flytekit-envd/flytekitplugins/envd/image_builder.py#L43-L45

Added lines #L43 - L45 were not covered by tests
asdict(image_spec, dict_factory=lambda x: {k: v for (k, v) in x if v is not None}), indent_size=2
)
)
raise e from None


def envd_context_switch(registry: str):
Expand Down Expand Up @@ -68,7 +80,7 @@

if p.returncode != 0:
_, stderr = p.communicate()
raise Exception(f"failed to run command {command} with error {stderr}")
raise RuntimeError(f"failed to run command {command} with error:\n {stderr.decode()}")

return result

Expand Down
2 changes: 1 addition & 1 deletion tests/flytekit/unit/core/image_spec/test_image_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def test_image_spec(mock_image_spec_builder):

assert "dummy" in ImageBuildEngine._REGISTRY
assert calculate_hash_from_image_spec(image_spec) == tag
assert image_spec.exist() is True
assert image_spec.exist() is None

# Remove the dummy builder, and build the image again
# The image has already been built, so it shouldn't fail.
Expand Down
Loading