Backward incompatible change: _DEFAULT_VALUE should be None and not object() in PollingFuture

[gbmarc1]

A refactor last month introduced the global _DEFAULT_VALUE in the PollingFuture class ->
here.
This change is backward incompatible as some libraries (ex: google-cloud-bigquery) are based on the fact that the default value is None and not object().
For example, the typeguard here becomes invalid and enters which results in a error on this line.

TypeError: unsupported operand type(s) for -: 'object' and 'float'

Stack trace

/usr/local/lib/python3.9/site-packages/feast/infra/offline_stores/bigquery.py:583: in block_until_done
--
  | if bq_job.exception():
  | /usr/local/lib/python3.9/site-packages/google/api_core/future/polling.py:282: in exception
  | self._blocking_poll(timeout=timeout)
  | /usr/local/lib/python3.9/site-packages/google/cloud/bigquery/job/query.py:1245: in _blocking_poll
  | super(QueryJob, self)._blocking_poll(timeout=timeout, **kwargs)
  | /usr/local/lib/python3.9/site-packages/google/api_core/future/polling.py:137: in _blocking_poll
  | polling(self._done_or_raise)(retry=retry)
  | /usr/local/lib/python3.9/site-packages/google/api_core/retry.py:349: in retry_wrapped_func
  | return retry_target(
  | /usr/local/lib/python3.9/site-packages/google/api_core/retry.py:191: in retry_target
  | return target()
  | /usr/local/lib/python3.9/site-packages/google/cloud/bigquery/job/query.py:1358: in _done_or_raise
  | self._reload_query_results(retry=retry, timeout=transport_timeout)
  | _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
  |  
  | self = QueryJob<project=..., location=US, id=...>
  | retry = None, timeout = <object object at 0x7fa46677c5c0>
  |  
  | def _reload_query_results(
  | self, retry: "retries.Retry" = DEFAULT_RETRY, timeout: float = None
  | ):
  | """Refresh the cached query results.
  |  
  | Args:
  | retry (Optional[google.api_core.retry.Retry]):
  | How to retry the call that retrieves query results.
  | timeout (Optional[float]):
  | The number of seconds to wait for the underlying HTTP transport
  | before using ``retry``.
  | """
  | if self._query_results and self._query_results.complete:
  | return
  |  
  | # Since the API to getQueryResults can hang up to the timeout value
  | # (default of 10 seconds), set the timeout parameter to ensure that
  | # the timeout from the futures API is respected. See:
  | # https://github.com/GoogleCloudPlatform/google-cloud-python/issues/4135
  | timeout_ms = None
  | if self._done_timeout is not None:
  | # Subtract a buffer for context switching, network latency, etc.
  | >           api_timeout = self._done_timeout - _TIMEOUT_BUFFER_SECS
  | E           TypeError: unsupported operand type(s) for -: 'object' and 'float'
  |  
  | /usr/local/lib/python3.9/site-packages/google/cloud/bigquery/job/query.py:1329: TypeError

Thanks!

[vam-google]

The default value cannot be None and the previous behavior of api-core was a bug. The gist of it is the need to distinguish between infinite timeout (None) and not specified timeout (_DEFAULT_TIMEOUT).

The bigquery library (https://github.com/googleapis/python-bigquery/blob/b8502a6641b653610643aeb38992d330823feb94/google/cloud/bigquery/job/query.py#L1327) fails on the type of its own declared variable _done_timeout. If its value is an indirectly affected by a change in PollingFuture.result() from api-core than please fix it in bigquery library not in api-core (where this issue is currenlty opened).

The #462 was written with backward compatibilty in mind, but it is done in Python (not strongly typed language) and it was fixing a completely broken logic in api-core on multiple levels. The code is still expected to work fine and backward-compatilbe in standard scenarios, but your case is special: it extend the polling future behavior and introduces and assumption that timeout value is always numeric. The fact that timeout was specified timeout (int): in documentation of api-core is a bug, as the rest of GAPIC libraries infrastrucuture uses _DEFAULT object marker to specify default values.

Making timeout = None would break other things (i.e. we will fix you by breaking others), making DEFAULT marker a numeric value instead of object would be even worse, as logic like yours would assume that a default numeric marker (lets say some very big integer value) is not a "default" value but an actual timeout number specified by a user (i.e. that if self._done_timeout is not None will always be true even when it shouldn't).

[vam-google]

I'm not in the gapic team anymore, so I can't do edits anymore, but please strongly consider reassigning this issue to the python-bigquery repository and make sure that python-bigquery manual layer works on top of the recent api-core changes.

[gbmarc1]

Thanks @vam-google, I have created this issue in bigquery.

[atulep]

Closing this down as the issue was reassigned to bigquery.