[Aio] Use Metadata type #23045

rmariano · 2020-05-26T15:09:41Z

Summary

Use the new metadata type (created in #22306), in all places where the code interacted with metadata.

Fixes #21953

Compatibility

Before in the code there was a mix of types to refer to the metadata: tuples, None, and in some cases dicts, because different parts used the metadata in different ways. Now that we have a specific type, it returns an empty metadata object where before it was None. The idea being to always use an io.Metadata object, to have consistent types (as it already behaves as a dictionary, for instance).

cc/ @pfreixes @lidizheng @gnossen

src/python/grpcio/grpc/_compression.py

src/python/grpcio/grpc/experimental/aio/_call.py

src/python/grpcio/grpc/experimental/aio/_typing.py

lidizheng · 2020-05-26T17:52:52Z

src/python/grpcio_tests/tests_aio/unit/metadata_test.py

@@ -56,20 +59,12 @@
    ),
    (
        TypeError,
-        (({}, {}),),
+        ((None, {}),),


The invalid metadata test is testing if our library is resistant against badly formed metadata. Maybe we should test both cases?

Please take a look at the corner cases here. If certain bad value crash the process, we should try to prevent it from happening.

src/python/grpcio_tests/tests_aio/unit/metadata_test.py

src/python/grpcio/grpc/experimental/aio/_typing.py

pfreixes · 2020-05-26T20:15:11Z

src/python/grpcio/grpc/experimental/aio/_call.py

-        self._initial_metadata = initial_metadata
-        self._trailing_metadata = trailing_metadata
+        self._initial_metadata = Metadata(*(initial_metadata or ()))
+        self._trailing_metadata = Metadata(*(trailing_metadata or ()))


If Im not wrong most of the signatures that we have for returning metadata are Optional[MetadataType] [1] , meaning that if there is no metadata a None is returned. If this is the case every time that we build a metadata object we should do that if and only if there are values, having something like this:

self._trailing_metadata = Metadata(*initial_metadata) if initial_metadata is not None else None

Also, note that maybe for avoiding the expansion - which might have a little impact - we could change the contractor of the Metadata for taking one parameter that would be the sequence.

[1] https://github.com/grpc/grpc/pull/23045/files#diff-802047f7d320089cb1d9c7de77c9cae5R107

In the cases where it was returning None it's better to now return an empty metadata object now that we have the more specific type, as that will allow us to treat all calls to these methods the same (without exceptions of the kind if metadata is not None: ... ), so I'd replace the Optional[MetadataType] by Metadata. WDYT?

For me I'm happy! WDYT @lidizheng | @gnossen ?

Agree. Deterministic Metadata is better.

src/python/grpcio/grpc/experimental/aio/_call.py

src/python/grpcio/grpc/experimental/aio/_channel.py

src/python/grpcio_tests/tests_aio/unit/call_test.py

src/python/grpcio/grpc/experimental/aio/_call.py

In all places where a tuple was used for metadata (in the aio version), replace it by the new ``Metadata`` class.

Fixes grpc#21953

* Replace ``MetadataType`` by ``Metadata`` in all places * Fix annotations * Use the new ``Metadata.from_tuple`` to create Metadata objects

Using the new aio.Metadata() type instead of tuple.

Replace the signature to allow methods to use the metadata object. Internally, they'll still wrap the data in a tuple, but the interface makes it clear that the ``aio.Metadata()`` object is supported. Remove the ``tuple(<metadata>)`` conversions done in the tests.

The old interface of accepting the metadata as a list, should be kept due to a backwards incompatibility with a client. The new ``aio.Metadata()`` type supports iteration, so creating a list from it, is possible.

rmariano · 2020-06-02T10:40:49Z

@pfreixes @lidizheng @gnossen feedback has been addressed or replied. PTAL

gnossen · 2020-06-02T17:33:37Z

src/python/grpcio/grpc/experimental/aio/_metadata.py

@@ -37,6 +37,12 @@ def __init__(self, *args: Tuple[MetadataKey, MetadataValue]) -> None:
        for md_key, md_value in args:
            self.add(md_key, md_value)

+    @classmethod
+    def from_tuple(cls, raw_metadata: tuple):
+        if raw_metadata:


What's the reasoning behind the conditional here? Shouldn't return cls(*raw_metadata) just work?

Yes, but raw_metadata can be None.
I'll change it to be explicit, like

if raw_metadata is None: ...

pfreixes

@rmariano I left a couple of comments!

pfreixes · 2020-06-02T19:41:49Z

src/python/grpcio/grpc/experimental/aio/_call.py

-                 initial_metadata: Optional[MetadataType] = None,
-                 trailing_metadata: Optional[MetadataType] = None,
+                 initial_metadata: Optional[Metadata] = None,
+                 trailing_metadata: Optional[Metadata] = None,


Can be them set to None? If so the return type for the getter methods would need to be changed, for example [1], if not we should remove the Optional (the same for the class type definition)

https://github.com/grpc/grpc/pull/23045/files#diff-802047f7d320089cb1d9c7de77c9cae5R107

Good point, I think we can remove the Optional[] because it's always a Metadata object, however, I wouldn't remove the default argument = None because that would imply changing the position of the parameters (they would have to go right after code), and that could cause backward incompatibilities, and also confuse users.
The proposal is then to remove the Optional[..] annotation but keep the default argument to None. WDYT?

weird :), If we keep the = None I will keep the Optional, regarding backward compatibility the constuctor of AioRpcError is internal, so it menas that If Im not wrong we can turn it to a positional argument.

