Propagate exceptions through `Delayed` objects

[EvanKirshenbaum]

One of the most awkward aspects to using Delayed futures is that when something goes wrong that would normally result in raising an exception, the code may well be at the end of a long chain of futures and may well be in a different thread from the code that would logically handle it.

Currently, the code base deals with this in three ways. First, in the DML implementation, I have

Val_ = TypeVar("Val_")
class EvaluationError(RuntimeError): ...
MaybeError = Union[Val_, EvaluationError]

def error_check(fn: Callable[..., Val_]) -> Callable[..., MaybeError[Val_]]:
    def check(*args: Any) -> MaybeError[Val_]:
        first = args[0]
        if isinstance(first, EvaluationError):
            return first
        return fn(*args)
    return check

def error_check_delayed(fn: Callable[..., Delayed[Val_]]) -> Callable[..., Delayed[MaybeError[Val_]]]:
    def check(*args: Any) -> Delayed[MaybeError[Val_]]:
        first = args[0]
        if isinstance(first, EvaluationError):
            return Delayed.complete(first)
        return fn(*args)
    return check

and then throughout the code, functions return Delayed[MaybeError[bool]] or (most often) Delayed[MaybeError[Any]] and there's a lot of f.transform(error_check(next_step)). That is, since we can't change what transform does, we wrap the transforming function into something that checks and propagates any errors without making any calls.

Of course, this means that we're calling isinstance(first, EvaluationError) a lot, which can't be cheap.

Outside the DML implementation, we typically either just print an error message and short-circuit as best we can (e.g., when we find we can't walk to a non-existent pad) or we raise an exception and let it happen wherever it happens.

Neither of these approaches is very useful, especially when (as with the drop motion errors), they could be due to user error rather than implementation error.

I'll outline my proposed solution next.

Migrated from internal repository. Originally created by @EvanKirshenbaum on Aug 03, 2023 at 4:31 PM PDT.

[EvanKirshenbaum]

My proposal is to simplify things by having the futures propagate error values.

Currently, Delayed object holds a tuple that indicates whether it holds a value and, if so, what that value is:

ValTuple = tuple[bool, T]

class Delayed(Generic[Tco]):
    _val: ValTuple[Tco] = (False, cast(Tco, None))

Instead, we can give ourselves a three-value state, perhaps something like

class _DelayedState(Enum):
    NO_VAL = auto()
    HAS_VAL = auto()
    HAS_ERROR = auto()

ValTuple = tuple[_DelayedState, T, RuntimeError]

class Delayed(Generic[Tco]):
    _val: ValTuple[Tco] = (_DelayedState.NO_VAL, cast(Tco, None), cast(RuntimeError, None))

Then, the logic would be that Postable would have a post_exception() method that posted a RuntimeException rather than a value, and all of the Delayed objects chained onto this one would get the exception propagated rather than calling the registered callbacks.

Note that this means that I'd have to change the callback list implementation slightly. Currently, it's just holds the functions, and they post to the created Postables internally, as

    def transformed(self, fn: Callable[[Tco], V]) -> Delayed[V]:
        future = Postable[V]()
        def post_transformed(val: Any) -> None:
            future.post(fn(val))
        self.when_value(post_transformed)
        return future

To allow short-circuiting on errors, when_value() should take an (optional) future. If there is such a future and an error is noted, the error gets posted there (and the callback is never called). If there isn't one, the callback is silently elided. I had been thinking that we could do this instead of the current transformation, but I know think it may make more sense to do it instead, and possibly even to keep the short-circuit futures on a separate list.

If we do this, then the only changes would seem to be that in when_value(), if we already have a value, we call the callback, and if we already have an error, we post it to the optional short-circuit postable. Postable.post() would pretty much stay the same other than the tuple added to _val and deleting the short-circuit list. And we would want a Postable.post_error(), which posts the error to any short-circuit futures and deletes both lists.

Delayed.wait() would need to be changed slightly to stop blocking on an error. This could involve a when_error() callback method and an error callbacks list.

Finally, value() would probably want to raise the exception if there is one.

Migrated from internal repository. Originally created by @EvanKirshenbaum on Aug 03, 2023 at 5:23 PM PDT.

[EvanKirshenbaum]

With this, it would seem likely that the DML code could get a lot cleaner, as we would presumably be able to get rid of (nearly?) all of the manual error checking and propagation. It would also probably be a lot more efficient, since we would no longer have to be continually doing isinstance checks.

Migrated from internal repository. Originally created by @EvanKirshenbaum on Aug 03, 2023 at 5:29 PM PDT.