[python] Subclassing RpcError

[aa...@dropbox.com]

Hi,

I had some questions about the api design of RpcError. From what I can tell in docs I should catch RpcError and switch on code (since it's also implicitly a `Call`). A few things frustrate me about this.

1. There are certain codes I'd like to treat one way and certain codes I'd like to treat another way. It seems slightly more pythonic to have one exception class per code, especially given the number of codes is fixed and the python client will treat any code that is not one of the known status codes as "unknown". This would let me catch the specific codes I care about by `excepting` only the classes that represent those codes, rather than catching all RpcError, switching, and reraising otherwise.
2. The api of `_Rendezvous` is pretty large, and we're told to count on behavior that is implicitly there as a result of it being a `Call`, rather that an api where we can catch an `GrpcException` that has a base api of methods that are available and make sense to call on the exception. I'm not sure which methods out of Call/Future make sense to call when I catch the RpcError (that is actually a _Rendezvous). Right now I use `code` and `details` and the `metadata`. I think changing this would also help toward the goal of "Support static type-checking of both gRPC Python itself and of code that uses gRPC Python" mentioned in the gsoc list. 

In my opinion, it makes more sense that instead of the opaque RpcError, grpc python exposed a exception class per code that derived from a base class that actually had a meaningful set of methods to call (rather than inheriting most of the future methods that don't make sense for me to call on the error). I was wondering if I'm missing something about idiomatically handling errors on the client, and what went into the current design of the exception api.

Thanks,

Ashwin

[Nathaniel Manista]

On Mon, Feb 26, 2018 at 11:16 AM, aamit via grpc.io <grp...@googlegroups.com> wrote:

I had some questions about the api design of RpcError.

You're not alone; it's a part of the gRPC Python API that's somewhat different than how APIs are often shaped in Python (we knew this all along) and with which users have had some confusion (more than we expected!).

From what I can tell in docs I should catch RpcError and switch on code (since it's also implicitly a `Call`).

Not all grpc.RpcError instances are also grpc.Call instances, but many are. Speaking just for those that are: sure, switch on the value of the code if that's what you want to do, but you don't have to do so.

A few things frustrate me about this.

1. There are certain codes I'd like to treat one way and certain codes I'd like to treat another way. It seems slightly more pythonic to have one exception class per code,

It seems code-bloaty to have one exception class per code! We believe that classes that have no behavioral differences (for example: ever see a subclass that contains no method definitions but rather only a constructor that calls its superclass' constructor?) are a code smell. Exception classes are a bit of a special case because they're mostly behaviorless anyway, but they're not so special that different data values deserve different class definitions.

1. especially given the number of codes is fixed

Fun fact: the set of status codes is (at least in theory) not fixed. Additional codes are supposed to be able to be added one day without breaking working code. This may or may not ever happen, but it was a concern at the time we were designing the gRPC Python API.

1. and the python client will treat any code that is not one of the known status codes as "unknown". This would let me catch the specific codes I care about by `excepting` only the classes that represent those codes, rather than catching all RpcError, switching, and reraising otherwise.

One of the reasons this wasn't a priority for us in the API design was that we figured that if there were some common behavior that you wanted to be done for seven or eight different status codes, you would write a helper function and call it from inside your except:. If there were different Exception classes for each status code... would you be writing

except (<first chosen Exception class>, <five or six others>, <last chosen Exception class>,) as my_exception:

  <common behavior using <my_exception>

? Or are you saying something else, that you'd want seven or eight different except: syntax elements?

1. The api of `_Rendezvous` is pretty large,

I'm glad you wrote this as you did, because the response is the exact opposite: the API of _Rendezvous is pretty small, because it is zero, because _Rendezvous is a private code element not even visible to applications. (Yes, it shows its name in stack traces when something goes wrong, but that's to aid debugging only.)

1. and we're told to count on behavior that is implicitly there as a result of it being a `Call`,

It's not implicit; it's explicitly right there in the API specification. Count on it. It is guaranteed as much as any other part of the API.

Let's step away from gRPC Python for a moment: consider in otherwise complete isolation a function that takes no arguments and returns an object that is a sequence of length-one strings. You're perfectly well able to write code that uses this function, right? It doesn't matter that sequence is an abstract type, right? You don't need to know the class of the returned value, right? The usability of the function comes from the way that it guarantees that the returned value is usable according to a specified type. The class of the returned value could be tuple, or list, or some private sequence implementation ("_RendezSequenceOfLengthOneStrings" :-P). It could even be string (since for better or worse and for all the trouble it's caused over the years in Python strings happen to be sequences of length-one strings). This isn't controversial or challenging; this is foundational abstraction that's pretty universal throughout all software.

Now back to gRPC Python: we did two things that in combination have thrown for a loop folks that hadn't seen them before:

1) Rather than an abstract return value, implemented by some private internal class, we have methods with an abstract raises value, implemented by some private internal class. This may be rare, but it shouldn't be strange. Imagine that the Python interpreter were changed one day so that when you made an out-of-bounds access on a Tuple, what was raised was not an IndexError but rather a private subclass of IndexError. This might be curious, but it shouldn't break anything; all your code that says "except IndexError:" should still work just as it worked before. What drives this is the Liskov Substitution Principle: your code should not depend in any way upon superclasses being implementing classes and break when instances of subclasses are provided in places where you thought you were getting instances of superclasses.

2) Rather than providing one type according to which an object passed from gRPC Python to applications may be used, we (at some points in the API) provide two types according to which an object passed from gRPC Python to applications may be used. I didn't think at the time that I was starting a revolution but... that's kind of how it's played out among some users. But there's also nothing conceptually difficult or challenging about this either. The fact that the object is a grpc.RpcError is what allows applications to catch it. The fact that the object is a grpc.Call is what allows applications to do interesting things with it after having caught it. The sum of those two offers all the functionality that is needed, right? (When you talk about a hierarchy of Exception classes, this is about making something more convenient than it is today, right? Not making possible something that today is impossible?)

1. rather that an api where we can catch an `GrpcException` that has a base api of methods that are available and make sense to call on the exception. I'm not sure which methods out of Call/Future make sense to call when I catch the RpcError (that is actually a _Rendezvous). Right now I use `code` and `details` and the `metadata`. I think changing this would also help toward the goal of "Support static type-checking of both gRPC Python itself and of code that uses gRPC Python" mentioned in the gsoc list.

In my opinion, it makes more sense that instead of the opaque RpcError,

grpc.RpcError isn't opaque; it's one of the few concrete classes exposed in the gRPC Python API. It defines no behavior, but it's there and it's an Exception subclass.

grpc python exposed a exception class per code that derived from a base class that actually had a meaningful set of methods to call (rather than inheriting most of the future methods that don't make sense for me to call on the error). I was wondering if I'm missing something about idiomatically handling errors on the client, and what went into the current design of the exception api.

If you haven't yet watched it I'd suggest looking at this talk; a lot of the ideas developed there motivated the gRPC Python API. Committing to maintaining a stable API over the course of several years to come was also at the forefront of our minds and manifested as design pressures against concrete classes in the API (though there are a few) and against introducing code elements that were merely the combination of other code elements (we needed grpc.RpcError, and we needed grpc.Call, but we decided that we did not need a third thing that was nothing but both a grpc.RpcError and a grpc.Call). Both of those push toward saying "no" to a subclass-per-status-code Exception hierarchy: we already have the set of status codes expressed in the API and we already have Exceptions expressed in the API; we're reluctant to add additional expressions of the same concepts. Yes, there might be some convenience to be gained (and there are lots of places in the API where we have done things for application convenience!) but not so much in this case that we think it's worth it.

-Nathaniel

[aa...@dropbox.com]

Hey Nathan,

Thanks for taking the time for the detailed answer. I'm still not entirely convinced by your points. To respond to your points and try and clarify what I wrote:

> (When you talk about a hierarchy of Exception classes, this is about making something more convenient than it is today, right? Not making possible something that today is impossible?)

Yes. As such, it's not a blocker for me that things are the way they are and I don't want to bikeshed my opinions too heavily here if you disagree with the points I raise below.

> It seems code-bloaty to have one exception class per code! We believe that classes that have no behavioral differences (for example: ever see a subclass that contains no method definitions but rather only a constructor that calls its superclass' constructor?) are a code smell. Exception classes are a bit of a special case because they're mostly behaviorless anyway, but they're not so special that different data values deserve different class definitions.

Normally I'd agree, but because of how exception handling works in Python (as well as other languages like Java), I think it is not uncommon to have behaviorless exception class e.g. https://github.com/requests/requests/blob/master/requests/exceptions.py. https://github.com/celery/celery/blob/master/celery/exceptions.py (picking 2 random popular open source packages). Of course, not every package does this.

> Fun fact: the set of status codes is (at least in theory) not fixed. Additional codes are supposed to be able to be added one day without breaking working code. This may or may not ever happen, but it was a concern at the time we were designing the gRPC Python API.

What I meant to convey was that from the client perspective, we already treat any code that is not known by the client as unknown. It seems that is intended behavior. I brought that up to show that if we had a class per code,  the client behavior of falling back to "UnknownGrpcError" class is already established.

> except (<first chosen Exception class>, <five or six others>, <last chosen Exception class>,) as my_exception:

Yes - if you had a few codes you want to handle the same way you could either specify the ones you care about or catch the base exception class and treat it as we currently do. The way to handle it now would be I think.

```

catch RpcError:

  if we care about code handle

  else raise

```

> grpc.RpcError isn't opaque; it's one of the few concrete classes exposed in the gRPC Python API. It defines no behavior, but it's there and it's an Exception subclass.

When I say it's opaque I mean that it could represent almost anything that went wrong with gRPC, and that it is has no defined behavior. I think this gets to your mention of how rather then having an inheritance chain for the behavior, the actual behavior is from an adjacent class from multiple inheritance. I do understand how treating a subclass as a base class is common and often preferred, but I'd argue that this multiple inheritance setup is different.

This gets into where it's strictly my opinion that it would be more clear if the behavior that I see most people expect on RpcError was part of its defined behavior. I understand that right now the docs make this guarantee, but from a perspective of statically typing this code it would be easier to make it clear that RpcError has methods I can call. I'd prefer not to rely on messages in the docs which can fall out of date.  Otherwise I think code that is like:

```

try:

  make grpc call

except RpcError as e:

  switch on e.code

```

which I see recommended as how to handle gRPC errros, should be written as below to be more defensive as:

```

try:

  make grpc call

except RpcError as e:

  if instanceof(e, Call):

    switch on e.code

```

If the convenience of this is not worth the cost of committing to these concrete classes I understand, but in my opinion having an exception chain that inherits from a base class like `RpcCallError(RpcError,Call` makes sense for python and doesn't make the API surface more fragile.

Thanks,