WDYT @lidizheng ?

To me, Optional looks better in terms of conveying the variable is "optional".

Yes, this is an internal API, users won't be impacted. I don't have strong opinion here, either way looks fine to me. Since @rmariano is the author of the PR, maybe he should make the call?

up to you @rmariano !

I'll refactor this, to make these parameters no longer optional.

pfreixes · 2020-06-02T19:54:39Z

src/python/grpcio/grpc/experimental/aio/_interceptor.py

@@ -248,7 +248,7 @@ async def intercept_stream_stream(


 class InterceptedCall:
-    """Base implementation for all intecepted call arities.
+    """Base implementation for all intercepted call arities.


Thanks for fixing the typo. In any case seems that within the _interceptror.py file there are some reference to the old Metadata type like this [1] which would need to be changed, the same for the ClientCallDetaills which is still using an Optional[MetadataType] should this be changed to Metadata? [2]

[1] https://github.com/grpc/grpc/pull/23045/files#diff-173e8bd5d2219ba109f86237680068a4R383
[2] https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/experimental/aio/_interceptor.py#L85

One last thing, if the input from the user hasn't changed, so its still providing the metadata for te call using a tuple I wold not change either the ClientCallDetails since this is an interface used by the interceptors for mutating the metadata provided by the user.

lidizheng

Thank you for the changes! This should improve the usability of metadata quite a bit. One minor comment, LGTM if it is taken care of.

lidizheng · 2020-06-02T23:08:35Z

src/python/grpcio_tests/tests_aio/unit/metadata_test.py

@@ -56,20 +59,12 @@
    ),
    (
        TypeError,
-        (({}, {}),),
+        ((None, {}),),


Please take a look at the corner cases here. If certain bad value crash the process, we should try to prevent it from happening.

lidizheng · 2020-06-09T20:39:33Z

@pfreixes Is there anything else blocking this PR?
@rmariano Please take a look at the sanity check, also the invalid metadata test.

pfreixes

Considering that the open conversations are not a blockers and everything if it is needed can be changed later without breaking compatibility, approving the PR!

rmariano · 2020-06-10T18:11:50Z

@lidizheng I'll fix the sanity check, but I was also taking a look at the tests for invalid metadata. Basically they're as they were before, but now I'm wondering if they make sense, because when passing something that's not a Metadata object, they should break, which is what the tests are asserting. Is there anything I'm missing?
Maybe those tests can be changed to directly test the new _init_metadata method, rather than composing the entire call object, and awaiting it.

lidizheng · 2020-06-10T18:20:14Z

when passing something that's not a Metadata object, they should break

Yes. They should break, and they should raise one exception. The test was there because in the past a malformed metadata could cause segfault or abort which crashes entire process. So, to be sure, this test tried various malformed cases to ensure no crashes and exception are raised as expected.

I see you removed (({}, {}),), and added ((None, {}),),, can we add the former one back? It's a corner case, sorry for being nitpicking here.

rmariano · 2020-06-10T18:25:26Z

@lidizheng oh thanks, now I get it. Sorry I thought it was restored, but yes, I'll add the case for (({}, {}),),

Code formatted.

rmariano · 2020-06-11T07:34:36Z

hi @pfreixes @lidizheng I've addressed your comments. Let me know if it looks OK for you, and in case not, I'll iterate further.

lidizheng · 2020-06-12T17:35:08Z

Known failures: #23190

lidizheng added lang/Python release notes: yes Indicates if PR needs to be in release notes labels May 26, 2020

lidizheng changed the title ~~Issue 21953 use metadata type~~ [Aio] Use Metadata type May 26, 2020

lidizheng added this to In progress in gRPC Async API via automation May 26, 2020

lidizheng reviewed May 26, 2020

View reviewed changes

lidizheng requested review from pfreixes and gnossen May 26, 2020 17:56

lidizheng assigned pfreixes, gnossen and lidizheng May 26, 2020

lidizheng added the kokoro:run label May 26, 2020

grpc-kokoro removed the kokoro:run label May 26, 2020

pfreixes requested changes May 26, 2020

View reviewed changes

gnossen reviewed May 26, 2020

View reviewed changes

src/python/grpcio_tests/tests_aio/unit/call_test.py Outdated Show resolved Hide resolved

src/python/grpcio/grpc/experimental/aio/_call.py Outdated Show resolved Hide resolved

lidizheng added the kokoro:run label May 28, 2020

grpc-kokoro removed the kokoro:run label May 28, 2020

Mariano Anaya added 7 commits June 2, 2020 09:30

[issue-21953] Use the Metadata type

e04fcd2

In all places where a tuple was used for metadata (in the aio version), replace it by the new ``Metadata`` class.

[issue-24953] Fix tests, format, & types

e9dadf4

Fixes grpc#21953

[issue-21953] Improvements from review

8fcc77a

* Replace ``MetadataType`` by ``Metadata`` in all places * Fix annotations * Use the new ``Metadata.from_tuple`` to create Metadata objects

Fix new metadata tests

5a5a578

Using the new aio.Metadata() type instead of tuple.

Restore test that passes metadata in a list

7b3430e

The old interface of accepting the metadata as a list, should be kept due to a backwards incompatibility with a client. The new ``aio.Metadata()`` type supports iteration, so creating a list from it, is possible.

Apply formatting & fix typing

6e83eb7

rmariano force-pushed the issue-21953_use-metadata-type branch from 8f52446 to 6e83eb7 Compare June 2, 2020 10:39

gnossen approved these changes Jun 2, 2020

View reviewed changes

pfreixes reviewed Jun 2, 2020

View reviewed changes

lidizheng added the kokoro:run label Jun 2, 2020

grpc-kokoro removed the kokoro:run label Jun 2, 2020

lidizheng approved these changes Jun 2, 2020

View reviewed changes

Mariano Anaya added 2 commits June 8, 2020 16:52

Remove references to the old MetadataType

36f79ad

Remove metadata as optional from AioRpcError

18e0f9f

lidizheng added the kokoro:run label Jun 8, 2020

grpc-kokoro removed the kokoro:run label Jun 8, 2020

pfreixes approved these changes Jun 9, 2020

View reviewed changes

Add missing metadata TypeError case

376c0f0

Code formatted.

lidizheng added the kokoro:run label Jun 11, 2020

grpc-kokoro removed the kokoro:run label Jun 11, 2020

lidizheng merged commit c47bc1a into grpc:master Jun 12, 2020

gRPC Async API automation moved this from In progress to Done Jun 12, 2020

rmariano deleted the issue-21953_use-metadata-type branch June 15, 2020 08:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Aio] Use Metadata type #23045

