Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: use correct typing for retries #1026

Merged
merged 6 commits into from
Oct 13, 2021
Merged

fix: use correct typing for retries #1026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
67634ad
fix: use correct typing for retries
tseaver Oct 13, 2021
19ea487
fix: overlooked golden change
tseaver Oct 13, 2021
4ae06f1
fix: autopep8
tseaver Oct 13, 2021
b0590b5
fix: one more overlooked golden change
tseaver Oct 13, 2021
8dafd46
fix: remove typing from base transport 'operations_client'
tseaver Oct 13, 2021
9ce843c
fix: moar typing fixes for transport 'operations_client' properties
tseaver Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ from google.auth.transport.grpc import SslCredentials # type: ignore
from google.auth.exceptions import MutualTLSChannelError # type: ignore
from google.oauth2 import service_account # type: ignore

OptionalRetry = Union[retries.Retry, object]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be equivalent to Any type?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to do some gymnastics to get a default sentinel type checkable in the BQ library:

https://github.com/googleapis/python-bigquery/blob/f83c00acead70fc0ce9959eefb133a672d816277/google/cloud/bigquery/schema.py#L52-L65

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I do know that gapic_v1.method.DEFAULT_RETRY is just an instance of object.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. That's what I mean. object is a very generic type. Basically everything is object, so union with that is basically Any.

Would folks be open to making a more explicit type for the default sentinel like we did in BQ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test if it is equivalent to Any, we could try passing a string as a retry argument. It ideally should fail type checks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe DEFAULT_RETRY is a specific object used as a canary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we update api-core to provide a different type for the marker object, then we can tweak the templates to use it -- otherwise, the typechecker hates that we are using that specific object as the default when we say the parameter is a retries.Retry.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Well I suspect this makes the type checker happy with any object, which isn't what we want. Did you see the pattern I used in google-cloud-bigquery? That pattern uses the fact that enums can be type checked to look for a specific object.


{% filter sort_lines %}
{% for method in service.methods.values() %}
{% for ref_type in method.flat_ref_types %}
Expand Down Expand Up @@ -306,7 +308,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
requests: Iterator[{{ method.input.ident }}] = None,
*,
{% endif %}
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
{% if not method.server_streaming %}
Expand Down
2 changes: 1 addition & 1 deletion gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/transports/base.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class {{ service.name }}Transport(metaclass=abc.ABCMeta):
{% if service.has_lro %}

@property
def operations_client(self) -> operations_v1.OperationsClient:
def operations_client(self):
"""Return the client designed to process long-running operations."""
raise NotImplementedError
{% endif %}
Expand Down
12 changes: 7 additions & 5 deletions gapic/templates/%namespace/%name_%version/%sub/services/%service/async_client.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@ import pkg_resources
import warnings
{% endif %}

import google.api_core.client_options as ClientOptions # type: ignore
from google.api_core.client_options import ClientOptions # type: ignore
from google.api_core import exceptions as core_exceptions # type: ignore
from google.api_core import gapic_v1 # type: ignore
from google.api_core import retry as retries # type: ignore
from google.auth import credentials as ga_credentials # type: ignore
from google.oauth2 import service_account # type: ignore

OptionalRetry = Union[retries.Retry, object]

{% filter sort_lines %}
{% for method in service.methods.values() %}
{% for ref_type in method.flat_ref_types %}
Expand Down Expand Up @@ -156,7 +158,7 @@ class {{ service.async_client_name }}:
requests: AsyncIterator[{{ method.input.ident }}] = None,
*,
{% endif %}
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
{% if not method.server_streaming %}
Expand Down Expand Up @@ -331,7 +333,7 @@ class {{ service.async_client_name }}:
self,
request: iam_policy_pb2.SetIamPolicyRequest = None,
*,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> policy_pb2.Policy:
Expand Down Expand Up @@ -439,7 +441,7 @@ class {{ service.async_client_name }}:
self,
request: iam_policy_pb2.GetIamPolicyRequest = None,
*,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> policy_pb2.Policy:
Expand Down Expand Up @@ -548,7 +550,7 @@ class {{ service.async_client_name }}:
self,
request: iam_policy_pb2.TestIamPermissionsRequest = None,
*,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> iam_policy_pb2.TestIamPermissionsResponse:
Expand Down
10 changes: 6 additions & 4 deletions gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ from google.auth.transport.grpc import SslCredentials # type: ignore
from google.auth.exceptions import MutualTLSChannelError # type: ignore
from google.oauth2 import service_account # type: ignore

OptionalRetry = Union[retries.Retry, object]

{% filter sort_lines %}
{% for method in service.methods.values() %}
{% for ref_type in method.flat_ref_types %}
Expand Down Expand Up @@ -322,7 +324,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
requests: Iterator[{{ method.input.ident }}] = None,
*,
{% endif %}
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
{% if not method.server_streaming %}
Expand Down Expand Up @@ -495,7 +497,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
self,
request: iam_policy_pb2.SetIamPolicyRequest = None,
*,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> policy_pb2.Policy:
Expand Down Expand Up @@ -605,7 +607,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
self,
request: iam_policy_pb2.GetIamPolicyRequest = None,
*,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> policy_pb2.Policy:
Expand Down Expand Up @@ -716,7 +718,7 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta):
self,
request: iam_policy_pb2.TestIamPermissionsRequest = None,
*,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> iam_policy_pb2.TestIamPermissionsResponse:
Expand Down
2 changes: 1 addition & 1 deletion gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/base.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ class {{ service.name }}Transport(abc.ABC):
{% if service.has_lro %}

