Address comments

* Move download functionality to extras module
* Update Makefile command for stopping MinIO server to use .env
* Clarify instructions for running local MinIO server in README
* Fix flag description for '--download-files' CLI option
* Simplify Docker Compose YAML file
* Simplify logging in 'download_input_files'
  * Log clear messages indicating download counts
  * Log clear messages
* Raise RuntimeError if any files fail to download

Author: jonavellecuerdo

Makefile

@@ -1,6 +1,6 @@
 SHELL=/bin/bash
 DATETIME:=$(shell date -u +%Y%m%dT%H%M%SZ)
-MINIO_COMPOSE_FILE=abdiff/helpers/minio/docker-compose.yaml
+MINIO_COMPOSE_FILE=abdiff/extras/minio/docker-compose.yaml
 
 help: # Preview Makefile commands
 	@awk 'BEGIN { FS = ":.*#"; print "Usage:  make <target>\n\nTargets:" } \
@@ -56,9 +56,12 @@ black-apply: # Apply changes with 'black'
 ruff-apply: # Resolve 'fixable errors' with 'ruff'
 	pipenv run ruff check --fix .
 
-# Development commands
+####################################
+# MinIO local S3 commands
+####################################
+
 start-minio-server:
 	docker compose --env-file .env -f $(MINIO_COMPOSE_FILE) up -d
 
 stop-minio-server:
-	docker compose -f $(MINIO_COMPOSE_FILE) stop
+	docker compose --env-file .env -f $(MINIO_COMPOSE_FILE) stop

README.md

@@ -17,13 +17,18 @@ Compare transformed TIMDEX records from two versions (A,B) of Transmogrifier.
 
 ### Running a Local MinIO Server
 