[Aio] Use Metadata type #23045

rmariano commented May 26, 2020

lidizheng May 26, 2020

lidizheng Jun 2, 2020

pfreixes May 26, 2020

rmariano May 27, 2020

pfreixes May 27, 2020

lidizheng May 27, 2020

rmariano commented Jun 2, 2020

gnossen Jun 2, 2020

rmariano Jun 3, 2020

pfreixes left a comment

pfreixes Jun 2, 2020

rmariano Jun 3, 2020

pfreixes Jun 3, 2020

lidizheng Jun 3, 2020

pfreixes Jun 3, 2020

rmariano Jun 8, 2020

pfreixes Jun 2, 2020

pfreixes Jun 3, 2020

lidizheng left a comment

lidizheng Jun 2, 2020

lidizheng commented Jun 9, 2020

pfreixes left a comment

rmariano commented Jun 10, 2020

lidizheng commented Jun 10, 2020

rmariano commented Jun 10, 2020

rmariano commented Jun 11, 2020

lidizheng commented Jun 12, 2020

[Aio] Use Metadata type #23045

[Aio] Use Metadata type #23045

Conversation

rmariano commented May 26, 2020

Summary

Compatibility

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmariano commented Jun 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfreixes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidizheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidizheng commented Jun 9, 2020

pfreixes left a comment

Choose a reason for hiding this comment

rmariano commented Jun 10, 2020

lidizheng commented Jun 10, 2020

rmariano commented Jun 10, 2020

rmariano commented Jun 11, 2020

lidizheng commented Jun 12, 2020