Update tests for code_utils and use ThreadPoolExecutor for code execution in windows (#1472)

* threadpoolexecutor

* update tests

* update tests

* fix tests on windows

* fix test

* update tests

* update tests

* update tests

* update tests

* update tests

* update tests

* fix build.yml

* build yaml

* test

* use skip-docker to explicitly skipping docker tests

* update tests

* contribution doc update

* Update website/docs/Contribute.md

Co-authored-by: Chi Wang <[email protected]>

* update doc

---------

Co-authored-by: Chi Wang <[email protected]>

Author: ekzhu

.github/workflows/build.yml

@@ -20,17 +20,15 @@ concurrency:
   group: ${{ github.workflow }}-${{ github.ref }}-${{ github.head_ref }}
   cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}
 permissions: {}
-    # actions: read
-  # checks: read
-  # contents: read
-  # deployments: read
 jobs:
   build:
     runs-on: ${{ matrix.os }}
+    env:
+      AUTOGEN_USE_DOCKER: ${{ matrix.os != 'ubuntu-latest'  && 'False' }}
     strategy:
       fail-fast: false
       matrix:
-        os: [ubuntu-latest, macos-latest, windows-2019]
+        os: [ubuntu-latest, macos-latest, windows-latest]
         python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"]
     steps:
       - uses: actions/checkout@v3
@@ -44,16 +42,14 @@ jobs:
           pip install -e .
           python -c "import autogen"
           pip install pytest mock
-      - name: Set AUTOGEN_USE_DOCKER based on OS
-        shell: bash
-        run: |
-          if [[ ${{ matrix.os }} != ubuntu-latest ]]; then
-            echo "AUTOGEN_USE_DOCKER=False" >> $GITHUB_ENV
-          fi
-      - name: Test with pytest
-        if: matrix.python-version != '3.10'
+      - name: Test with pytest skipping openai tests
+        if: matrix.python-version != '3.10' && matrix.os == 'ubuntu-latest'
         run: |
           pytest test --skip-openai
+      - name: Test with pytest skipping openai and docker tests
+        if: matrix.python-version != '3.10' && matrix.os != 'ubuntu-latest'
+        run: |
+          pytest test --skip-openai --skip-docker
       - name: Coverage
         if: matrix.python-version == '3.10'
         run: |

autogen/code_utils.py

@@ -12,10 +12,7 @@
 
 from autogen import oai
 
-try:
-    import docker
-except ImportError:
-    docker = None
+import docker
 
 SENTINEL = object()
 DEFAULT_MODEL = "gpt-4"
