-
Notifications
You must be signed in to change notification settings - Fork 408
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
Try/catch for Docker client #2513
Try/catch for Docker client #2513
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe changes across various files in the ZenML codebase focus on improving error handling when creating a Docker client from the environment. Specifically, if the process of initializing a Docker client fails due to a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
@coderabbitai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (6)
- src/zenml/container_registries/base_container_registry.py (2 hunks)
- src/zenml/image_builders/local_image_builder.py (2 hunks)
- src/zenml/orchestrators/local_docker/local_docker_orchestrator.py (1 hunks)
- src/zenml/service_connectors/docker_service_connector.py (1 hunks)
- src/zenml/services/container/container_service.py (3 hunks)
- src/zenml/utils/docker_utils.py (6 hunks)
Additional comments: 11
src/zenml/image_builders/local_image_builder.py (1)
- 110-115: The addition of a try/catch block around
DockerClient.from_env()
improves error handling by providing a more informative error message when the Docker daemon is not running. This aligns with the PR's objective to enhance user experience by addressing common issues encountered with Docker-based stacks. The implementation correctly catchesDockerException
and raises aRuntimeError
with a clear and actionable error message. This change is a significant improvement in terms of usability and error clarity.src/zenml/container_registries/base_container_registry.py (1)
- 146-151: The addition of a try/catch block around
DockerClient.from_env()
in thedocker_client
property is well-implemented. It enhances error handling by providing a user-friendly error message when the Docker daemon is not running. This change is consistent with the PR's goal to improve user experience by making error messages more informative and actionable. The implementation correctly catchesDockerException
and raises aRuntimeError
with a clear error message, which is a good practice for error handling in this context.src/zenml/orchestrators/local_docker/local_docker_orchestrator.py (1)
- 123-128: The implementation of the try/catch block around
DockerClient.from_env()
in theprepare_or_run_pipeline
function is appropriate and enhances error handling by providing a more informative error message when the Docker daemon is not running. This change aligns with the PR's objective to improve the user experience by making error messages clearer and more actionable. CatchingDockerException
and raising aRuntimeError
with a specific message is a good practice and improves the clarity of errors related to Docker daemon connectivity issues.src/zenml/service_connectors/docker_service_connector.py (1)
- 262-267: The addition of a try/catch block around
DockerClient.from_env()
in the_connect_to_resource
method is a positive change that enhances error handling by providing a clearer error message when the Docker daemon is not running. This change is consistent with the PR's goal to improve user experience by making error messages more informative and actionable. The implementation correctly catchesDockerException
and raises aRuntimeError
with a specific message, which is a good practice for error handling in this context.src/zenml/utils/docker_utils.py (5)
- 231-236: The addition of try/catch blocks around
DockerClient.from_env()
calls in thebuild_image
function enhances error handling by providing a more informative error message when the Docker daemon is not running. This change aligns with the PR's objective to improve user experience by making error messages clearer and more actionable. The implementation correctly catchesDockerException
and raises aRuntimeError
with a specific message, which is a good practice for error handling in this context.- 268-273: The addition of a try/catch block around
DockerClient.from_env()
in thepush_image
function is correctly implemented and enhances error handling by providing a clearer error message when the Docker daemon is not running. This change is consistent with the PR's goal to improve user experience by making error messages more informative and actionable. CatchingDockerException
and raising aRuntimeError
with a specific message is a good practice and improves the clarity of errors related to Docker daemon connectivity issues.- 298-303: The implementation of the try/catch block around
DockerClient.from_env()
in thetag_image
function is appropriate and enhances error handling by providing a more informative error message when the Docker daemon is not running. This change aligns with the PR's objective to improve the user experience by making error messages clearer and more actionable. CatchingDockerException
and raising aRuntimeError
with a specific message is a good practice and improves the clarity of errors related to Docker daemon connectivity issues.- 318-323: The addition of a try/catch block around
DockerClient.from_env()
in theget_image_digest
function is well-implemented. It enhances error handling by providing a user-friendly error message when the Docker daemon is not running. This change is consistent with the PR's goal to improve user experience by making error messages more informative and actionable. The implementation correctly catchesDockerException
and raises aRuntimeError
with a clear error message, which is a good practice for error handling in this context.- 347-352: The implementation of the try/catch block around
DockerClient.from_env()
in theis_local_image
function is appropriate and enhances error handling by providing a more informative error message when the Docker daemon is not running. This change aligns with the PR's objective to improve the user experience by making error messages clearer and more actionable. CatchingDockerException
and raising aRuntimeError
with a specific message is a good practice and improves the clarity of errors related to Docker daemon connectivity issues.src/zenml/services/container/container_service.py (2)
- 26-26: The import of
DockerException
is correctly placed and follows Python's convention of grouping imports from the same library together. This is a good practice as it keeps the import statements organized and readable.- 314-316: Refactoring the setting of
ENV_ZENML_CONFIG_PATH
is done correctly and maintains the readability and maintainability of the code. This change does not directly relate to the handling ofDockerException
, but it's a good practice to keep environment variable settings clear and concise. No further action is needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for contributing! I left one suggestion. Also the docstring are impacted now - my suggestion will also simplify resolution for this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go, once CI passes
thanks for merging @strickvl & reviews @avishniakov ! |
* update TOC (zenml-io#2406) * Add try/except to DockerClient from env * Move generation of Docker client from environment to separate utility function --------- Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com>
Describe changes
When your Docker daemon is not running in a Docker based stack, users get somewhat obscure errors like
DockerException: Error while fetching server API version: ('Connection aborted.', FileNotFoundError(2, 'No such file or directory'))
.This PR wraps all
DockerClient.from_env()
calls where no try/catch construct is present with one, raising a more user-friendly error as to what is wrong and could be done to help resolve the issue.Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes
Summary by CodeRabbit