Add WorkflowUpdateRPCTimeoutOrCancelledError

[cretz]

What was changed

• Add temporalio.client.RPCTimeoutOrCancelledError that is a base exception class that can be used for RPC timeout/cancelled errors
• Add temporalio.client.WorkflowUpdateRPCTimeoutOrCancelledError that extends RPCTimeoutOrCancelledError and is raised when any update RPC call times out or is cancelled
• Tests to confirm

See temporalio/features#483. I will update current features repo Python CI branch just before merging this.

Checklist

1. Closes Wrap GRPC::CANCELED and DEADLINE_EXCEEDED in an SDK Timeout exception for Update #529

[cretz]

I would like feedback on whether this is what we want to call this. Options:

• WorkflowUpdateRPCTimeoutOrCancelledError
• WorkflowUpdateRPCTimedOutOrCancelledError
• WorkflowUpdateRPCTimeoutOrCancellationError

There may be some other options, but it's important that we include "rpc" in here so users know it's about the RPC call and not some potential future concept of update timeout/cancellation.

[Sushisource]

I'd voted TimedOut, since that consistent with cancelled

[cretz]

👍 Will leave open a bit to get more opinions (and confer w/ team before decision)

[drewhoskins-temporal]

I think Timeout for search optimization despite grammatical oddness. It's what people in python expect.

it's important that we include "rpc" in here so users know it's about the RPC call and not some potential future concept of update timeout/cancellation

👎 For me, the main point of this exercise is to reduce the size and complexity of the handler block and the number of times users need to add logic. Thus we want to capture all timeouts into one exception. If we need to disambiguate in the future for power users, let's add fields.

Taking this idea further, please consider one more option. Given that we have the GRPC::Canceled inner exception, that also lets us keep the name simple. For example, we can just do WorkflowUpdateTimeoutError leaving us the ability to have a Canceled error when we add update cancels. But I don't feel strongly about this one.

[drewhoskins-temporal]

Chad and I chatted offline. He's thinking of when we add a "run timeout" to the update, and I'm convinced that we should distinguish "the client gave up" from "the workflow gave up".

Lemme clarify my thoughts. The thing that snags me about "RPC" specifically is it's a leaky abstraction, at least the way I read it. You could easily implement these patterns by polling or by having two RPC calls (start and then wait vs execute), in which case the meaning of "RPC" changes and could be ambiguous. If I put an rpc_timeout=30 but I have client retries, what does that mean? Does each RPC get 30 seconds, or do I as the user want to wait 30 seconds total?

What we really mean, semantically, is "the client gave up" so in the abstract I would prefer terms like ClientTimeoutError. (And maybe that's also general enough to encompass the "client canceled" meaning?) But since we already expose fields like rpc_timeout, we've already crossed that bridge, so I'm fine with the naming you chose if you like it better or feel it's more consistent with other parts of the product.

When we add durable-on-admitted, it'd be nice if we could add a field on this exception like last_wait_stage_confirmed={None, Admitted, Accepted}. (But perhaps people who want that should use start_update(wait_stage=Admitted) and then poll on the handle.)

[cretz]

we've already crossed that bridge

Not necessarily. This is a brand new thing to wrap these kinds of gRPC errors, so we can change. We can have ClientTimeoutError and WorkflowUpdateClientTimeoutError, but need to confirm that that also includes Cancelled status codes and asyncio.Cancelled cancellation or if we just want it as DeadlineExceeded (I am not sure that "timeout" is a good name for something that includes cancellation).

[Quinn-With-Two-Ns]

I would leave off the RPC prefix if the exceptions are already in the client package. Otherwise I have no strong opinion

[Sushisource]

Suggested change

	"""Error that occurs on some client calls that timeout or get cancelled."""
	"""Error that occurs on some client calls when they time out or get cancelled."""

[Sushisource]

I'd voted TimedOut, since that consistent with cancelled

[drewhoskins-temporal]

All of these are fine. If you like ...ClientTimeoutOrCancelled, it seems flexible enough. (DeadlineExceeded doesn't help me understand that it includes cancel, personally. I'm good with the explicitness of what you did.)

…

On Tue, Jun 11, 2024 at 05:38 Chad Retz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In temporalio/client.py <#548 (comment)> : > @@ -4456,6 +4478,24 @@ def cause(self) -> BaseException: return self.__cause__ +class RPCTimeoutOrCancelledError(temporalio.exceptions.TemporalError): + """Error that occurs on some client calls that timeout or get cancelled.""" + + pass + + +class WorkflowUpdateRPCTimeoutOrCancelledError(RPCTimeoutOrCancelledError): we've already crossed that bridge Not necessarily. This is a brand new thing to wrap these kinds of gRPC errors, so we can change. We can have ClientTimeoutError and WorkflowUpdateClientTimeoutError, but need to confirm that that also includes Cancelled status codes and asyncio.Cancelled cancellation or if we just want it as DeadlineExceeded (I am not sure that "timeout" is a good name for something that includes cancellation). — Reply to this email directly, view it on GitHub <#548 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BHV3GXP4ITVSHX2DSWTTKD3ZG3VVTAVCNFSM6AAAAABJDDCPC6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMJQGMZTOOBVGE> . You are receiving this because you commented.Message ID: ***@***.***>