Refactor system tests to use the new FTP server and to run in pytest

.github/workflows/main.yml

@@ -143,30 +143,6 @@ jobs:
     timeout-minutes: 30
     steps:
       - uses: actions/checkout@v3
-
-      - uses: docker/setup-buildx-action@v3
-      # pre-build and cache the postgres container as installing python3 takes a while, doesn't push
-      - name: Cache Docker layers
-        uses: actions/cache@v3
-        with:
-          path: /tmp/.buildx-cache
-          key: ${{ runner.os }}-buildx-${{ github.sha }}
-          restore-keys: |
-            ${{ runner.os }}-buildx-
-      - uses: docker/build-push-action@v5
-        with:
-          context: .
-          file: docker/postgres/Dockerfile
-          cache-from: type=local,src=/tmp/.buildx-cache
-          cache-to: type=local,dest=/tmp/.buildx-cache-new,mode=max
-      - # Temp fix
-        # https://github.com/docker/build-push-action/issues/252
-        # https://github.com/moby/buildkit/issues/1896
-        name: Move cache
-        run: |
-          rm -rf /tmp/.buildx-cache
-          mv /tmp/.buildx-cache-new /tmp/.buildx-cache
-
       - name: Init Python
         uses: actions/setup-python@v4
         with:
@@ -187,7 +163,6 @@ jobs:
     timeout-minutes: 10
     steps:
       - uses: actions/checkout@v3
-
       - name: Init Python
         uses: actions/setup-python@v4
         with:
@@ -209,25 +184,21 @@ jobs:
     timeout-minutes: 30
     steps:
       - uses: actions/checkout@v3
-      - name: Prune docker volumes
-        # seems like we're getting an error from ftp-server exiting with zero status code
-        # this is a workaround that resolves it locally 🤞
-        run: |
-          docker volume prune --force
-
       - uses: docker/setup-buildx-action@v3
-      # pre-build and cache the postgres container as installing python3 takes a while, doesn't push
+      # pre-build and cache the orthanc-anon image: installing python3 takes a while, doesn't push
       - name: Cache Docker layers
-        uses: actions/cache@v3
+        uses: actions/cache@v4
         with:
           path: /tmp/.buildx-cache
-          key: ${{ runner.os }}-buildx-${{ github.sha }}
+          key: ${{ runner.os }}-buildx-${{ github.ref }}-${{ github.sha }}
           restore-keys: |
+            ${{ runner.os }}-buildx-${{ github.ref }}-
             ${{ runner.os }}-buildx-
+          save-always: true
       - uses: docker/build-push-action@v5
         with:
           context: .
-          file: docker/postgres/Dockerfile
+          file: docker/orthanc-anon/Dockerfile
           cache-from: type=local,src=/tmp/.buildx-cache
           cache-to: type=local,dest=/tmp/.buildx-cache-new,mode=max
       - # Temp fix
@@ -244,6 +215,14 @@ jobs:
           python-version: 3.10.6
           cache: "pip"
 
+      - name: Install Python dependencies
+        # The -e option is required here due to the way the
+        # code determines the export directory. See issue #318.
+        run: |
+          pip install -e pixl_core/
+          pip install -e cli/[test]
+          pip install -e pytest-pixl/
+
       - name: Build test services
         working-directory: test
         run: |
@@ -257,3 +236,19 @@ jobs:
         working-directory: test
         run: |
           ./run-system-test.sh
+          echo FINISHED SYSTEM TEST SCRIPT
+
+      - name: Dump ehr-api docker logs for debugging
+        if: ${{ failure() }}
+        run: |
+          docker logs -t system-test-ehr-api-1 2>&1
+
+      - name: Dump orthanc-anon docker logs for debugging
+        if: ${{ failure() }}
+        run: |
+          docker logs -t system-test-orthanc-anon-1 2>&1
+
+      - name: Dump imaging-api docker logs for debugging
+        if: ${{ failure() }}
+        run: |
+          docker logs -t system-test-imaging-api-1 2>&1

cli/src/pixl_cli/main.py

@@ -125,7 +125,10 @@ def extract_radiology_reports(parquet_dir: Path) -> None:
 
     success_code = 200
     if response.status_code != success_code:
-        msg = f"Failed to run export-patient-data due to: {response.text}"
+        msg = (
+            f"Failed to run export-patient-data due to: "
+            f"error code {response.status_code} {response.text}"
+        )
         raise RuntimeError(msg)
 
 

