fix: store asset failure report supports exception without logs (#970)

* chore: move `get_error_type` Signed-off-by: Guilhem Barthés <[email protected]> * feat: create `_BuildError` as base error for build error Signed-off-by: Guilhem Barthés <[email protected]> * doc: change fragment Signed-off-by: Guilhem Barthés <[email protected]> * chore: refactor `get_error_type` Signed-off-by: Guilhem Barthés <[email protected]> * chore: move change fragment `970.fixed` to the correct place Signed-off-by: Guilhem Barthés <[email protected]> --------- Signed-off-by: Guilhem Barthés <[email protected]>
Substra · Aug 13, 2024 · 2186fbe · 2186fbe
1 parent 7f70495
commit 2186fbe
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 46 deletions.
diff --git a/backend/builder/exceptions.py b/backend/builder/exceptions.py
@@ -3,7 +3,6 @@
 from substrapp.compute_tasks.errors import CeleryNoRetryError
 from substrapp.compute_tasks.errors import CeleryRetryError
 from substrapp.compute_tasks.errors import ComputeTaskErrorType
-from substrapp.compute_tasks.errors import _ComputeTaskError
 
 
 class PodError(Exception):
@@ -14,7 +13,7 @@ class PodTimeoutError(Exception):
     pass
 
 
-class BuildRetryError(_ComputeTaskError, CeleryRetryError):
+class _BuildError(RuntimeError):
     """An error occurred during the build of a container image.
 
     Args:
@@ -28,15 +27,17 @@ def __init__(self, logs: str, *args: list, **kwargs: dict):
         super().__init__(logs, *args, **kwargs)
 
 
-class BuildError(_ComputeTaskError, CeleryNoRetryError):
+class BuildError(_BuildError, CeleryNoRetryError):
     """An error occurred during the build of a container image.
 
     Args:
         logs (str): the container image build logs
     """
 
-    error_type = ComputeTaskErrorType.BUILD_ERROR
 
-    def __init__(self, logs: str, *args: list, **kwargs: dict):
-        self.logs = BytesIO(str.encode(logs))
-        super().__init__(logs, *args, **kwargs)
+class BuildRetryError(_BuildError, CeleryRetryError):
+    """An error occurred during the build of a container image.
+
+    Args:
+        logs (str): the container image build logs
+    """
diff --git a/backend/substrapp/compute_tasks/errors.py b/backend/substrapp/compute_tasks/errors.py
@@ -74,24 +74,6 @@ def __init__(self, logs: BinaryIO, *args, **kwargs):
         super().__init__(logs, *args, **kwargs)
 
 
-def get_error_type(exc: Exception) -> failure_report_pb2.ErrorType.ValueType:
-    """From a given exception, return an error type safe to store and to advertise to the user.
-
-    Args:
-        exc: The exception to process.
-
-    Returns:
-        The error type corresponding to the exception.
-    """
-
-    if isinstance(exc, _ComputeTaskError):
-        error_type = exc.error_type
-    else:
-        error_type = ComputeTaskErrorType.INTERNAL_ERROR
-
-    return error_type.value
-
-
 class InvalidContextError(_ComputeTaskError, CeleryNoRetryError):
     """Error due to invalid task Context"""
 

diff --git a/backend/substrapp/tasks/tasks_asset_failure_report.py b/backend/substrapp/tasks/tasks_asset_failure_report.py
@@ -5,9 +5,9 @@
 from django.conf import settings
 
 from backend.celery import app
+from orchestrator import failure_report_pb2
 from orchestrator import get_orchestrator_client
 from substrapp.compute_tasks import errors as compute_task_errors
-from substrapp.models import FailedAssetKind
 from substrapp.utils.errors import store_failure
 
 REGISTRY = settings.REGISTRY
@@ -45,10 +45,7 @@ def store_asset_failure_report(
 ) -> None:
     exception = pickle.loads(exception_pickled)  # nosec B301
 
-    if asset_type == FailedAssetKind.FAILED_ASSET_FUNCTION:
-        error_type = compute_task_errors.ComputeTaskErrorType.BUILD_ERROR.value
-    else:
-        error_type = compute_task_errors.get_error_type(exception)
+    error_type = get_error_type(exception)
 
     failure_report = store_failure(exception, asset_key, asset_type, error_type)
 
@@ -66,3 +63,24 @@ def store_asset_failure_report(
         client.register_failure_report(
             {"asset_key": asset_key, "error_type": error_type, "asset_type": asset_type, "logs_address": logs_address}
         )
+
+
+def get_error_type(exc: Exception) -> failure_report_pb2.ErrorType.ValueType:
+    """From a given exception, return an error type safe to store and to advertise to the user.
+
+    Args:
+        exc: The exception to process.
+
+    Returns:
+        The error type corresponding to the exception.
+    """
+
+    if not hasattr(exc, "logs"):
+        return compute_task_errors.ComputeTaskErrorType.INTERNAL_ERROR.value
+
+    try:
+        error_type = exc.error_type  # type: ignore[attr-defined]
+    except AttributeError:
+        error_type = compute_task_errors.ComputeTaskErrorType.INTERNAL_ERROR
+
+    return error_type.value
diff --git a/backend/substrapp/tests/compute_tasks/test_errors.py b/backend/substrapp/tests/compute_tasks/test_errors.py
@@ -1,9 +1,5 @@
-import io
-
 import pytest
 
-from builder import exceptions as build_errors
-from orchestrator import failure_report_pb2
 from substrapp.compute_tasks import errors
 
 
@@ -15,15 +11,3 @@ class TestComputeTaskErrorType:
     def test_from_int(self, input_value: int, expected: errors.ComputeTaskErrorType):
         result = errors.ComputeTaskErrorType.from_int(input_value)
         assert result == expected
-
-
-@pytest.mark.parametrize(
-    ("exc", "expected"),
-    [
-        (build_errors.BuildError(logs="some build error"), failure_report_pb2.ERROR_TYPE_BUILD),
-        (errors.ExecutionError(logs=io.BytesIO()), failure_report_pb2.ERROR_TYPE_EXECUTION),
-        (Exception(), failure_report_pb2.ERROR_TYPE_INTERNAL),
-    ],
-)
-def test_get_error_type(exc: Exception, expected: str):
-    assert errors.get_error_type(exc) == expected
diff --git a/backend/substrapp/tests/tasks/test_store_asset_failure_report.py b/backend/substrapp/tests/tasks/test_store_asset_failure_report.py
@@ -5,9 +5,12 @@
 import pytest
 from pytest_mock import MockerFixture
 
+from builder import exceptions as build_errors
+from orchestrator import failure_report_pb2
 from substrapp.compute_tasks import errors
 from substrapp.compute_tasks.errors import ComputeTaskErrorType
 from substrapp.models import FailedAssetKind
+from substrapp.tasks.tasks_asset_failure_report import get_error_type
 from substrapp.tasks.tasks_asset_failure_report import store_asset_failure_report
 from substrapp.utils.errors import store_failure
 
@@ -67,3 +70,15 @@ def test_store_failure_ignored_exception(exc_class: Type[Exception]):
         )
         is None
     )
+
+
+@pytest.mark.parametrize(
+    ("exc", "expected"),
+    [
+        (build_errors.BuildError(logs="some build error"), failure_report_pb2.ERROR_TYPE_BUILD),
+        (errors.ExecutionError(logs=io.BytesIO()), failure_report_pb2.ERROR_TYPE_EXECUTION),
+        (Exception(), failure_report_pb2.ERROR_TYPE_INTERNAL),
+    ],
+)
+def test_get_error_type(exc: Exception, expected: str):
+    assert get_error_type(exc) == expected
diff --git a/changes/970.fixed b/changes/970.fixed
@@ -0,0 +1 @@
+Celery was failing silently when a task called `FailableTask.on_failure` if the task didn't have a `logs` attribute (now return internal error)