@property
def operations_client(self) -> operations_v1.OperationsClient:
def operations_client(self):
"""Return the client designed to process long-running operations."""
raise NotImplementedError()
{% endif %}
Expand Down
2 changes: 1 addition & 1 deletion gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/grpc.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class {{ service.name }}GrpcTransport({{ service.name }}Transport):
self._ssl_channel_credentials = ssl_channel_credentials
self._stubs: Dict[str, Callable] = {}
{% if service.has_lro %}
self._operations_client = None
self._operations_client: Optional[operations_v1.OperationsClient] = None
{% endif %}

if api_mtls_endpoint:
Expand Down
2 changes: 1 addition & 1 deletion .../templates/%namespace/%name_%version/%sub/services/%service/transports/grpc_asyncio.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ class {{ service.grpc_asyncio_transport_name }}({{ service.name }}Transport):
self._ssl_channel_credentials = ssl_channel_credentials
self._stubs: Dict[str, Callable] = {}
{% if service.has_lro %}
self._operations_client = None
self._operations_client: Optional[operations_v1.OperationsAsyncClient] = None
{% endif %}

if api_mtls_endpoint:
Expand Down
6 changes: 4 additions & 2 deletions gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ from google.api_core import path_template # type: ignore
from google.api_core import gapic_v1 # type: ignore
from google.api_core import operations_v1
from requests import __version__ as requests_version
from typing import Callable, Dict, Optional, Sequence, Tuple
from typing import Callable, Dict, Optional, Sequence, Tuple, Union
import warnings

OptionalRetry = Union[retries.Retry, object]
{% extends '_base.py.j2' %}

{% block content %}
Expand Down Expand Up @@ -153,7 +155,7 @@ class {{service.name}}RestTransport({{service.name}}Transport):
{%- if method.http_options and not method.lro and not (method.server_streaming or method.client_streaming) %}
def _{{method.name | snake_case}}(self,
request: {{method.input.ident}}, *,
retry: retries.Retry=gapic_v1.method.DEFAULT,
retry: OptionalRetry=gapic_v1.method.DEFAULT,
timeout: float=None,
metadata: Sequence[Tuple[str, str]]=(),
) -> {{method.output.ident}}:
Expand Down
28 changes: 15 additions & 13 deletions tests/integration/goldens/asset/google/cloud/asset_v1/services/asset_service/async_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@
from typing import Dict, Sequence, Tuple, Type, Union
import pkg_resources

import google.api_core.client_options as ClientOptions # type: ignore
from google.api_core.client_options import ClientOptions # type: ignore
from google.api_core import exceptions as core_exceptions # type: ignore
from google.api_core import gapic_v1 # type: ignore
from google.api_core import retry as retries # type: ignore
from google.auth import credentials as ga_credentials # type: ignore
from google.oauth2 import service_account # type: ignore

OptionalRetry = Union[retries.Retry, object]

from google.api_core import operation # type: ignore
from google.api_core import operation_async # type: ignore
from google.cloud.asset_v1.services.asset_service import pagers
Expand Down Expand Up @@ -153,7 +155,7 @@ def __init__(self, *,
async def export_assets(self,
request: asset_service.ExportAssetsRequest = None,
*,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> operation_async.AsyncOperation:
Expand Down Expand Up @@ -233,7 +235,7 @@ async def list_assets(self,
request: asset_service.ListAssetsRequest = None,
*,
parent: str = None,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> pagers.ListAssetsAsyncPager:
Expand Down Expand Up @@ -321,7 +323,7 @@ async def list_assets(self,
async def batch_get_assets_history(self,
request: asset_service.BatchGetAssetsHistoryRequest = None,
*,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> asset_service.BatchGetAssetsHistoryResponse:
Expand Down Expand Up @@ -387,7 +389,7 @@ async def create_feed(self,
request: asset_service.CreateFeedRequest = None,
*,
parent: str = None,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> asset_service.Feed:
Expand Down Expand Up @@ -475,7 +477,7 @@ async def get_feed(self,
request: asset_service.GetFeedRequest = None,
*,
name: str = None,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> asset_service.Feed:
Expand Down Expand Up @@ -563,7 +565,7 @@ async def list_feeds(self,
request: asset_service.ListFeedsRequest = None,
*,
parent: str = None,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> asset_service.ListFeedsResponse:
Expand Down Expand Up @@ -647,7 +649,7 @@ async def update_feed(self,
request: asset_service.UpdateFeedRequest = None,
*,
feed: asset_service.Feed = None,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> asset_service.Feed:
Expand Down Expand Up @@ -729,7 +731,7 @@ async def delete_feed(self,
request: asset_service.DeleteFeedRequest = None,
*,
name: str = None,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> None:
Expand Down Expand Up @@ -805,7 +807,7 @@ async def search_all_resources(self,
scope: str = None,
query: str = None,
asset_types: Sequence[str] = None,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> pagers.SearchAllResourcesAsyncPager:
Expand Down Expand Up @@ -991,7 +993,7 @@ async def search_all_iam_policies(self,
*,
scope: str = None,
query: str = None,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> pagers.SearchAllIamPoliciesAsyncPager:
Expand Down Expand Up @@ -1155,7 +1157,7 @@ async def search_all_iam_policies(self,
async def analyze_iam_policy(self,
request: asset_service.AnalyzeIamPolicyRequest = None,
*,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> asset_service.AnalyzeIamPolicyResponse:
Expand Down Expand Up @@ -1217,7 +1219,7 @@ async def analyze_iam_policy(self,
async def analyze_iam_policy_longrunning(self,
request: asset_service.AnalyzeIamPolicyLongrunningRequest = None,
*,
retry: retries.Retry = gapic_v1.method.DEFAULT,
retry: OptionalRetry = gapic_v1.method.DEFAULT,
timeout: float = None,
metadata: Sequence[Tuple[str, str]] = (),
) -> operation_async.AsyncOperation:
Expand Down
Loading