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.

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

fix(snippetgen): fix client streaming samples #1061

Merged
merged 7 commits into from
Nov 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -359,4 +359,4 @@ jobs:
python -m pip install autopep8
- name: Check diff
run: |
find gapic tests -name "*.py" -not -path 'tests/integration/goldens/*' | xargs autopep8 --diff --exit-code
find gapic tests -name "*.py" -not -path 'tests/**/goldens/*' | xargs autopep8 --diff --exit-code
4 changes: 2 additions & 2 deletions DEVELOPMENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ Execute unit tests by running one of the sessions prefixed with `unit-`.
- Lint sources by running `autopep8`. The specific command is the following.

```
find gapic tests -name "*.py" -not -path 'tests/integration/goldens/*' | xargs autopep8 --diff --exit-code
find gapic tests -name "*.py" -not -path 'tests/**/goldens/*' | xargs autopep8 --diff --exit-code
```

- Format sources in place:

```
find gapic tests -name "*.py" -not -path 'tests/integration/goldens/*' | xargs autopep8 --in-place
find gapic tests -name "*.py" -not -path 'tests/**/goldens/*' | xargs autopep8 --in-place
```

## Integration Tests
Expand Down
14 changes: 1 addition & 13 deletions gapic/samplegen/samplegen.py
Original file line number Diff line number Diff line change
Expand Up @@ -466,9 +466,7 @@ def validate_and_transform_request(

Raises:
InvalidRequestSetup: If a dict in the request lacks a "field" key,
a "value" key, if there is an unexpected keyword,
or if more than one base parameter is given for
a client-side streaming calling form.
a "value" key or if there is an unexpected keyword.
BadAttributeLookup: If a request field refers to a non-existent field
in the request message type.
ResourceRequestMismatch: If a request attempts to describe both
Expand Down Expand Up @@ -548,16 +546,6 @@ def validate_and_transform_request(
AttributeRequestSetup(**r_dup) # type: ignore
)

client_streaming_forms = {
types.CallingForm.RequestStreamingClient,
types.CallingForm.RequestStreamingBidi,
}

if len(base_param_to_attrs) > 1 and calling_form in client_streaming_forms:
raise types.InvalidRequestSetup(
"Too many base parameters for client side streaming form"
)

Comment on lines -551 to -560
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@software-dov 👋 I'm having trouble understanding this error, would you remember what this was written to prevent?

Some streaming method request messages have 2+ required fields (pubsub), so the current snippetgen logic adds two fields to the dictionary. That causes this error to be raised. Are fields expected to be set differently for streaming?

# Request passed to `validate_and_transform_request`
[
   {
      "field":"subscription",
      "value":"projects/{project}/subscriptions/{subscription}"
   },
   {
      "field":"stream_ack_deadline_seconds",
      "value":2813
   }
]

I see two unit tests that raise this error, but I don't see any pre-existing tests showing a valid sample config for a client streaming method.

def test_single_request_client_streaming(dummy_api_schema,
calling_form=types.CallingForm.RequestStreamingClient):
# Each API client method really only takes one parameter:
# either a single protobuf message or an iterable of protobuf messages.
# With unary request methods, python lets us describe attributes as positional
# and keyword parameters, which simplifies request construction.
# The 'base' in the transformed request refers to an attribute, and the
# 'field's refer to sub-attributes.
# Client streaming and bidirectional streaming methods can't use this notation,
# and generate an exception if there is more than one 'base'.
input_type = DummyMessage(
fields={
"cephalopod": DummyField(
message=DummyMessage(
fields={
"order": DummyField(
message=DummyMessage(type="ORDER_TYPE")
)
},
type="CEPHALOPOD_TYPE"
)
),
"gastropod": DummyField(
message=DummyMessage(
fields={
"order": DummyField(
message=DummyMessage(type="ORDER_TYPE")
)
},
type="GASTROPOD_TYPE"
)
)
},
type="MOLLUSC_TYPE"
)
v = samplegen.Validator(DummyMethod(input=input_type), dummy_api_schema)
with pytest.raises(types.InvalidRequestSetup):
v.validate_and_transform_request(
types.CallingForm.RequestStreamingClient,
[
{"field": "cephalopod.order", "value": "cephalopoda"},
{"field": "gastropod.order", "value": "pulmonata"},
],
)
def test_single_request_bidi_streaming():
test_single_request_client_streaming(
types.CallingForm.RequestStreamingBidi)

Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't remember what was going on here. My guess is that I was mildly incorrect about the actual python method signature convention and I was trying to imply that we only wanted a single request to be put into a list...?

TL;DR: It's probably a bug or at least a misreading of the spec on my part.

# We can only flatten a collection of request parameters if they're a
# subset of the flattened fields of the method.
flattenable = self.flattenable_fields >= set(base_param_to_attrs)
Expand Down
19 changes: 17 additions & 2 deletions gapic/templates/examples/feature_fragments.j2
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ with open({{ attr.input_parameter }}, "rb") as f:
client = {{ module_name }}.{{ client_name }}()
{% endmacro %}

{% macro render_request_setup(full_request, module_name, request_type) %}
{% macro render_request_setup(full_request, module_name, request_type, calling_form, calling_form_enum) %}
# Initialize request argument(s)
{% for parameter_block in full_request.request_list if parameter_block.body %}
{% if parameter_block.pattern %}
Expand All @@ -179,6 +179,21 @@ request = {{ module_name }}.{{ request_type.ident.name }}(
{{ parameter.base }}={{ parameter.base if parameter.body else parameter.single.value }},
{% endfor %}
)
{# Note: This template assumes only one request needs to be sent. When samples accept
configs the client streaming logic should be modified to allow 2+ request objects. #}
{# If client streaming, wrap the single request in a generator that produces 'requests' #}
{% if calling_form in [calling_form_enum.RequestStreamingBidi,
calling_form_enum.RequestStreamingClient] %}

# This method expects an iterator which contains
# '{{module_name}}.{{ request_type.ident.name }}' objects
# Here we create a generator that yields a single `request` for
# demonstrative purposes.
requests = [request]
def request_generator():
for request in requests:
yield request
{% endif %}
{% endif %}
{% endmacro %}

Expand Down Expand Up @@ -219,7 +234,7 @@ await{{ " "}}
{%- endif -%}
{% if calling_form in [calling_form_enum.RequestStreamingBidi,
calling_form_enum.RequestStreamingClient] %}
client.{{ sample.rpc|snake_case }}([{{ render_request_params(sample.request.request_list)|trim }}])
client.{{ sample.rpc|snake_case }}(requests=request_generator())
{% else %}{# TODO: deal with flattening #}
{# TODO: set up client streaming once some questions are answered #}
client.{{ sample.rpc|snake_case }}({{ render_request_params_unary(sample.request)|trim }})
Expand Down
2 changes: 1 addition & 1 deletion gapic/templates/examples/sample.py.j2
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ from {{ sample.module_namespace|join(".") }} import {{ sample.module_name }}
"""{{ sample.description }}"""

{{ frags.render_client_setup(sample.module_name, sample.client_name)|indent }}
{{ frags.render_request_setup(sample.request, sample.module_name, sample.request_type)|indent }}
{{ frags.render_request_setup(sample.request, sample.module_name, sample.request_type, calling_form, calling_form_enum)|indent }}
{% with method_call = frags.render_method_call(sample, calling_form, calling_form_enum, sample.transport) %}
{{ frags.render_calling_form(method_call, calling_form, calling_form_enum, sample.transport, sample.response)|indent -}}
{% endwith %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,17 @@ async def sample_tail_log_entries():
resource_names=['resource_names_value_1', 'resource_names_value_2'],
)

# This method expects an iterator which contains
# 'logging_v2.TailLogEntriesRequest' objects
# Here we create a generator that yields a single `request` for
# demonstrative purposes.
requests = [request]
Copy link
Contributor Author

@busunkim96 busunkim96 Nov 5, 2021

Choose a reason for hiding this comment

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

One simpler alternative is:

requests = iter([requests])

I chose the generator over iter() to nudge folks towards generating requests on demand.

Opinions/suggestions on the best way to present client streaming welcome!

def request_generator():
for request in requests:
yield request

# Make the request
stream = await client.tail_log_entries([resource_names=['resource_names_value_1', 'resource_names_value_2']])
stream = await client.tail_log_entries(requests=request_generator())
async for response in stream:
print(response)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,17 @@ def sample_tail_log_entries():
resource_names=['resource_names_value_1', 'resource_names_value_2'],
)

# This method expects an iterator which contains
# 'logging_v2.TailLogEntriesRequest' objects
# Here we create a generator that yields a single `request` for
# demonstrative purposes.
requests = [request]
def request_generator():
for request in requests:
yield request

# Make the request
stream = client.tail_log_entries([resource_names=['resource_names_value_1', 'resource_names_value_2']])
stream = client.tail_log_entries(requests=request_generator())
for response in stream:
print(response)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,17 @@ async def sample_method_bidi_streaming():
my_string="my_string_value",
)

# This method expects an iterator which contains
# 'mollusca_v1.SignatureRequestOneRequiredField' objects
# Here we create a generator that yields a single `request` for
# demonstrative purposes.
requests = [request]
def request_generator():
for request in requests:
yield request

# Make the request
stream = await client.method_bidi_streaming([my_string="my_string_value"])
stream = await client.method_bidi_streaming(requests=request_generator())
async for response in stream:
print(response)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,17 @@ def sample_method_bidi_streaming():
my_string="my_string_value",
)

# This method expects an iterator which contains
# 'mollusca_v1.SignatureRequestOneRequiredField' objects
# Here we create a generator that yields a single `request` for
# demonstrative purposes.
requests = [request]
def request_generator():
for request in requests:
yield request

# Make the request
stream = client.method_bidi_streaming([my_string="my_string_value"])
stream = client.method_bidi_streaming(requests=request_generator())
for response in stream:
print(response)

Expand Down
51 changes: 0 additions & 51 deletions tests/unit/samplegen/test_samplegen.py
Original file line number Diff line number Diff line change
Expand Up @@ -1350,57 +1350,6 @@ def test_validate_request_reserved_input_param(dummy_api_schema):
)


def test_single_request_client_streaming(dummy_api_schema,
calling_form=types.CallingForm.RequestStreamingClient):
# Each API client method really only takes one parameter:
# either a single protobuf message or an iterable of protobuf messages.
# With unary request methods, python lets us describe attributes as positional
# and keyword parameters, which simplifies request construction.
# The 'base' in the transformed request refers to an attribute, and the
# 'field's refer to sub-attributes.
# Client streaming and bidirectional streaming methods can't use this notation,
# and generate an exception if there is more than one 'base'.
input_type = DummyMessage(
fields={
"cephalopod": DummyField(
message=DummyMessage(
fields={
"order": DummyField(
message=DummyMessage(type="ORDER_TYPE")
)
},
type="CEPHALOPOD_TYPE"
)
),
"gastropod": DummyField(
message=DummyMessage(
fields={
"order": DummyField(
message=DummyMessage(type="ORDER_TYPE")
)
},
type="GASTROPOD_TYPE"
)
)
},
type="MOLLUSC_TYPE"
)
v = samplegen.Validator(DummyMethod(input=input_type), dummy_api_schema)
with pytest.raises(types.InvalidRequestSetup):
v.validate_and_transform_request(
types.CallingForm.RequestStreamingClient,
[
{"field": "cephalopod.order", "value": "cephalopoda"},
{"field": "gastropod.order", "value": "pulmonata"},
],
)


def test_single_request_bidi_streaming():
test_single_request_client_streaming(
types.CallingForm.RequestStreamingBidi)


def test_validate_request_calling_form():
assert (
types.CallingForm.method_default(DummyMethod(lro=True))
Expand Down
14 changes: 8 additions & 6 deletions tests/unit/samplegen/test_template.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ def test_render_request_unflattened():
check_template(
'''
{% import "feature_fragments.j2" as frags %}
{{ frags.render_request_setup(request, module_name, request_type) }}
{{ frags.render_request_setup(request, module_name, request_type, calling_form, calling_form_enum) }}
''',
'''
# Initialize request argument(s)
Expand Down Expand Up @@ -289,7 +289,9 @@ def test_render_request_unflattened():
)
},
ident=common_types.DummyIdent(name="CreateMolluscRequest")
)
),
calling_form_enum=CallingForm,
calling_form=CallingForm.Request,
)


Expand Down Expand Up @@ -1015,7 +1017,7 @@ def test_render_method_call_bidi():
calling_form, calling_form_enum, transport) }}
''',
'''
client.categorize_mollusc([video])
client.categorize_mollusc(requests=request_generator())
''',
request=samplegen.FullRequest(
request_list=[
Expand All @@ -1040,7 +1042,7 @@ def test_render_method_call_bidi_async():
calling_form, calling_form_enum, transport) }}
''',
'''
await client.categorize_mollusc([video])
await client.categorize_mollusc(requests=request_generator())
''',
request=samplegen.FullRequest(
request_list=[
Expand All @@ -1065,7 +1067,7 @@ def test_render_method_call_client():
calling_form, calling_form_enum, transport) }}
''',
'''
client.categorize_mollusc([video])
client.categorize_mollusc(requests=request_generator())
''',
request=samplegen.FullRequest(
request_list=[
Expand All @@ -1090,7 +1092,7 @@ def test_render_method_call_client_async():
calling_form, calling_form_enum, transport) }}
''',
'''
await client.categorize_mollusc([video])
await client.categorize_mollusc(requests=request_generator())
''',
request=samplegen.FullRequest(
request_list=[
Expand Down