[CT-2136] fix: Update redact requests and redact_file (#107)

* add parameter redact request * [CT-2136] fix: Update redact requests and redact_file Add start_job_timeout for job creation. Make sure the job gets deleted on errors. * [CT-2136] fix: Handle nonexistent job variable --------- Co-authored-by: jose.larrain <[email protected]>
brighter-ai · Aug 6, 2024 · 800377b · 800377b
1 parent 37293b9
commit 800377b
Show file tree

Hide file tree

Showing 12 changed files with 123 additions and 15 deletions.
diff --git a/Makefile b/Makefile
@@ -1,4 +1,4 @@
-VERSION=9.0.0
+VERSION=9.1.0
 
 SHELL := /bin/bash
 
@@ -11,6 +11,13 @@ install:
 	make build
 	pip install . --upgrade
 
+reinstall:
+	make build
+	pip install . --force-reinstall --upgrade
+
+uninstall:
+	pip uninstall redact
+
 test-functional:
 	poetry run pytest tests/${api_version}/functional/ --api_key $(api_key) --redact_url $(redact_url)
 

diff --git a/pyproject.toml b/pyproject.toml
@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "redact"
-version = "9.0.0"
+version = "9.1.0"
 description = "Command-line and Python client for Brighter AI's Redact"
 authors = ["brighter AI <[email protected]>"]
 readme = "README.md"

diff --git a/redact/__init__.py b/redact/__init__.py
@@ -2,7 +2,7 @@
 Python client for "brighter Redact"
 """
 
-__version__ = "9.0.0"
+__version__ = "9.1.0"
 
 from .errors import RedactConnectError, RedactResponseError
 from .v4.data_models import (

diff --git a/redact/tools/v4.py b/redact/tools/v4.py
@@ -137,6 +137,13 @@ def redact_file(
         [],
         help="Key-value pairs in the format key=value which will be added to allr equest header",
     ),
+    start_job_timeout: Optional[float] = typer.Option(
+        60.0,
+        help=(
+            "Set the Redact Job creation timeout in seconds, "
+            "specifying how long to wait before the request to Redact fails."
+        ),
+    ),
 ):
     setup_logging(verbose_logging)
 
@@ -170,6 +177,7 @@ def redact_file(
         skip_existing=skip_existing,
         auto_delete_job=auto_delete_job,
         custom_headers=parsed_header,
+        start_job_timeout=start_job_timeout,
     )
 
 
@@ -294,6 +302,13 @@ def redact_folder(
         [],
         help="Key-value pairs in the format key=value which will be added to allr equest header",
     ),
+    start_job_timeout: Optional[float] = typer.Option(
+        60.0,
+        help=(
+            "Set the Redact Job creation timeout in seconds, "
+            "specifying how long to wait before the request to Redact fails."
+        ),
+    ),
 ):
     setup_logging(verbose_logging)
 
@@ -330,4 +345,5 @@ def redact_folder(
         auto_delete_job=auto_delete_job,
         auto_delete_input_file=auto_delete_input_file,
         custom_headers=parsed_header,
+        start_job_timeout=start_job_timeout,
     )
diff --git a/redact/v4/redact_instance.py b/redact/v4/redact_instance.py
@@ -36,6 +36,7 @@ def create(
         subscription_id: Optional[str] = None,
         api_key: Optional[str] = None,
         custom_headers: Optional[Dict] = None,
+        start_job_timeout: Optional[float] = 60.0,
     ) -> "RedactInstance":
         """
         The default way of creating RedactInstance objects.
@@ -45,6 +46,7 @@ def create(
             subscription_id=subscription_id,
             api_key=api_key,
             custom_headers=custom_headers,
+            start_job_timeout=start_job_timeout,
         )
         return cls(redact_requests=redact_requests, service=service, out_type=out_type)
 
@@ -61,6 +63,7 @@ def start_job(
             job_args=job_args,
             licence_plate_custom_stamp=licence_plate_custom_stamp,
         )
+
         return RedactJob(
             redact_requests=self.redact_requests,
             service=self.service,

diff --git a/redact/v4/redact_requests.py b/redact/v4/redact_requests.py
@@ -57,11 +57,14 @@ def __init__(
         api_key: Optional[str] = None,
         httpx_client: Optional[httpx.Client] = None,
         custom_headers: Optional[Dict] = None,
+        start_job_timeout: Optional[float] = 60.0,
+        retry_total_time_limit: Optional[int] = 600,  # 10 minutes in seconds
     ):
         self.redact_url = normalize_url(redact_url)
         self.api_key = api_key
         self.subscription_id = subscription_id
-        self.retry_total_time_limit: float = 600  # 10 minutes in seconds
+        self.retry_total_time_limit: float = retry_total_time_limit
+        self.start_job_timeout = start_job_timeout
 
         self._headers = {"Accept": "*/*"}
         if custom_headers is not None:
@@ -116,7 +119,7 @@ def post_job(
                 files=files,
                 params=job_args.dict(exclude_none=True),
                 headers=self._headers,
-                timeout=60.0,
+                timeout=self.start_job_timeout,
             )
             log.debug(
                 f"Post response to debug id (not output_id) {upload_debug_uuid}: {response}"

diff --git a/redact/v4/tools/redact_file.py b/redact/v4/tools/redact_file.py
@@ -37,6 +37,7 @@ def redact_file(
     waiting_time_between_job_status_checks: Optional[float] = None,
     redact_requests_param: Optional[RedactRequests] = None,
     custom_headers: Optional[Dict[str, str]] = None,
+    start_job_timeout: Optional[float] = 60.0,
 ) -> Optional[JobStatus]:
     """
     If no out_path is given, <input_filename_redacted> will be used.
@@ -81,6 +82,7 @@ def redact_file(
                 redact_url=redact_url,
                 api_key=api_key,
                 custom_headers=custom_headers,
+                start_job_timeout=start_job_timeout,
             )
         with open(file_path, "rb") as file:
             job: RedactJob = redact.start_job(
@@ -112,14 +114,19 @@ def redact_file(
         if licence_plate_custom_stamp:
             licence_plate_custom_stamp.close()
 
-    # delete input file
-    if auto_delete_input_file:
-        log.debug(f"Deleting {file_path}")
-        Path(file_path).unlink()
-
-    # delete job
-    if auto_delete_job:
-        job.delete()
+        # delete input file
+        if auto_delete_input_file:
+            log.debug(f"Deleting {file_path}")
+            Path(file_path).unlink()
+
+        try:
+            # delete job
+            if auto_delete_job:
+                log.debug(f"Deleting job {job.output_id}")
+                job.delete()
+        except UnboundLocalError:
+            # if the starting the job failed, there is no job variable and this Exception will be thrown
+            pass
 
     return job_status
 

diff --git a/redact/v4/tools/redact_folder.py b/redact/v4/tools/redact_folder.py
@@ -45,6 +45,7 @@ def redact_folder(
     auto_delete_job: bool = True,
     auto_delete_input_file: bool = False,
     custom_headers: Optional[Dict[str, str]] = None,
+    start_job_timeout: Optional[float] = 60.0,
 ) -> JobsSummary:
     # Normalize paths, e.g.: '~/..' -> '/home'
     in_dir_path = normalize_path(input_dir)
@@ -83,6 +84,7 @@ def redact_folder(
         auto_delete_job=auto_delete_job,
         auto_delete_input_file=auto_delete_input_file,
         custom_headers=custom_headers,
+        start_job_timeout=start_job_timeout,
     )
 
     log.info(f"Starting {n_parallel_jobs} parallel jobs to anonymize files ...")

diff --git a/tests/v4/functional/test_redact_command.py b/tests/v4/functional/test_redact_command.py
@@ -66,6 +66,7 @@ def test_redact_file_command_sends_none_values(
             skip_existing=True,
             auto_delete_job=True,
             custom_headers={"foo": "boo", "hello": "world"},
+            start_job_timeout=60,
         )
 
     def test_redact_folder_command_sends_none_values(
@@ -115,4 +116,5 @@ def test_redact_folder_command_sends_none_values(
             auto_delete_job=True,
             auto_delete_input_file=False,
             custom_headers={"foo": "boo", "hello": "world"},
+            start_job_timeout=60,
         )
diff --git a/tests/v4/functional/test_redact_folder.py b/tests/v4/functional/test_redact_folder.py
@@ -4,8 +4,9 @@
 
 import pytest
 
+from redact.errors import RedactConnectError
 from redact.v4 import InputType, JobArguments, OutputType, Region, ServiceType
-from redact.v4.tools.redact_file import redact_file
+from redact.v4.tools.redact_file import RedactRequests, redact_file
 from redact.v4.tools.redact_folder import redact_folder
 from tests.conftest import NUMBER_OF_IMAGES
 
@@ -98,6 +99,42 @@ def test_image_correct_file_ending(
         result_file = file_folder / f"{image_path.stem}_redacted{file_extension}"
         assert result_file.exists()
 
+    @pytest.mark.parametrize(
+        "output_type,service",
+        [
+            [OutputType.labels, ServiceType.redact_area],
+            [OutputType.overlays, ServiceType.dnat],
+            [OutputType.images, ServiceType.blur],
+        ],
+    )
+    def test_delete_method_called_on_error(
+        self,
+        image_path: Path,
+        redact_url,
+        optional_api_key,
+        output_type: OutputType,
+        service: ServiceType,
+        mocker,
+    ):
+        mocker.patch.object(
+            RedactRequests, "get_status", side_effect=RedactConnectError("Testing")
+        )
+        redact_request_delete = mocker.patch.object(RedactRequests, "delete_output")
+        # GIVEN an input image, service, and output_type
+        # WHEN the the file is anonymized
+        with pytest.raises(RedactConnectError):
+            redact_file(
+                file_path=image_path,
+                output_type=output_type,
+                service=service,
+                redact_url=redact_url,
+                api_key=optional_api_key,
+                job_args=JobArguments(region=Region.germany),
+            )
+
+        # THEN the delete enpoint is called
+        redact_request_delete.assert_called_once()
+
     @pytest.mark.parametrize(
         "output_type,service,file_extension",
         [

diff --git a/tests/v4/integration/mock_server.py b/tests/v4/integration/mock_server.py
@@ -15,6 +15,7 @@ def _mock_server_main(
     expected_job_args: Optional[JobArguments] = None,
     expected_form_content: Optional[dict] = None,
     expected_headers_contain: Optional[dict] = None,
+    expected_timeout: Optional[float] = None,
 ):
     app = FastAPI()
 
@@ -26,6 +27,7 @@ async def get_all_routes(request: Request):
             expected_job_args=expected_job_args,
             expected_form_content=expected_form_content,
             expected_headers_contain=expected_headers_contain,
+            expected_timeout=expected_timeout,
         )
 
     uvicorn.run(app=app, port=8787)
@@ -37,6 +39,7 @@ def mock_redact_server(
     expected_job_args: Optional[JobArguments] = None,
     expected_form_content: Optional[dict] = None,
     expected_headers_contain: Optional[dict] = None,
+    expected_timeout: Optional[float] = None,
 ):
     """
     Context manager that starts a mock Redact server (127.0.0.1:8787) which returns a 500 error when the request does
@@ -50,6 +53,7 @@ def mock_redact_server(
             expected_job_args,
             expected_form_content,
             expected_headers_contain,
+            expected_timeout,
         ),
         daemon=True,
     )
@@ -66,6 +70,7 @@ async def _mock_redact_request_handler(
     expected_job_args: Optional[JobArguments] = None,
     expected_form_content: Optional[dict] = None,
     expected_headers_contain: Optional[dict] = None,
+    expected_timeout: Optional[float] = None,
 ) -> Response:
     """
     Handle requests by taking a look at the requested path and the query arguments and comparing them to expected ones.
@@ -117,6 +122,9 @@ async def _mock_redact_request_handler(
                 content=f"Received headers content {repr(request.headers)} != expected to contain {repr(expected_headers_contain)}",
             )
 
+    if expected_timeout:
+        time.sleep(expected_timeout + 3)
+
     return Response(
         status_code=200, content=JobPostResponse(output_id=uuid.uuid4()).json()
     )
diff --git a/tests/v4/integration/test_requests.py b/tests/v4/integration/test_requests.py
@@ -1,6 +1,6 @@
 import pytest
 
-from redact.errors import RedactResponseError
+from redact.errors import RedactConnectError, RedactResponseError
 from redact.v4 import JobArguments, OutputType, RedactRequests, Region, ServiceType
 from tests.v4.integration.mock_server import mock_redact_server
 
@@ -116,3 +116,26 @@ def test_mock_server_receives_headers(some_image):
             out_type=out_type,
             job_args=job_args,
         )
+
+
+def test_request_fail_by_timeout(some_image):
+    service = ServiceType.blur
+    out_type = OutputType.images
+    job_args = JobArguments(face=True)
+
+    # GIVEN a (mocked) Redact server with expected timeout
+    with mock_redact_server(
+        expected_path=f"{service.value}/{API_VERSION}/{out_type.value}",
+        expected_timeout=1,
+    ):
+        # WHEN request is created with timeout and retry limit
+        redact_requests = RedactRequests(start_job_timeout=1, retry_total_time_limit=3)
+
+        # THEN the request fails with RedactConnectError because of the timeout
+        with pytest.raises(RedactConnectError):
+            redact_requests.post_job(
+                file=some_image,
+                service=service,
+                out_type=out_type,
+                job_args=job_args,
+            )