From e99e62faf5c5074f564d4134e3a82e715fdf3e1d Mon Sep 17 00:00:00 2001 From: Chad Retz Date: Thu, 15 Jun 2023 14:34:17 -0500 Subject: [PATCH 1/4] Remove build info dump from CI --- .github/workflows/ci.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a85784241..7641c5f38 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,8 +25,6 @@ jobs: runsOn: buildjet-4vcpu-ubuntu-2204-arm runs-on: ${{ matrix.runsOn || matrix.os }} steps: - - name: Print build information - run: "echo head_ref: ${{ github.head_ref }}, ref: ${{ github.ref }}, os: ${{ matrix.os }}, python: ${{ matrix.python }}" - uses: actions/checkout@v2 with: submodules: recursive From 43cc9bfa360baaabb4dc1e440242ea83a25a876d Mon Sep 17 00:00:00 2001 From: Chad Retz Date: Thu, 15 Jun 2023 14:36:59 -0500 Subject: [PATCH 2/4] Remove Pydantic generic reference --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index fefbdda11..9c27d3cb9 100644 --- a/README.md +++ b/README.md @@ -306,8 +306,8 @@ This notably doesn't include any `date`, `time`, or `datetime` objects as they m Users are strongly encouraged to use a single `dataclass` for parameter and return types so fields with defaults can be easily added without breaking compatibility. -Classes with generics may not have the generics properly resolved. The current implementation, similar to Pydantic, does -not have generic type resolution. Users should use concrete types. +Classes with generics may not have the generics properly resolved. The current implementation, does not have generic +type resolution. Users should use concrete types. ##### Custom Type Data Conversion From 242f2a1851b55ef0ebe337cc0a4908364c241f3d Mon Sep 17 00:00:00 2001 From: Chad Retz Date: Thu, 15 Jun 2023 16:21:35 -0500 Subject: [PATCH 3/4] Allow payload conversion exception to fail workflow --- poetry.lock | 25 ++++++------- pyproject.toml | 4 +-- temporalio/exceptions.py | 30 ++++++++++++++++ temporalio/worker/_workflow_instance.py | 47 ++++++++++++++----------- tests/worker/test_workflow.py | 44 +++++++++++++++++++++++ 5 files changed, 113 insertions(+), 37 deletions(-) diff --git a/poetry.lock b/poetry.lock index 6b32e8ff0..6e6ebc31c 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry and should not be changed by hand. +# This file is automatically @generated by Poetry 1.4.0 and should not be changed by hand. [[package]] name = "appdirs" @@ -1248,13 +1248,15 @@ toml = ["toml"] [[package]] name = "pydoctor" -version = "22.9.1.dev0" +version = "23.4.1" description = "API doc generator." category = "dev" optional = false python-versions = ">=3.6" -files = [] -develop = false +files = [ + {file = "pydoctor-23.4.1-py3-none-any.whl", hash = "sha256:4aeff65750651942314032b85f854394ff9cf77d1353035e7cf6388ad8ede5ee"}, + {file = "pydoctor-23.4.1.tar.gz", hash = "sha256:3df965f2463430915a03de30b63e0c8dd93f0118e05d61786a13f5e780520f64"}, +] [package.dependencies] appdirs = "*" @@ -1263,24 +1265,19 @@ attrs = "*" CacheControl = {version = "*", extras = ["filecache"]} configargparse = "*" docutils = ">=0.17" -importlib_metadata = {version = "*", markers = "python_version < \"3.8\""} -importlib_resources = {version = "*", markers = "python_version < \"3.9\""} +importlib-metadata = {version = "*", markers = "python_version < \"3.8\""} +importlib-resources = {version = "*", markers = "python_version < \"3.9\""} lunr = "0.6.2" requests = "*" toml = "*" Twisted = "*" +urllib3 = "<2" [package.extras] -docs = ["Sphinx", "sphinx-argparse", "sphinx_rtd_theme", "sphinxcontrib-spelling"] +docs = ["Sphinx", "sphinx-argparse", "sphinx-rtd-theme", "sphinxcontrib-spelling"] rst = ["docutils"] test = ["Sphinx (>=3.5)", "bs4", "coverage", "cython-test-exception-raiser (==1.0.0)", "docutils (>=0.18.1)", "hypothesis", "pytest"] -[package.source] -type = "git" -url = "https://github.com/cretz/pydoctor.git" -reference = "overloads" -resolved_reference = "2a1bb77259e0d01b763c6e0ea52b77175e589cb0" - [[package]] name = "Pygments" version = "2.13.0" @@ -1925,4 +1922,4 @@ opentelemetry = ["opentelemetry-api", "opentelemetry-sdk"] [metadata] lock-version = "2.0" python-versions = "^3.7" -content-hash = "8c0a2f562b3e904f55fe41c16722119cbfbf31f4d4fadf1aaf1c29b9127160bd" +content-hash = "fceaa83bf87bed96fcb061b77723a1ebe22525199bf1a20a56197557242e72ac" diff --git a/pyproject.toml b/pyproject.toml index 4a08f6a32..a5765a63d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -51,9 +51,7 @@ protoc-wheel-0 = "^21.1" psutil = "^5.9.3" pydantic = "^1.9.1" pydocstyle = "^6.1.1" -# TODO(cretz): Update when https://github.com/twisted/pydoctor/pull/595 released -# pydoctor = "^22.5.1" -pydoctor = { git = "https://github.com/cretz/pydoctor.git", branch = "overloads" } +pydoctor = "^23.4.1" pytest = "^7.1.2" pytest-asyncio = "^0.18.3" pytest-timeout = "^2.1.0" diff --git a/temporalio/exceptions.py b/temporalio/exceptions.py index 59014fa99..3a593415b 100644 --- a/temporalio/exceptions.py +++ b/temporalio/exceptions.py @@ -1,5 +1,6 @@ """Common Temporal exceptions.""" +import asyncio from enum import IntEnum from typing import Any, Optional, Sequence, Tuple @@ -322,3 +323,32 @@ def started_event_id(self) -> int: def retry_state(self) -> Optional[RetryState]: """Retry state for this error.""" return self._retry_state + + +def is_cancelled_exception(exception: BaseException) -> bool: + """Check whether the given exception is considered a cancellation exception + according to Temporal. + + This is often used in a conditional of a catch clause to check whether a + cancel occurred inside of a workflow. This can occur from + :py:class:`asyncio.CancelledError` or :py:class:`CancelledError` or either + :py:class:`ActivityError` or :py:class:`ChildWorkflowError` if either of + those latter two have a :py:class:`CancelledError` cause. + + Args: + exception: Exception to check. + + Returns: + True if a cancelled exception, false if not. + """ + return ( + isinstance(exception, asyncio.CancelledError) + or isinstance(exception, CancelledError) + or ( + ( + isinstance(exception, ActivityError) + or isinstance(exception, ChildWorkflowError) + ) + and isinstance(exception.cause, CancelledError) + ) + ) diff --git a/temporalio/worker/_workflow_instance.py b/temporalio/worker/_workflow_instance.py index 2f1ac5ff0..2dc065fe4 100644 --- a/temporalio/worker/_workflow_instance.py +++ b/temporalio/worker/_workflow_instance.py @@ -259,6 +259,7 @@ def activate( self._time_ns = act.timestamp.ToNanoseconds() self._is_replaying = act.is_replaying + activation_err: Optional[Exception] = None try: # Split into job sets with patches, then signals, then non-queries, then # queries @@ -287,7 +288,17 @@ def activate( # be checked in patch jobs (first index) or query jobs (last # index). self._run_once(check_conditions=index == 1 or index == 2) + except temporalio.exceptions.FailureError as err: + # We want failure errors during activation, like those that can + # happen during payload conversion, to fail the workflow not the + # task + try: + self._set_workflow_failure(err) + except Exception as inner_err: + activation_err = inner_err except Exception as err: + activation_err = err + if activation_err: logger.warning( f"Failed activation on workflow {self._info.workflow_type} with ID {self._info.workflow_id} and run ID {self._info.run_id}", exc_info=True, @@ -296,7 +307,7 @@ def activate( self._current_completion.failed.failure.SetInParent() try: self._failure_converter.to_failure( - err, + activation_err, self._payload_converter, self._current_completion.failed.failure, ) @@ -1134,6 +1145,9 @@ def _convert_payloads( payloads, type_hints=types, ) + except temporalio.exceptions.FailureError: + # Don't wrap payload conversion errors that would fail the workflow + raise except Exception as err: raise RuntimeError("Failed decoding arguments") from err @@ -1227,33 +1241,26 @@ async def _run_top_level_workflow_function(self, coro: Awaitable[None]) -> None: # cancel later on will show the workflow as cancelled. But this is # a Temporal limitation in that cancellation is a state not an # event. - if self._cancel_requested and ( - isinstance(err, temporalio.exceptions.CancelledError) - or ( - ( - isinstance(err, temporalio.exceptions.ActivityError) - or isinstance(err, temporalio.exceptions.ChildWorkflowError) - ) - and isinstance(err.cause, temporalio.exceptions.CancelledError) - ) + if self._cancel_requested and temporalio.exceptions.is_cancelled_exception( + err ): self._add_command().cancel_workflow_execution.SetInParent() elif isinstance(err, temporalio.exceptions.FailureError): # All other failure errors fail the workflow - failure = self._add_command().fail_workflow_execution.failure - failure.SetInParent() - try: - self._failure_converter.to_failure( - err, self._payload_converter, failure - ) - except Exception as inner_err: - raise ValueError( - "Failed converting workflow exception" - ) from inner_err + self._set_workflow_failure(err) else: # All other exceptions fail the task self._current_activation_error = err + def _set_workflow_failure(self, err: temporalio.exceptions.FailureError) -> None: + # All other failure errors fail the workflow + failure = self._add_command().fail_workflow_execution.failure + failure.SetInParent() + try: + self._failure_converter.to_failure(err, self._payload_converter, failure) + except Exception as inner_err: + raise ValueError("Failed converting workflow exception") from inner_err + async def _signal_external_workflow( self, # Should not have seq set diff --git a/tests/worker/test_workflow.py b/tests/worker/test_workflow.py index fdf661305..999f726c0 100644 --- a/tests/worker/test_workflow.py +++ b/tests/worker/test_workflow.py @@ -20,6 +20,7 @@ Optional, Sequence, Tuple, + Type, cast, ) @@ -51,6 +52,7 @@ from temporalio.converter import ( DataConverter, DefaultFailureConverterWithEncodedAttributes, + DefaultPayloadConverter, PayloadCodec, PayloadConverter, ) @@ -2714,3 +2716,45 @@ async def test_workflow_optional_param(client: Client): task_queue=worker.task_queue, ) assert result3 == OptionalParam(some_string="foo") + + +class ExceptionRaisingPayloadConverter(DefaultPayloadConverter): + bad_str = "bad-payload-str" + + def from_payloads( + self, payloads: Sequence[Payload], type_hints: Optional[List] = None + ) -> List[Any]: + # Check if any payloads contain the bad data + for payload in payloads: + if ExceptionRaisingPayloadConverter.bad_str.encode() in payload.data: + raise ApplicationError("Intentional converter failure") + return super().from_payloads(payloads, type_hints) + + +@workflow.defn +class ExceptionRaisingConverterWorkflow: + @workflow.run + async def run(self, some_param: str) -> str: + return some_param + + +async def test_exception_raising_converter_param(client: Client): + # Clone the client but change the data converter to use our converter + config = client.config() + config["data_converter"] = dataclasses.replace( + config["data_converter"], + payload_converter_class=ExceptionRaisingPayloadConverter, + ) + client = Client(**config) + + # Run workflow and confirm error + async with new_worker(client, ExceptionRaisingConverterWorkflow) as worker: + with pytest.raises(WorkflowFailureError) as err: + await client.execute_workflow( + ExceptionRaisingConverterWorkflow.run, + ExceptionRaisingPayloadConverter.bad_str, + id=f"workflow-{uuid.uuid4()}", + task_queue=worker.task_queue, + ) + assert isinstance(err.value.cause, ApplicationError) + assert "Intentional converter failure" in str(err.value.cause) From 18fdc40b12cf05a4c5254031747436b0e03ba1ce Mon Sep 17 00:00:00 2001 From: Chad Retz Date: Wed, 5 Jul 2023 08:36:38 -0500 Subject: [PATCH 4/4] Remove one comma --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 9c27d3cb9..129c0f0e6 100644 --- a/README.md +++ b/README.md @@ -306,7 +306,7 @@ This notably doesn't include any `date`, `time`, or `datetime` objects as they m Users are strongly encouraged to use a single `dataclass` for parameter and return types so fields with defaults can be easily added without breaking compatibility. -Classes with generics may not have the generics properly resolved. The current implementation, does not have generic +Classes with generics may not have the generics properly resolved. The current implementation does not have generic type resolution. Users should use concrete types. ##### Custom Type Data Conversion