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.

Sign up for GitHub

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

Add error message when docker isn't running #2942

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add error message when docker isn't running #2942

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 11 additions & 1 deletion flytekit/image_spec/image_spec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ def exist(self) -> Optional[bool]:
Return None if failed to check if the image exists due to the permission issue or other reasons.
"""
import docker
from docker.errors import APIError, ImageNotFound
from docker.errors import APIError, DockerException, ImageNotFound

try:
client = docker.from_env()
Expand Down Expand Up @@ -278,6 +278,16 @@ def exist(self) -> Optional[bool]:
f" pip install --upgrade docker"
)

if isinstance(e, DockerException) and "Error while fetching server API version" in str(e):
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd like to see the original error still.

Suggested change
if isinstance(e, DockerException) and "Error while fetching server API version" in str(e):
click.secho(f"Failed to check if the image exists with error:\n {e}", fg="red")
if isinstance(e, DockerException) and "Error while fetching server API version" in str(e):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'll make sure the original error is still shown.

click.secho(
Copy link
Member

Choose a reason for hiding this comment

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

If the image exists in your ghcr.io registry and docker is not running, then that should still work. This message tells a user to install Docker, but it is not strictly required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. But I don't think we want to hide the fact that docker should generally be running. If it's not clear, the user will eventually run into images that don't exist in their registry and they will get image pull failures they don't understand. This happened today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe I should change the "error message" wording in the PR title, but this change doesn't block images being found and used if the docker is off but the image exists

Copy link
Member

Choose a reason for hiding this comment

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

Let's write out these details in the message. Something like: "Docker is not running and flytekit will assume that the image exists in your registry. If you want to build the image, please install docker: ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I was going to change the click.secho(f"Flytekit assumes the image {img_name} already exists.", fg="blue") in should_build(), but that could occur even if docker is running, for example an APIError that returns None.

The current suggestion actually mentions that docker isn't running if it catches the exact error docker throws, then the Flytekit assumes the image ghcr.io/dansola/test-image:ORI1BZVtVKVludLUJsq_Rg already exists. log still comes up after.

Do you think it's not clear that those two are associated? Happy to keep it general and just toss in the mention of docker in should_build(), but I just don't want to sound that alarm if it isn't the case.

"Unable to connect to Docker daemon. Please ensure Docker is installed and running.\n"
"You can start Docker by:\n"
" - Windows/Mac: Start the Docker Desktop application\n"
" - Linux: Run 'sudo systemctl start docker'",
fg="red",
)
return None

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

Expand Down
Loading