@@ -232,8 +229,6 @@ def is_docker_running():
     Returns:
         bool: True if docker is running; False otherwise.
     """
-    if docker is None:
-        return False
     try:
         client = docker.from_env()
         client.ping()
@@ -394,29 +389,20 @@ def execute_code(
             sys.executable if lang.startswith("python") else _cmd(lang),
             f".\\{filename}" if WIN32 else filename,
         ]
-        if WIN32:
-            logger.warning("SIGALRM is not supported on Windows. No timeout will be enforced.")
-            result = subprocess.run(
+        with ThreadPoolExecutor(max_workers=1) as executor:
+            future = executor.submit(
+                subprocess.run,
                 cmd,
                 cwd=work_dir,
                 capture_output=True,
                 text=True,
             )
-        else:
-            with ThreadPoolExecutor(max_workers=1) as executor:
-                future = executor.submit(
-                    subprocess.run,
-                    cmd,
-                    cwd=work_dir,
-                    capture_output=True,
-                    text=True,
-                )
-                try:
-                    result = future.result(timeout=timeout)
-                except TimeoutError:
-                    if original_filename is None:
-                        os.remove(filepath)
-                    return 1, TIMEOUT_MSG, None
+            try:
+                result = future.result(timeout=timeout)
+            except TimeoutError:
+                if original_filename is None:
+                    os.remove(filepath)
+                return 1, TIMEOUT_MSG, None
         if original_filename is None:
             os.remove(filepath)
         if result.returncode:

test/conftest.py

@@ -2,19 +2,23 @@
 
 skip_openai = False
 skip_redis = False
+skip_docker = False
 
 
-# Registers command-line option '--skip-openai' and '--skip-redis' via pytest hook.
+# Registers command-line options like '--skip-openai' and '--skip-redis' via pytest hook.
 # When these flags are set, it indicates that tests requiring OpenAI or Redis (respectively) should be skipped.
 def pytest_addoption(parser):
     parser.addoption("--skip-openai", action="store_true", help="Skip all tests that require openai")
     parser.addoption("--skip-redis", action="store_true", help="Skip all tests that require redis")
+    parser.addoption("--skip-docker", action="store_true", help="Skip all tests that require docker")
 
 
-# pytest hook implementation extracting the '--skip-openai' and '--skip-redis' command line arg and exposing it globally
+# pytest hook implementation extracting command line args and exposing it globally
 @pytest.hookimpl(tryfirst=True)
 def pytest_configure(config):
     global skip_openai
     skip_openai = config.getoption("--skip-openai", False)
     global skip_redis
     skip_redis = config.getoption("--skip-redis", False)
+    global skip_docker
+    skip_docker = config.getoption("--skip-docker", False)

test/test_code.py

@@ -1,15 +1,12 @@
-import importlib.metadata
 import os
-import sys
+import tempfile
 import unittest
 
 import pytest
 
 import autogen
 from autogen.code_utils import (
-    PATH_SEPARATOR,
     UNKNOWN,
-    WIN32,
     content_str,
     execute_code,
     extract_code,
@@ -21,27 +18,13 @@
     decide_use_docker,
     check_can_use_docker_or_throw,
 )
+from conftest import skip_docker
 
 KEY_LOC = "notebook"
 OAI_CONFIG_LIST = "OAI_CONFIG_LIST"
 here = os.path.abspath(os.path.dirname(__file__))
 
 
-def is_package_installed(package_name):
-    """Check if a package is installed. This is a preferred way to check if a
-    package is installed or not than doing a try-catch around an import
-    because it avoids name conflict with local modules and
-    code execution in the imported module."""
-    try:
-        importlib.metadata.version(package_name)
-        return True
-    except importlib.metadata.PackageNotFoundError:
-        return False
-
-
-docker_package_installed = is_package_installed("docker")
-
-
 # def test_find_code():
 #     try:
 #         import openai
@@ -313,86 +296,119 @@ def scrape(url):
     assert len(codeblocks) == 1 and codeblocks[0] == ("", "source setup.sh")
 
 
-# skip if os is windows
 @pytest.mark.skipif(
-    sys.platform in ["win32"] or (not is_docker_running() and not in_docker_container()), reason="docker is not running"
+    skip_docker or not is_docker_running(),
+    reason="docker is not running or requested to skip docker tests",
 )
-def test_execute_code(use_docker=None):
-    try:
-        import docker
-    except ImportError as exc:
-        print(exc)
-        docker = None
-    if use_docker is None:
-        use_docker = docker is not None
-    exit_code, msg, image = execute_code("print('hello world')", filename="tmp/codetest.py", use_docker=use_docker)
-    assert exit_code == 0 and msg == "hello world\n", msg
-    # read a file
-    print(execute_code("with open('tmp/codetest.py', 'r') as f: a=f.read()", use_docker=use_docker))
-    # create a file
-    exit_code, msg, image = execute_code(
-        "with open('tmp/codetest.py', 'w') as f: f.write('b=1')",
-        work_dir=f"{here}/my_tmp",
-        filename="tmp2/codetest.py",
-        use_docker=use_docker,
-    )
-    assert exit_code and (
-        'File "tmp2/codetest.py"'.replace("/", PATH_SEPARATOR) in msg
-        or 'File ".\\tmp2/codetest.py' in msg  # py3.8 + win32
-    ), msg
-    print(
-        execute_code(
-            "with open('tmp/codetest.py', 'w') as f: f.write('b=1')", work_dir=f"{here}/my_tmp", use_docker=use_docker
+def test_execute_code(use_docker=True):
+    # Test execute code and save the code to a file.
+    with tempfile.TemporaryDirectory() as tempdir:
+        filename = "temp_file_with_code.py"
+
+        # execute code and save the code to a file.
+        exit_code, msg, image = execute_code(
+            "print('hello world')",
+            filename=filename,
+            work_dir=tempdir,
+            use_docker=use_docker,
         )
-    )
-    # execute code in a file
-    print(execute_code(filename="tmp/codetest.py", use_docker=use_docker))
-    print(execute_code("python tmp/codetest.py", lang="sh", use_docker=use_docker))
-    # execute code for assertion error
-    exit_code, msg, image = execute_code("assert 1==2", use_docker=use_docker)
-    assert exit_code, msg
-    assert 'File ""' in msg or 'File ".\\"' in msg  # py3.8 + win32
-    # execute code which takes a long time
-    exit_code, error, image = execute_code("import time; time.sleep(2)", timeout=1, use_docker=use_docker)
-    assert exit_code and error == "Timeout" or WIN32
-    assert isinstance(image, str) or docker is None or os.path.exists("/.dockerenv") or use_docker is False
+        assert exit_code == 0 and msg == "hello world\n", msg
+
+        # read the file just saved
+        exit_code, msg, image = execute_code(
+            f"with open('{filename}', 'rt') as f: print(f.read())",
+            use_docker=use_docker,
+            work_dir=tempdir,
+        )
+        assert exit_code == 0 and "print('hello world')" in msg, msg
+
+        # execute code in a file
+        exit_code, msg, image = execute_code(
+            filename=filename,
+            use_docker=use_docker,
+            work_dir=tempdir,
+        )
+        assert exit_code == 0 and msg == "hello world\n", msg
+
+        # execute code in a file using shell command directly
+        exit_code, msg, image = execute_code(
+            f"python {filename}",
+            lang="sh",
+            use_docker=use_docker,
+            work_dir=tempdir,
+        )
+        assert exit_code == 0 and msg == "hello world\n", msg
+
+    with tempfile.TemporaryDirectory() as tempdir:
+        # execute code for assertion error
+        exit_code, msg, image = execute_code(
+            "assert 1==2",
+            use_docker=use_docker,
+            work_dir=tempdir,
+        )
+        assert exit_code, msg
+        assert "AssertionError" in msg
+        assert 'File "' in msg or 'File ".\\"' in msg  # py3.8 + win32
+
+    with tempfile.TemporaryDirectory() as tempdir:
+        # execute code which takes a long time
+        exit_code, error, image = execute_code(
+            "import time; time.sleep(2)",
+            timeout=1,
+            use_docker=use_docker,
+            work_dir=tempdir,
+        )
+        assert exit_code and error == "Timeout"
+        if use_docker is True:
+            assert isinstance(image, str)
 
 
 @pytest.mark.skipif(
-    sys.platform in ["win32"] or (not is_docker_running()) or in_docker_container(),
-    reason="docker is not running or in docker container already",
+    skip_docker or not is_docker_running(),
+    reason="docker is not running or requested to skip docker tests",
 )
 def test_execute_code_with_custom_filename_on_docker():
-    exit_code, msg, image = execute_code("print('hello world')", filename="tmp/codetest.py", use_docker=True)
-    assert exit_code == 0 and msg == "hello world\n", msg
-    assert image == "python:tmp_codetest.py"
+    with tempfile.TemporaryDirectory() as tempdir:
+        filename = "codetest.py"
+        exit_code, msg, image = execute_code(
+            "print('hello world')",
+            filename=filename,
+            use_docker=True,
+            work_dir=tempdir,
+        )
+        assert exit_code == 0 and msg == "hello world\n", msg
+        assert image == "python:codetest.py"
 
 
 @pytest.mark.skipif(
-    sys.platform in ["win32"] or (not is_docker_running()) or in_docker_container(),
-    reason="docker is not running or in docker container already",
+    skip_docker or not is_docker_running(),
+    reason="docker is not running or requested to skip docker tests",
 )
 def test_execute_code_with_misformed_filename_on_docker():
-    exit_code, msg, image = execute_code(
-        "print('hello world')", filename="tmp/codetest.py (some extra information)", use_docker=True
-    )
-    assert exit_code == 0 and msg == "hello world\n", msg
-    assert image == "python:tmp_codetest.py__some_extra_information_"
+    with tempfile.TemporaryDirectory() as tempdir:
+        filename = "codetest.py (some extra information)"
+        exit_code, msg, image = execute_code(
+            "print('hello world')",
+            filename=filename,
+            use_docker=True,
+            work_dir=tempdir,
+        )
+        assert exit_code == 0 and msg == "hello world\n", msg
+        assert image == "python:codetest.py__some_extra_information_"
 
 
 def test_execute_code_raises_when_code_and_filename_are_both_none():
     with pytest.raises(AssertionError):
         execute_code(code=None, filename=None)
 
 
-def test_execute_code_nodocker():
+def test_execute_code_no_docker():
     test_execute_code(use_docker=False)
 
 
-def test_execute_code_no_docker():
+def test_execute_code_timeout_no_docker():
     exit_code, error, image = execute_code("import time; time.sleep(2)", timeout=1, use_docker=False)
-    if sys.platform != "win32":
-        assert exit_code and error == "Timeout"
+    assert exit_code and error == "Timeout"
     assert image is None
 
 

website/docs/Contribute.md

@@ -155,23 +155,38 @@ Tests are automatically run via GitHub actions. There are two workflows:
 
 The first workflow is required to pass for all PRs (and it doesn't do any OpenAI calls). The second workflow is required for changes that affect the OpenAI tests (and does actually call LLM). The second workflow requires approval to run. When writing tests that require OpenAI calls, please use [`pytest.mark.skipif`](https://github.com/microsoft/autogen/blob/b1adac515931bf236ac59224269eeec683a162ba/test/oai/test_client.py#L19) to make them run in only when `openai` package is installed. If additional dependency for this test is required, install the dependency in the corresponding python version in [openai.yml](https://github.com/microsoft/autogen/blob/main/.github/workflows/openai.yml).
 
-#### Run non-OpenAI tests
+Make sure all tests pass, this is required for [build.yml](https://github.com/microsoft/autogen/blob/main/.github/workflows/build.yml) checks to pass
 
-To run the subset of the tests not depending on `openai` (and not calling LLMs)):
+#### Running tests locally
 
-- Install pytest:
+To run tests, install the [test] option:
 
-``` code
-pip install pytest
+```bash
+pip install -e."[test]"
 ```
 
-- Run the tests from the `test` folder using the `--skip-openai` flag.
+Then you can run the tests from the `test` folder using the following command:
 
-``` code
-pytest test --skip-openai
+```bash
+pytest test
 ```
 
-- Make sure all tests pass, this is required for [build.yml](https://github.com/microsoft/autogen/blob/main/.github/workflows/build.yml) checks to pass
+Tests for the `autogen.agentchat.contrib` module may be skipped automatically if the
+required dependencies are not installed. Please consult the documentation for
+each contrib module to see what dependencies are required.
+
+#### Skip flags for tests
+
+- `--skip-openai` for skipping tests that require access to OpenAI services.
+- `--skip-docker` for skipping tests that explicitly use docker
+- `--skip-redis` for skipping tests that require a Redis server
+
+For example, the following command will skip tests that require access to
+OpenAI and docker services:
+
+```bash
+pytest test --skip-openai --skip-docker
+```
 
 ### Coverage