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 4 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
22 changes: 15 additions & 7 deletions flytekit/image_spec/image_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ def is_container(self) -> bool:
def exist(self) -> 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,10 @@ def exist(self) -> bool:
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")
click.secho(f"Flytekit assumes the image {self.image_name()} already exists.", fg="blue")
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

done

return None
except ImageNotFound:
return False
except Exception as e:
Expand Down Expand Up @@ -153,9 +158,9 @@ def exist(self) -> bool:
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")
click.secho(f"Flytekit assumes the image {self.image_name()} already exists.", fg="blue")
Copy link
Member

Choose a reason for hiding this comment

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

I think this print statement belongs in should_build instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense, I updated it, thanks!

return None

def __hash__(self):
return hash(asdict(self).__str__())
Expand Down Expand Up @@ -240,14 +245,17 @@ def should_build(self, image_spec: ImageSpec) -> bool:
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")
click.secho(f"Overwriting existing image {img_name}.", fg="blue")
return True

click.secho(f"Image {img_name} found. Skip building.", fg="blue")
if exist is True:
click.secho(f"Image {img_name} found. Skip building.", 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 @@ def build_image(self, image_spec: ImageSpec):
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(
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 @@ def execute_command(command: str):

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

Choose a reason for hiding this comment

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

Change this to raise assertionerror
Or runtime or specific error

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