Fix check_can_use_docker_or_throw

[davorrunje]

Why are these changes needed?

If the code is already running in a docker container, check_can_use_docker_or_throw should raise an exception.

Related issue number

Closes #1395

Checks

• I've included any doc changes needed for https://microsoft.github.io/autogen/. See https://microsoft.github.io/autogen/docs/Contribute#documentation to build and test documentation locally.
• I've added tests (if relevant) corresponding to the changes introduced in this PR.
• I've made sure all auto checks have passed.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bcfd770) 32.48% compared to head (536a28a) 45.25%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1396       +/-   ##
===========================================
+ Coverage   32.48%   45.25%   +12.77%     
===========================================
  Files          41       41               
  Lines        4907     4910        +3     
  Branches     1120     1188       +68     
===========================================
+ Hits         1594     2222      +628     
+ Misses       3187     2504      -683     
- Partials      126      184       +58     

Flag	Coverage Δ
unittests	45.19% <100.00%> (+12.75%)	⬆️

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.

[olgavrou]

The idea was that if you are running in a docker container then we don't bug you to do anything about code execution (since it defaults to ON). So I don't think it would be ideal to have users run in docker and have them change the default setting too. We could rename the function name?

[davorrunje]

The idea was that if you are running in a docker container then we don't bug you to do anything about code execution (since it defaults to ON). So I don't think it would be ideal to have users run in docker and have them change the default setting too. We could rename the function name?

I am not sure that I understand what you mean so please let me check if my understanding is correct. The function check_can_use_docker_or_throw is used in code execution functions to determine if docker can be used for execution. If not, it should raise an exception. If you are already running in a container, you cannot use docker for execution. Hence, it should raise an exception.

If my understanding is incorrect, then the name is indeed misleading and should be changed.

[olgavrou]

The idea was that if you are running in a docker container then we don't bug you to do anything about code execution (since it defaults to ON). So I don't think it would be ideal to have users run in docker and have them change the default setting too. We could rename the function name?

I am not sure that I understand what you mean so please let me check if my understanding is correct. The function check_can_use_docker_or_throw is used in code execution functions to determine if docker can be used for execution. If not, it should raise an exception. If you are already running in a container, you cannot use docker for execution. Hence, it should raise an exception.

If my understanding is incorrect, then the name is indeed misleading and should be changed.

If you are already running inside of a docker container then you could use that container for execution. 'use' is applied liberally here. Happy to change the function name to something else though to make it more clear. Maybe check_docker_available_or_throw?

[davorrunje]

I am a bit confused about the intended purpose of this. I was under impression that execution in a docker container is primarily motivated by security concerns of running untrusted code in the same environment where we might have sensitive information. If we are running autogen already in a docker container, that means that API keys are probably contained in environment variables, files or in some way accessible within that container. In such case, I would believe that setting running in docker would execute untrusted code in a separate container, while it would actually run in the same one. As a user, I would expect that not to be considered "using docker" and would expect an exception to be raised and force me to explicitly pass a configuration for which I know there is a security risk involved. I only discovered this behavior by writing missing tests, I did not expect such behavior.

[olgavrou]

I am a bit confused about the intended purpose of this. I was under impression that execution in a docker container is primarily motivated by security concerns of running untrusted code in the same environment where we might have sensitive information. If we are running autogen already in a docker container, that means that API keys are probably contained in environment variables, files or in some way accessible within that container. In such case, I would believe that setting running in docker would execute untrusted code in a separate container, while it would actually run in the same one. As a user, I would expect that not to be considered "using docker" and would expect an exception to be raised and force me to explicitly pass a configuration for which I know there is a security risk involved. I only discovered this behavior by writing missing tests, I did not expect such behavior.

yeah ok good point

[davorrunje]

@olgavrou btw, feel free to close this PR and the issue if you think it is not relevant.