-TIMDEX extract files from S3 (i.e., input files to use in transformations) can be downloaded to a local MinIO server hosted with a Docker container. [MinIO is an object storage solution that provides an Amazon Web Services S3-compatible API and supports all core S3 features](https://min.io/docs/minio/kubernetes/upstream/). Downloading extract files improves the runtime of a diff by reducing the number of requests sent to S3 and avoids repeated downloads of extract files.
+TIMDEX extract files from S3 (i.e., input files to use in transformations) can be downloaded to a local MinIO server hosted via a Docker container. [MinIO is an object storage solution that provides an Amazon Web Services S3-compatible API and supports all core S3 features](https://min.io/docs/minio/kubernetes/upstream/). The MinIO server acts as a "local S3 file system", allowing the app to access data on disk through an S3 interface. Since the MinIO server runs in a Docker container, it can be easily started when needed and stopped when not in use. Any data stored in the MinIO server will persist as long as the files exist in the directory specified for `MINIO_S3_LOCAL_STORAGE`.
+
+Downloading extract files improves the runtime of a diff by reducing the number of requests sent to S3 and avoids AWS credentials timing out. Once an extract file is stored in the local MinIO server, the app can access the data from MinIO for all future runs that include the extract file, avoiding repeated downloads of data used across multiple runs. 
+
 
 1. Configure your `.env` file. In addition to the [required environment variables](#required), the following environment variables must also be set:
 
    ```text
-   MINIO_S3_LOCAL_STORAGE=<absolute-path-on-local-disk>
-   TIMDEX_BUCKET=<timdex-extract-bucket>
+   MINIO_S3_LOCAL_STORAGE=# full file system path to the directory where MinIO stores its object data on the local disk
+   MINIO_ROOT_USER=# username for root user account for MinIO server
+   MINIO_ROOT_PASSWORD=# password for root user account MinIO server
+   TIMDEX_BUCKET=# when using CLI command 'timdex-sources-csv', this is required to know what TIMDEX bucket to use
    ```
 
    Note: There are additional variables required by the Local MinIO server (see vars prefixed with "MINIO" in [optional environment variables](#optional)). For these variables, defaults are provided in [abdiff.config](abdiff/config.py).
@@ -45,7 +50,7 @@ TIMDEX extract files from S3 (i.e., input files to use in transformations) can b
 
 5. Proceed with A/B Diff CLI commands as needed!
 
-Once a diff run is complete, you can stop the local MinIO server using the Makefile command: `make stop-minio-server`. If you're planning to run another diff using the same files -- good news! All you have to do is restart the local MinIO server. Your data will persist as long as the files exist in the directory you specified for `MINIO_S3_LOCAL_STORAGE`.
+Once a diff run is complete, you can stop the local MinIO server using the Makefile command: `make stop-minio-server`. If you're planning to run another diff using the same files, all you have to do is restart the local MinIO server. Your data will persist as long as the files exist in the directory you specified for `MINIO_S3_LOCAL_STORAGE`.
 
 ## Concepts
 

abdiff/cli.py

@@ -150,7 +150,12 @@ def init_job(
     default="Not provided.",
 )
 @click.option(
-    "--download-files", is_flag=True, help="Pass to skip download of extract files"
+    "--download-files",
+    is_flag=True,
+    help=(
+        "Pass to download input files from AWS S3 to a local Minio S3 server "
+        "for Transmogrifier to use."
+    ),
 )
 def run_diff(
     job_directory: str, input_files: str, message: str, *, download_files: bool

abdiff/config.py

@@ -32,10 +32,12 @@ def __getattr__(self, name: str) -> Any:  # noqa: ANN401
 
     @property
     def minio_s3_url(self) -> str:
+        """Host for ABDiff context (host machine) to connect to MinIO."""
         return self.MINIO_S3_URL or "http://localhost:9000/"
 
     @property
     def minio_s3_container_url(self) -> str:
+        """Host for Transmogrifier Docker containers to connect to MinIO."""
         return self.MINIO_S3_CONTAINER_URL or "http://host.docker.internal:9000/"
 
     @property

abdiff/core/__init__.py

@@ -7,10 +7,10 @@
 from abdiff.core.calc_ab_diffs import calc_ab_diffs
 from abdiff.core.calc_ab_metrics import calc_ab_metrics
 from abdiff.core.collate_ab_transforms import collate_ab_transforms
-from abdiff.core.download_input_files import download_input_files
 from abdiff.core.init_job import init_job
 from abdiff.core.init_run import init_run
 from abdiff.core.run_ab_transforms import run_ab_transforms
+from abdiff.extras.minio.download_input_files import download_input_files
 
 __all__ = [
     "init_job",

abdiff/extras/minio/__init__.py

@@ -0,0 +1,17 @@
+services: 
+  minio:
+    image: quay.io/minio/minio:latest
+    command: server --console-address ":9001" /mnt/data
+    ports:
+      - "9000:9000"  # API port
+      - "9001:9001"  # Console port
+    environment:
+      MINIO_ROOT_USER: ${MINIO_ROOT_USER}
+      MINIO_ROOT_PASSWORD: ${MINIO_ROOT_PASSWORD}
+    healthcheck:
+      test: ["CMD", "mc", "ready", "local"]
+      interval: 5s
+      timeout: 5s
+      retries: 5
+    volumes:
+      - ${MINIO_S3_LOCAL_STORAGE}:/mnt/data

abdiff/extras/minio/docker-compose.yaml

@@ -40,36 +40,53 @@ def download_input_files(input_files: list[str]) -> None:
         aws_secret_access_key=CONFIG.minio_root_password,
     )
 
-    for input_file in input_files:
-        if check_object_exists(CONFIG.TIMDEX_BUCKET, input_file, s3_client):
-            logger.info(f"File found for input: {input_file}. Skipping download.")
-            continue
-
-        logger.info(f"Downloading input file from {CONFIG.TIMDEX_BUCKET}: {input_file}")
-        copy_command = ["aws", "s3", "cp", input_file, "-"]
-        upload_command = [
-            "aws",
-            "s3",
-            "cp",
-            "--endpoint-url",
-            CONFIG.minio_s3_url,
-            "--profile",
-            "minio",
-            "-",
-            input_file,
-        ]
-
+    success_count = 0
+    fail_count = 0
+    for i, input_file in enumerate(input_files):
         try:
-            copy_process = subprocess.run(
-                args=copy_command, check=True, capture_output=True
-            )
-            subprocess.run(
-                args=upload_command,
-                check=True,
-                input=copy_process.stdout,
+            download_input_file(input_file, s3_client)
+            success_count += 1
+            logger.info(
+                f"Input file: {i + 1} / {len(input_files)}: '{input_file}' "
+                "available locally for transformation."
             )
         except subprocess.CalledProcessError:
-            logger.exception(f"Failed to download input file: {input_file}")
+            fail_count += 1
+            logger.info(
+                f"Input file: {i + 1} / {len(input_files)}: '{input_file}' "
+                "failed to download."
+            )
+    logger.info(
+        f"Available input files: {success_count}, missing input files: {fail_count}."
+    )
+
+    if fail_count > 0:
+        raise RuntimeError(  # noqa: TRY003
+            f"{fail_count} input file(s) failed to download."
+        )
+
+
+def download_input_file(input_file: str, s3_client: S3Client) -> None:
+    if check_object_exists(CONFIG.TIMDEX_BUCKET, input_file, s3_client):
+        return
+    copy_command = ["aws", "s3", "cp", input_file, "-"]
+    upload_command = [
+        "aws",
+        "s3",
+        "cp",
+        "--endpoint-url",
+        CONFIG.minio_s3_url,
+        "--profile",
+        "minio",
+        "-",
+        input_file,
+    ]
+    copy_process = subprocess.run(args=copy_command, check=True, capture_output=True)
+    subprocess.run(
+        args=upload_command,
+        check=True,
+        input=copy_process.stdout,
+    )
 
 
 def check_object_exists(bucket: str, input_file: str, s3_client: S3Client) -> bool:
@@ -79,7 +96,6 @@ def check_object_exists(bucket: str, input_file: str, s3_client: S3Client) -> bo
     except ClientError as exception:
         if exception.response["Error"]["Code"] == "404":
             return False
-        logger.exception(f"Cannot determine if object exists for key {key}.")
         return False
     else:
         return True