docker-compose.yml

@@ -153,6 +153,9 @@ services:
       - ${PWD}/orthanc/orthanc-anon/config:/run/secrets:ro
     networks:
       - pixl-net
+    # needed for same reason as ehr-api
+    extra_hosts:
+      - "host.docker.internal:host-gateway"
     depends_on:
       postgres:
         condition: service_healthy
@@ -253,6 +256,10 @@ services:
       retries: 5
     networks:
       - pixl-net
+    # needed for testing under GHA (linux), so this container
+    # can reach the test FTP server running on the docker host
+    extra_hosts:
+      - "host.docker.internal:host-gateway"
     volumes:
       - ${PWD}/exports:/run/exports
 

docker/orthanc-anon/Dockerfile

@@ -13,6 +13,39 @@
 #  limitations under the License.
 FROM osimis/orthanc:22.9.0-full-stable
 SHELL ["/bin/bash", "-o", "pipefail", "-e", "-u", "-x", "-c"]
+# need some packages for building python 3.9.13
+RUN export DEBIAN_FRONTEND=noninteractive \
+    && apt update \
+    && apt install --yes --no-install-recommends \
+           build-essential \
+           libbz2-dev \
+           libffi-dev \
+           libgdbm-dev \
+           libncurses5-dev \
+           libnss3-dev \
+           libreadline-dev \
+           libsqlite3-dev \
+           libssl-dev \
+           zlib1g-dev \
+    && apt-get autoremove \
+    && apt-get clean \
+    && rm -rf /var/lib/apt/lists/*
+
+# Default for Debian Bullseye (11) is python 3.9.2, which has a bug in ftplib that is
+# affecting us.
+# See https://bugs.python.org/issue43285 for more details. It only happens in the system
+# test, when docker is using NAT to implement host.docker.internal
+# Install 3.9.13 alongside the existing version (removing the original python beforehand
+# seems to break orthanc)
+ADD https://www.python.org/ftp/python/3.9.13/Python-3.9.13.tgz /python3913/
+RUN cd /python3913 \
+    && tar -xvf Python-3.9.13.tgz \
+    && cd Python-3.9.13 \
+    && ./configure --enable-optimizations \
+    && make -j `nproc` \
+    && make install \
+    && make clean \
+    && rm -fr /python3913
 
 # Install requirements before copying modules
 COPY ./pixl_core/pyproject.toml ./pixl_core/pyproject.toml

orthanc/orthanc-anon/plugin/pixl.py

@@ -72,6 +72,9 @@ def AzureAccessToken():
     return response.json()["access_token"]
 
 
+TIMER = None
+
+
 def AzureDICOMTokenRefresh():
     """
     Refresh Azure DICOM token

pixl_core/src/core/exports.py

@@ -80,9 +80,10 @@ def copy_to_exports(self, input_omop_dir: pathlib.Path) -> str:
         """
         public_input = input_omop_dir / "public"
 
+        logging.info("Copying public parquet files from %s to %s", public_input, self.public_output)
+
         # Make directory for exports if they don't exist
         ParquetExport._mkdir(self.public_output)
-        logger.info("Copying public parquet files from %s to %s", public_input, self.public_output)
 
         # Copy extract files, overwriting if it exists
         shutil.copytree(public_input, self.public_output, dirs_exist_ok=True)
@@ -96,6 +97,7 @@ def export_radiology(self, export_df: pd.DataFrame) -> pathlib.Path:
         """Export radiology reports to parquet file"""
         self._mkdir(self.radiology_output)
         parquet_file = self.radiology_output / "radiology.parquet"
+        logging.info("Exporting radiology to %s", self.radiology_output)
         export_df.to_parquet(parquet_file)
         # We are not responsible for making the "latest" symlink, see `copy_to_exports`.
         # This avoids the confusion caused by EHR API (which calls export_radiology) having a

pixl_core/src/core/upload.py

@@ -76,10 +76,11 @@ def upload_dicom_image(zip_content: BinaryIO, pseudo_anon_id: str) -> None:
 
     # Store the file using a binary handler
     try:
+        logger.info("Running command %s", command)
         ftp.storbinary(command, zip_content)
     except ftplib.all_errors as ftp_error:
         ftp.quit()
-        error_msg = "Failed to run STOR command '%s': '%s'"
+        error_msg = f"Failed to run STOR command : {command}"
         raise ConnectionError(error_msg, command, ftp_error) from ftp_error
 
     # Close the FTP connection
@@ -159,8 +160,8 @@ def _connect_to_ftp() -> FTP_TLS:
         ftp.login(ftp_user, ftp_password)
         ftp.prot_p()
     except ftplib.all_errors as ftp_error:
-        error_msg = "Failed to connect to FTPS server: '%s'"
-        raise ConnectionError(error_msg, ftp_error) from ftp_error
+        error_msg = f"Failed to connect to FTPS server: {ftp_user}@{ftp_host}:{ftp_port}"
+        raise ConnectionError(error_msg) from ftp_error
     return ftp
 
 

pixl_core/tests/conftest.py

@@ -23,6 +23,7 @@
 import pytest
 from core.db.models import Base, Extract, Image
 from core.exports import ParquetExport
+from pytest_pixl.plugin import FtpHostAddress
 from sqlalchemy import Engine, create_engine
 from sqlalchemy.orm import Session, sessionmaker
 
@@ -79,6 +80,12 @@ def ftps_home_dir(ftps_server) -> Generator[Path, None, None]:
     return Path(ftps_server.home_dir)
 
 
+@pytest.fixture(scope="session")
+def ftp_host_address():
+    """Run FTP on localhost - no docker containers need to access it"""
+    return FtpHostAddress.LOCALHOST
+
+
 @pytest.fixture()
 def test_zip_content() -> BinaryIO:
     """Directory containing the test data for uploading to the ftp server."""

pytest-pixl/src/pytest_pixl/ftpserver.py

@@ -12,13 +12,16 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 """A ligthweight FTPS server supporting implicit SSL for use in PIXL tests."""
-
+import logging
 from pathlib import Path
 
 from decouple import config
 from pyftpdlib.authorizers import DummyAuthorizer
 from pyftpdlib.handlers import TLS_FTPHandler
-from pyftpdlib.servers import FTPServer
+from pyftpdlib.servers import ThreadedFTPServer
+
+logger = logging.getLogger(__name__)
+logger.setLevel("INFO")
 
 # User permission
 # from https://pyftpdlib.readthedocs.io/en/latest/api.html#pyftpdlib.authorizers.DummyAuthorizer.add_user
@@ -73,19 +76,20 @@ class PixlFTPServer:
     :type home_dir: str
     """
 
-    def __init__(self, home_dir: str) -> None:
+    def __init__(self, home_root: Path, host_address: str) -> None:
         """
         Initialise the FTPS server. Sets the hostname, port, username and password
         from the corresponding environment variables.
-        :param home_dir: The home directory for the FTP server. This is the directory where the
-            user will be placed after login.
+        :param home_root: The directory where the user's home directory will be created.
+        The home dir is the directory where the user will be placed after login.
         """
-        self.host = config("FTP_HOST", default="127.0.0.1")
+        self.host = host_address
         self.port = int(config("FTP_PORT", default=20021))
 
         self.user_name = config("FTP_USER_NAME", default="pixl_user")
         self.user_password = config("FTP_USER_PASSWORD", default="longpassword")
-        self.home_dir = home_dir
+        self.home_dir: Path = home_root / self.user_name
+        self.home_dir.mkdir()
 
         self.certfile = Path(__file__).parents[1] / "resources" / "ssl" / "localhost.crt"
         self.keyfile = Path(__file__).parents[1] / "resources" / "ssl" / "localhost.key"
@@ -103,7 +107,7 @@ def _add_user(self) -> None:
         will be a directory on the local filesystem!
         """
         self.authorizer.add_user(
-            self.user_name, self.user_password, self.home_dir, perm=USER_PERMISSIONS
+            self.user_name, self.user_password, str(self.home_dir), perm=USER_PERMISSIONS
         )
 
     def _setup_TLS_handler(self) -> None:
@@ -119,11 +123,12 @@ def _check_ssl_files(self) -> None:
         assert certfile_path.exists(), f"Could not find cerfile at {certfile_path.absolute()}"
         assert keyfile_path.exists(), f"Could not find cerfile at {keyfile_path.absolute()}"
 
-    def _create_server(self) -> FTPServer:
+    def _create_server(self) -> ThreadedFTPServer:
         """
         Creates the FTPS server and returns it. The server can be started with the `serve_forever`
         method.
         """
         address = (self.host, self.port)
-        self.server = FTPServer(address, self.handler)
+        logger.info("Starting FTP server on %s:%d", self.host, self.port)
+        self.server = ThreadedFTPServer(address, self.handler)
         return self.server