GaxError is no longer raised in 0.13.0 when expected

[dhermes]

For example, when getting a non-existent topic we expect a GaxError but see a grpc._Rendezvous instead.

Related: googleapis/google-cloud-python#2336

[geigerj]

I can reproduce this:

$ virtualenv venv
$ . venv/bin/activate
(venv) $ pip install gapic-google-pubsub-v1
(venv) $ python
Python 2.7.12 (default, Aug 24 2016, 08:19:55) 
[GCC 4.8.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from google.cloud.gapic.pubsub.v1 import publisher_api as p_api
>>> api = p_api.PublisherApi()
>>> topic = api.topic_path('MY-PROJECT', 'my-topic')
>>> api.get_topic(topic)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/google/home/jgeiger/gax-test/venv/lib/python2.7/site-packages/google/cloud/gapic/pubsub/v1/publisher_api.py", line 291, in get_topic
    return self._get_topic(request, options)
  File "/usr/local/google/home/jgeiger/gax-test/venv/lib/python2.7/site-packages/google/gax/api_callable.py", line 480, in inner
    return api_caller(api_call, this_settings, request)
  File "/usr/local/google/home/jgeiger/gax-test/venv/lib/python2.7/site-packages/google/gax/api_callable.py", line 468, in base_caller
    return api_call(*args)
  File "/usr/local/google/home/jgeiger/gax-test/venv/lib/python2.7/site-packages/google/gax/api_callable.py", line 430, in inner
    return a_func(*args, **kwargs)
  File "/usr/local/google/home/jgeiger/gax-test/venv/lib/python2.7/site-packages/google/gax/api_callable.py", line 64, in inner
    return a_func(*updated_args, **kwargs)
  File "/usr/local/google/home/jgeiger/gax-test/venv/lib/python2.7/site-packages/grpc/_channel.py", line 481, in __call__
    return _end_unary_response_blocking(state, False, deadline)
  File "/usr/local/google/home/jgeiger/gax-test/venv/lib/python2.7/site-packages/grpc/_channel.py", line 432, in _end_unary_response_blocking
    raise _Rendezvous(state, None, None, deadline)
grpc._channel._Rendezvous: <_Rendezvous of RPC that terminated with (StatusCode.NOT_FOUND, Resource not found (resource=my-topic).)>

We should talk with the gRPC folks (@nathanielmanistaatgoogle et al). I'm somewhat uncomfortable with catching _Rendezvous because it's underscore-prefixed, and so not supposed to be public. I am surprised that gRPC is raising an exception with a non-public type.

[dhermes]

@geigerj Ditto. We have googleapis/google-cloud-python#2156 for that discussion as well.

[nathanielmanistaatgoogle]

@geigerj: gRPC is raising an exception with a non-public class, but the public types of the instance that is raised are grpc.RpcError and grpc.Call. Our intention is that application code be written something like

try:
    my_response = my_stub.MyMethod(my_request)
except grpc.RpcError as rpc_error:
    my_status_code = rpc_error.code()
    my_status_details = rpc_error.details()
    <statements taking action in response to the terminated RPC>

.

[dhermes]

@nathanielmanistaatgoogle But what you're actually forcing users to do is

try:
    foo()
except grpc.RpcError as grpc_err:
    if isinstance(grpc_err, grpc.Call):
        code = grpc_err.code()
    else:
        raise

(Already mentioned this in googleapis/google-cloud-python#2156)

[nathanielmanistaatgoogle]

@dhermes: I don't understand the need for the instance check?

[geigerj]

@nathanielmanistaatgoogle What is the relationship between RpcError and AbortionError? Is AbortionError ever still raised?

[nathanielmanistaatgoogle]

@geigerj: AbortionError is only ever raised from the Beta API; it is entirely replaced by grpc.RpcError in the GA API.

[geigerj]

@nathanielmanistaatgoogle @dhermes Correct me if I misunderstood, but I think @dhermes's concern comes from the fact that grpc.RpcError does not define any attributes; the code() method is defined on gRPC.Call. So to be safe, I would expect either to do the isinstance check or to wrap the call in a try/except block. There doesn't seem to be reason to expect from having a grpc.RpcError alone that it would have a code() method.

[dhermes]

/cc @tseaver

@nathanielmanistaatgoogle Maybe we should have a more centralized place for this discussion?

[nathanielmanistaatgoogle]

@dhermes: absolutely have that discussion, but if you'll indulge me I'd rather observe it than lead it. I intended for the API to be clear that the exception raised will implement both types and can (without having to check first) be used according to both types so I'd like to see where the ambiguity is creeping in.

[dhermes]

@nathanielmanistaatgoogle The ambiguity is that RpcError doesn't implement the core functionality needed for the exception to be useful.

You've now got feedback from two downstream libraries / three users that this pattern is not intuitive / makes us uncomfortable.

[bjwatson]

@geigerj @dhermes Is this fixed by #131? (which will be in the forthcoming google-gax 0.14.0 release)

[geigerj]

Yes, I have verified against the pubsub snippet referenced above in this issue with an install of GAX from source.

[bjwatson]

Thanks @geigerj!

[dhermes]

Didn't realize you guys released so fast, 0.14.1 already out there! I'll whip up a PR to restore our usage of GaxError instead of the _Rendezvous.

[nathanielmanistaatgoogle]

@dhermes:

One of the things that I think is successful about the gRPC Python API is that I think it affords a lot of behavioral complexity while being very simply expressed. I admire this property in other systems and made an explicit goal for this design. By “simple” I mostly mean the raw number of code elements (constants, functions, classes, and methods, albeit weighted differently) in the API but also factor in the expected complexity of code using the API. The principal means of pulling it off (both in gRPC Python and in the other systems I’ve seen) is to avoid having code elements repeat one another statically (for example by having a method that does something in one class and then having another method in another class, possibly with the same name, that does the same thing) but then having the system dynamically interact across its API boundary in terms of combinations of those simple elements. One way to describe those combinations is with multiple inheritance: an API can define non-trivial classes A, B, C, and D all with interesting methods and then also define a class AB that inherits from A and B and a class CD that inherits from C and D, but then that widens the API with those additional code elements. In a type system like Java’s those subclasses (technically subinterfaces) would be required, but in Python if we want to communicate about an object that implements two (or more) types, what I prefer to do is... communicate about an object that implements two (or more) types. Of course there’s judgement to be made in the dimension of friendliness: because it’s more hospitable to ask less of and do more for another party than the other way around, one should be more reluctant to ask one’s API’s users to pass an object of many types (or of a complex type) and more willing to provide to one’s API’s users objects of many types (or of complex types). This is why on the client side there is a means for gRPC Python to provide Future-implementing objects to applications but there’s no corresponding API on the server side for applications to provide Future-implementing objects to gRPC Python.

So gRPC Python passes across its API boundary, in the direction from itself to applications, objects that implement more than one type. In fact it has for a year because this was a property of the Beta API; that’s one reason why the matter coming up now has caught me off-guard.

Another is that I always thought that having an object that implements two types and using it according to one type, then the other type, and then the first type again was just ordinary bread-and-butter programming rather than anything particularly advanced or special. Consider the case of

my_response_and_call = my_stub.MyMethod.future(my_request)
my_initial_metadata = my_response_future_and_call.initial_metadata()
<do something of interest with that initial metadata>
my_response = my_response_future_and_call.result()
<do something of interest with that response>
my_terminal_metadata = my_response_future_and_call.trailing_metadata()
<do something of interest with that terminal metadata>

– is that really strange? Other than the long name which is only what it is for clarity of the illustration? It looks like the short, clear code that I want my users to write. And yet because metadata is a feature of both response-unary and response-streaming RPCs, if the metadata access methods were part of the Future interface they’d have to be duplicated in the interface of the iterator object returned for response-streaming RPCs. Right?

Now to RpcError: there are two reasons why it is empty. The first is that it doesn’t have anything to say that isn’t already said in Call. If that were all, RpcError could maybe extend Call. The second is that it’s also used on the service side of an RPC where things like code and details don’t exist. Given the choice between two exception classes one of which duplicates methods available elsewhere and one exception class that at some of the places it is raised is combined with another interface, I found the latter the more attractive choice. Fewer code elements after all.

One change that I’ve been wanting to make is defining a new class in _channel that is only both an RpcError and a Call rather than an RpcError, a Call, and the several other things that _Rendezvous is, giving that new class a much more clear string representation (since string representations are much more widely read by users than I had earlier understood), and raising an instance of that new class instead. Do you agree that that would be an improvement? How much of an improvement?

Thank you for being clear about how strongly you reacted to this part of the API. Now that it’s been a few days, has your feeling mellowed at all? Having had it to think about, has it become any more familiar or comfortable? Have you found any portion of your discomfort to have come from not having seen an API do that before?

[dhermes]

Do you agree that that would be an improvement? How much of an improvement?

We don't have any problem with the _Rendezvous class, except it isn't public.

[dhermes]

Most of the virtues you extol above are about composability. I don't have any beef with this class inheriting from 3 (or 10) classes. The issue is that there is no single public class which represents the public exception interface.