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

[Core] check if docker is installed if user_docker is specified. #1145

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

ekzhu
Copy link
Collaborator

@ekzhu ekzhu commented Jan 4, 2024

Why are these changes needed?

Check if docker is installed if use_docker is specified. This is to adhere to the documentation of execute_code.

Related issue number

Address #498

Checks

@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (871e9e2) 30.74% compared to head (c7c6b4e) 40.35%.

Files Patch % Lines
autogen/code_utils.py 0.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1145      +/-   ##
==========================================
+ Coverage   30.74%   40.35%   +9.60%     
==========================================
  Files          30       30              
  Lines        4043     4047       +4     
  Branches      915      965      +50     
==========================================
+ Hits         1243     1633     +390     
+ Misses       2721     2302     -419     
- Partials       79      112      +33     
Flag Coverage Δ
unittests 40.27% <0.00%> (+9.58%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ekzhu ekzhu marked this pull request as ready for review January 4, 2024 22:34
@ekzhu ekzhu added code-execution execute generated code docker labels Jan 4, 2024
Copy link
Member

@afourney afourney left a comment

Choose a reason for hiding this comment

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

So, this PR addresses the same issue that was meant to be addressed by the lines 280 - 291 in your code below. Issues #172 and #200 are where they were introduced.

At the time, the warning was added because we wanted to highlight the risk, but not change default behavior due to the installed user base. Perhaps it is now time to change this, and @sonichi can weigh in on this.

But, if we decide now to throw an error, we should remove lines 280-291, and we should update the docstring to mention that the error is thrown, and also update the description of the use_docker field, which now reads:

 use_docker (Optional, list, str or bool): The docker image to use for code execution.
            If a list or a str of image name(s) is provided, the code will be executed in a docker container
            with the first image successfully pulled.
            If None, False or empty, the code will be executed in the current environment.
            Default is None, which will be converted into an empty list when docker package is available.
            Expected behaviour:
                - If `use_docker` is explicitly set to True and the docker package is available, the code will run in a Docker container.
                - If `use_docker` is explicitly set to True but the Docker package is missing, an error will be raised.
                - If `use_docker` is not set (i.e., left default to None) and the Docker package is not available, a warning will be displayed, but the code will run natively.
            If the code is executed in the current environment,
            the code must be trusted.

And I believe that description is correct (though the language about an "empty" list is confusing)

@ekzhu
Copy link
Collaborator Author

ekzhu commented Jan 5, 2024

Currently line 280-291 doesn't address the case when docker is not installed but use_docker is set to True. So technically the changes in this PR should address that case.

@afourney
Copy link
Member

afourney commented Jan 5, 2024

Currently line 280-291 doesn't address the case when docker is not installed but use_docker is set to True. So technically the changes in this PR should address that case.

Oh, ok. I see. Indeed you have provided a nicer error message.

Previously, If use docker was set to True, but Docker was not installed, then the code would throw an error here:

client = docker.from_env()
stating that docker was not defined.

@afourney afourney mentioned this pull request Jan 5, 2024
3 tasks
@ekzhu ekzhu added this pull request to the merge queue Jan 5, 2024
Merged via the queue into main with commit 55b03bf Jan 5, 2024
76 of 84 checks passed
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
@jackgerrits jackgerrits deleted the dockercheck branch September 25, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-execution execute generated code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants