Add error message when docker isn't running #2942
Conversation
Signed-off-by: Daniel Sola <[email protected]>
dansola
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
| @@ -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): | |||
| click.secho( | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: ..."
There was a problem hiding this comment.
Good idea, will do!
There was a problem hiding this comment.
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2942 +/- ##
===========================================
+ Coverage 79.32% 94.00% +14.68%
===========================================
Files 199 38 -161
Lines 20870 1768 -19102
Branches 2684 0 -2684
===========================================
- Hits 16555 1662 -14893
+ Misses 3566 106 -3460
+ Partials 749 0 -749 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| @@ -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): | |||
There was a problem hiding this comment.
nit: I'd like to see the original error still.
| 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): |
There was a problem hiding this comment.
Thanks! I'll make sure the original error is still shown.
Why are the changes needed?
When a user isn't running docker, it is unclear when they see the logs:
Ideally we'd have more clear logs when we can narrow down that the issue is that docker isn't running.
What changes were proposed in this pull request?
Added additional logging.
How was this patch tested?
Did not run docker, ran pyflyte run --remote with an ImangeSpec, observed the log message:
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link