Test regression when grpcio is installed but grpcio-status is not

[mgorny]

CC @atulep

The following commit causes a regression in the test suite:

commit ef6f0fcfdfe771172056e35e3c990998b3b00416
Author: Aza Tulepbergenov <[email protected]>
Date:   2021-10-19 20:17:49 +0200

    feat: add 'GoogleAPICallError.error_details' property (#286)
    
    Based on 'google.rpc.status.details'.

Prior to this commit, the package handled both missing and present grpc consistently. However, the following condition in exceptions.py:

try:
    import grpc
    from grpc_status import rpc_status
except ImportError:  # pragma: NO COVER
    grpc = None
    rpc_status = None

means that if grpc_status module is not installed, the exception module behaves as if grpc wasn't present even though other modules (notably tests) behave as if it was present. As a result, people having one but not the other module installed get a bunch of test failures because grpc-based tests are run but e.g. the exception module's from_grpc_error() function fails since grpc is forced to None.

This is especially problematic since grpcio-status is a new optional dependency, and since it's not packaged on Gentoo, we'd effectively have to force people to uninstall grpcio.

Environment details

• OS type and version: Gentoo Linux
• Python version: python --version 3.8.12, 3.9.7
• pip version: pip --version n/a
• google-api-core version: pip show google-api-core 2.2.0, 2.2.1, git master (d2a729e)

Steps to reproduce

1. git clone https://github.com/googleapis/python-api-core
2. cd python-api-core
3. python3.9 -m venv .venv
4. . .venv/bin/activate
5. pip install . grpcio pytest mock proto-plus pytest-asyncio
6. pytest

Stack trace

Example test failure:

_______________________________________________________ test_wrap_unary_errors ________________________________________________________

self = <google.api_core.grpc_helpers_async._WrappedUnaryUnaryCall object at 0x7fe0e3de3790>

    def __await__(self):
        try:
>           response = yield from self._call.__await__()

.venv/lib/python3.9/site-packages/google/api_core/grpc_helpers_async.py:84: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

_mock_self = <AsyncMock id='140603872279952'>, args = (1, 2), kwargs = {'three': 'four'}, self = <AsyncMock id='140603872279952'>
_call = call(1, 2, three='four'), effect = RpcErrorImpl()

    async def _execute_mock_call(_mock_self, *args, **kwargs):
        self = _mock_self
        # This is nearly just like super(), except for special handling
        # of coroutines
    
        _call = _Call((args, kwargs), two=True)
        self.await_count += 1
        self.await_args = _call
        self.await_args_list.append(_call)
    
        effect = self.side_effect
        if effect is not None:
            if _is_exception(effect):
>               raise effect
E               tests.asyncio.test_grpc_helpers_async.RpcErrorImpl

.venv/lib/python3.9/site-packages/mock/mock.py:2169: RpcErrorImpl

During handling of the above exception, another exception occurred:

    @pytest.mark.asyncio
    async def test_wrap_unary_errors():
        grpc_error = RpcErrorImpl(grpc.StatusCode.INVALID_ARGUMENT)
        callable_ = mock.AsyncMock(spec=["__call__"], side_effect=grpc_error)
    
        wrapped_callable = grpc_helpers_async._wrap_unary_errors(callable_)
    
        with pytest.raises(exceptions.InvalidArgument) as exc_info:
>           await wrapped_callable(1, 2, three="four")

tests/asyncio/test_grpc_helpers_async.py:57: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.venv/lib/python3.9/site-packages/google/api_core/grpc_helpers_async.py:87: in __await__
    raise exceptions.from_grpc_error(rpc_error) from rpc_error
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

rpc_exc = RpcErrorImpl()

    def from_grpc_error(rpc_exc):
        """Create a :class:`GoogleAPICallError` from a :class:`grpc.RpcError`.
    
        Args:
            rpc_exc (grpc.RpcError): The gRPC error.
    
        Returns:
            GoogleAPICallError: An instance of the appropriate subclass of
                :class:`GoogleAPICallError`.
        """
        # NOTE(lidiz) All gRPC error shares the parent class grpc.RpcError.
        # However, check for grpc.RpcError breaks backward compatibility.
>       if isinstance(rpc_exc, grpc.Call) or _is_informative_grpc_error(rpc_exc):
E       AttributeError: 'NoneType' object has no attribute 'Call'

.venv/lib/python3.9/site-packages/google/api_core/exceptions.py:532: AttributeError
======================================================= short test summary info =======================================================
FAILED tests/asyncio/test_grpc_helpers_async.py::test_wrap_unary_errors - AttributeError: 'NoneType' object has no attribute 'Call'

[busunkim96]

Hi @mgorny,

Thanks for opening an issue. The intent was that grpcio-status would always be installed next to grpcio (see how the extra is written in setup.py) so I don't think we considered this particular scenario.

python-api-core/setup.py

Lines 38 to 42 in d2a729e

	extras = {
	"grpc": ["grpcio >= 1.33.2, < 2.0dev", "grpcio-status >= 1.33.2, < 2.0dev"],
	"grpcgcp": "grpcio-gcp >= 0.2.2",
	"grpcio-gcp": "grpcio-gcp >= 0.2.2",
	}

That said, I don't think separating the imports into two try/except blocks will make the code terribly more complicated.

This is especially problematic since grpcio-status is a new optional dependency, and since it's not packaged on Gentoo, we'd effectively have to force people to uninstall grpcio.

Could you go into more details on this? Is it difficult to add grpcio-status?

[mgorny]

The point is more like there's no clean way of expressing "if you have grpcio installed, install grpcio-status as well" from the point of python-api-core. We could technically make grpcio always install grpcio-status but if that was the right thing to do, why are they separate packages in the first place?

That said, I'm not maintaining grpcio in Gentoo and i have no interest in it. From my perspective, it's just an optional dependency that now started causing trouble.

[liu-du]

Experiencing the same issue indirectly with google pubsub client, the stacktrace is:

2022-01-31 00:34:16 INFO google.cloud.pubsub_v1.subscriber._protocol.streaming_pull_manager Observed non-terminating stream error 'NoneType' object has no attribute 'Call'
2022-01-31 00:34:16 INFO google.cloud.pubsub_v1.subscriber._protocol.streaming_pull_manager Observed non-recoverable stream error 'NoneType' object has no attribute 'Call'
2022-01-31 00:34:16 ERROR google.api_core.bidi Thread-ConsumeBidirectionalStream caught unexpected exception 'NoneType' object has no attribute 'Call' and will exit.
Traceback (most recent call last):
  File "/opt/atlassian/pipelines/agent/build/discovery-client/.tox/py36-bravado10-googleoauth03/lib/python3.6/site-packages/google/api_core/grpc_helpers.py", line 106, in __next__
    return next(self._wrapped)
  File "/opt/atlassian/pipelines/agent/build/discovery-client/.tox/py36-bravado10-googleoauth03/lib/python3.6/site-packages/grpc/_channel.py", line 426, in __next__
    return self._next()
  File "/opt/atlassian/pipelines/agent/build/discovery-client/.tox/py36-bravado10-googleoauth03/lib/python3.6/site-packages/grpc/_channel.py", line 809, in _next
    raise self
grpc._channel._MultiThreadedRendezvous: <_MultiThreadedRendezvous of RPC that terminated with:
	status = StatusCode.CANCELLED
	details = "Locally cancelled by application!"
	debug_error_string = "None"
>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/atlassian/pipelines/agent/build/discovery-client/.tox/py36-bravado10-googleoauth03/lib/python3.6/site-packages/google/api_core/bidi.py", line 655, in _thread_main
    response = self._bidi_rpc.recv()
  File "/opt/atlassian/pipelines/agent/build/discovery-client/.tox/py36-bravado10-googleoauth03/lib/python3.6/site-packages/google/api_core/bidi.py", line 561, in recv
    return self._recoverable(self._recv)
  File "/opt/atlassian/pipelines/agent/build/discovery-client/.tox/py36-bravado10-googleoauth03/lib/python3.6/site-packages/google/api_core/bidi.py", line 520, in _recoverable
    raise exc
  File "/opt/atlassian/pipelines/agent/build/discovery-client/.tox/py36-bravado10-googleoauth03/lib/python3.6/site-packages/google/api_core/bidi.py", line 504, in _recoverable
    return method(*args, **kwargs)
  File "/opt/atlassian/pipelines/agent/build/discovery-client/.tox/py36-bravado10-googleoauth03/lib/python3.6/site-packages/google/api_core/bidi.py", line 558, in _recv
    return next(call)
  File "/opt/atlassian/pipelines/agent/build/discovery-client/.tox/py36-bravado10-googleoauth03/lib/python3.6/site-packages/google/api_core/grpc_helpers.py", line 109, in __next__
    raise exceptions.from_grpc_error(exc) from exc
  File "/opt/atlassian/pipelines/agent/build/discovery-client/.tox/py36-bravado10-googleoauth03/lib/python3.6/site-packages/google/api_core/exceptions.py", line 592, in from_grpc_error
    if isinstance(rpc_exc, grpc.Call) or _is_informative_grpc_error(rpc_exc):
AttributeError: 'NoneType' object has no attribute 'Call'
2022-01-31 00:34:16 ERROR root Exception in callback <bound method ResumableBidiRpc._on_call_done of <google.api_core.bidi.ResumableBidiRpc object at 0x7ffb838c8d68>>: AttributeError("'NoneType' object has no attribute 'Call'",)

[ddoskind]

Thanks for taking a look at this!

I've noticed that while the fix in #355 is helpful in figuring out what's going on, consumer libraries (such as google-ads-python in our specific case) will still crash with an AttributeError whenever a gRPC error occurs.

Could the relevant code here:

python-api-core/google/api_core/exceptions.py

Lines 600 to 611 in 112049e

	if isinstance(rpc_exc, grpc.Call) or _is_informative_grpc_error(rpc_exc):
	details, err_info = _parse_grpc_error_details(rpc_exc)
	return from_grpc_status(
	rpc_exc.code(),
	rpc_exc.details(),
	errors=(rpc_exc,),
	details=details,
	response=rpc_exc,
	error_info=err_info,
	)
	else:
	return GoogleAPICallError(str(rpc_exc), errors=(rpc_exc,), response=rpc_exc)

be adjusted with a guard to first check if grpc.Call actually exists, falling through to just returning an "generic" GoogleAPICallError if that's not the case?