[Python] [Typeguard] Part 4 - Add Typeguard to AIO stack in tests #40226
[Python] [Typeguard] Part 4 - Add Typeguard to AIO stack in tests #40226asheshvidyut wants to merge 62 commits intogrpc:masterfrom
Conversation
26572b0 to
c584ba8
Compare
|
@sergiitk as discussed - I am only supressing the test cases which have |
|
|
||
| def get( | ||
| self, key: MetadataKey, default: MetadataValue = None | ||
| self, key: MetadataKey, default: Optional[MetadataValue] = None |
There was a problem hiding this comment.
Had to change this because some tests were failing with following stack trace
======================================================================
ERROR: test_delete_values (src.python.grpcio_tests.tests_aio.unit._metadata_test.TestTypeMetadata)
----------------------------------------------------------------------
Traceback (most recent call last):
File ".src/python/grpcio_tests/tests_aio/unit/_metadata_test.py", line 186, in test_delete_values
self.assertEqual(metadata.get("key1"), "other value 1")
File ".src/python/grpcio/grpc/aio/_metadata.py", line 104, in get
def get(
File ".site-packages/typeguard/_functions.py", line 136, in check_argument_types
check_type_internal(value, annotation, memo)
File ".site-packages/typeguard/_checkers.py", line 779, in check_type_internal
checker(value, origin_type, args, memo)
File ".site-packages/typeguard/_checkers.py", line 428, in check_union
raise TypeCheckError(f"did not match any element in the union:\n{formatted_errors}")
typeguard.TypeCheckError: argument "default" (None) did not match any element in the union:
str: is not an instance of str
bytes: is not bytes-like
======================================================================
ERROR: test_get_item (src.python.grpcio_tests.tests_aio.unit._metadata_test.TestTypeMetadata)
----------------------------------------------------------------------
Traceback (most recent call last):
File ".src/python/grpcio_tests/tests_aio/unit/_metadata_test.py", line 118, in test_get_item
self.assertEqual(metadata.get("key"), "value1")
File ".src/python/grpcio/grpc/aio/_metadata.py", line 104, in get
def get(
File ".site-packages/typeguard/_functions.py", line 136, in check_argument_types
check_type_internal(value, annotation, memo)
File ".site-packages/typeguard/_checkers.py", line 779, in check_type_internal
checker(value, origin_type, args, memo)
File ".site-packages/typeguard/_checkers.py", line 428, in check_union
raise TypeCheckError(f"did not match any element in the union:\n{formatted_errors}")
typeguard.TypeCheckError: argument "default" (None) did not match any element in the union:
str: is not an instance of str
bytes: is not bytes-like
5df8618 to
d7cf624
Compare
d7cf624 to
f67aaa0
Compare
f67aaa0 to
7fa0f84
Compare
a01f89b to
bbe3bf9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enables Typeguard for more AIO modules and fixes several type hints. The changes look good, especially the use of @typeguard.suppress_type_checks in tests that intentionally use invalid types. I've found one area for improvement regarding a type hint in _metadata.py that could be made more accurate to reflect the implementation and the author's intent as described in the PR description. I've left a couple of comments with suggestions.
|
|
||
| def test_metadata_from_tuple(self): | ||
| scenarios = ( | ||
| (None, Metadata()), |
There was a problem hiding this comment.
| @typeguard.suppress_type_checks | ||
| async def test_invalid_client_args(self): | ||
| # This test works on invalid client_args which is expected | ||
| # to make typeguard fail, hence the decorator | ||
| # @typeguard.supress_type_checks is used |
There was a problem hiding this comment.
here and elsewhere: the comment can be as simple as this, no need for long explanation in the test itself
| @typeguard.suppress_type_checks | |
| async def test_invalid_client_args(self): | |
| # This test works on invalid client_args which is expected | |
| # to make typeguard fail, hence the decorator | |
| # @typeguard.supress_type_checks is used | |
| @typeguard.suppress_type_checks # testing negative cases |
|
|
||
| def get( | ||
| self, key: MetadataKey, default: MetadataValue = None | ||
| self, key: MetadataKey, default: Optional[MetadataValue] = None |
| def compression_algorithm_to_metadata(compression: grpc.Compression): | ||
| return ( | ||
| cygrpc.GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY, | ||
| grpc._common.decode(cygrpc.GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY), |
There was a problem hiding this comment.
For posterity, this fixes
======================================================================
ERROR: test_client_call_level_compression_allowed_compression (src.python.grpcio_tests.tests_aio.unit.compression_test.TestCompression.test_client_call_level_compression_allowed_compression)
----------------------------------------------------------------------
Traceback (most recent call last):
File "./src/python/grpcio_tests/tests_aio/unit/_test_base.py", line 31, in wrapper
return loop.run_until_complete(f(*args, **kwargs))
~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
File "/opt/local/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/base_events.py", line 725, in run_until_complete
return future.result()
~~~~~~~~~~~~~^^
File "./src/python/grpcio_tests/tests_aio/unit/compression_test.py", line 157, in test_client_call_level_compression_allowed_compression
call = multicallable(_REQUEST, compression=grpc.Compression.Deflate)
File "./src/python/grpcio/grpc/aio/_channel.py", line 150, in __call__
metadata = self._init_metadata(metadata, compression)
File "./src/python/grpcio/grpc/aio/_channel.py", line 131, in _init_metadata
metadata = Metadata(
*_compression.augment_metadata(metadata, compression)
)
File "./src/python/grpcio/grpc/aio/_metadata.py", line 38, in __init__
def __init__(self, *args: Tuple[MetadataKey, MetadataValue]) -> None:
...<2 lines>...
self.add(md_key, md_value)
File "./site-packages/typeguard/_functions.py", line 137, in check_argument_types
check_type_internal(value, annotation, memo)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
File "./site-packages/typeguard/_checkers.py", line 951, in check_type_internal
checker(value, origin_type, args, memo)
~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "./site-packages/typeguard/_checkers.py", line 384, in check_tuple
check_type_internal(element, element_type, memo)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "./site-packages/typeguard/_checkers.py", line 951, in check_type_internal
checker(value, origin_type, args, memo)
~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "./site-packages/typeguard/_checkers.py", line 400, in check_tuple
check_type_internal(element, element_type, memo)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "./site-packages/typeguard/_checkers.py", line 956, in check_type_internal
raise TypeCheckError(f"is not an instance of {qualified_name(origin_type)}")
typeguard.TypeCheckError: item 0 of item 0 of argument "args" (tuple) is not an instance of str
======================================================================
ERROR: test_client_call_level_compression_baned_compression (src.python.grpcio_tests.tests_aio.unit.compression_test.TestCompression.test_client_call_level_compression_baned_compression)
----------------------------------------------------------------------
Traceback (most recent call last):
File "./src/python/grpcio_tests/tests_aio/unit/_test_base.py", line 31, in wrapper
return loop.run_until_complete(f(*args, **kwargs))
~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
File "/opt/local/Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/base_events.py", line 725, in run_until_complete
return future.result()
~~~~~~~~~~~~~^^
File "./src/python/grpcio_tests/tests_aio/unit/compression_test.py", line 147, in test_client_call_level_compression_baned_compression
call = multicallable(_REQUEST, compression=grpc.Compression.Gzip)
File "./src/python/grpcio/grpc/aio/_channel.py", line 150, in __call__
metadata = self._init_metadata(metadata, compression)
File "./src/python/grpcio/grpc/aio/_channel.py", line 131, in _init_metadata
metadata = Metadata(
*_compression.augment_metadata(metadata, compression)
)
File "./src/python/grpcio/grpc/aio/_metadata.py", line 38, in __init__
def __init__(self, *args: Tuple[MetadataKey, MetadataValue]) -> None:
...<2 lines>...
self.add(md_key, md_value)
File "./site-packages/typeguard/_functions.py", line 137, in check_argument_types
check_type_internal(value, annotation, memo)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
File "./site-packages/typeguard/_checkers.py", line 951, in check_type_internal
checker(value, origin_type, args, memo)
~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "./site-packages/typeguard/_checkers.py", line 384, in check_tuple
check_type_internal(element, element_type, memo)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "./site-packages/typeguard/_checkers.py", line 951, in check_type_internal
checker(value, origin_type, args, memo)
~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "./site-packages/typeguard/_checkers.py", line 400, in check_tuple
check_type_internal(element, element_type, memo)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "./site-packages/typeguard/_checkers.py", line 956, in check_type_internal
raise TypeCheckError(f"is not an instance of {qualified_name(origin_type)}")
typeguard.TypeCheckError: item 0 of item 0 of argument "args" (tuple) is not an instance of str
----------------------------------------------------------------------| def compression_algorithm_to_metadata(compression: grpc.Compression): | ||
| return ( | ||
| cygrpc.GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY, | ||
| grpc._common.decode(cygrpc.GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY), |
There was a problem hiding this comment.
Looking further into this, I recommend defining GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY as a string in
grpc/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi
Lines 114 to 115 in 456a47b
Since we know that a metadata key should never be bytes, we should just do
GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY = (
_GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY
).decode()There was a problem hiding this comment.
agree, and IIRC I had looked into this and had a similar comment when I first reviewed this PR too. Sadly, looks like the comment disappeared after the force push. Anyways, we also have a similar situation with cygrpc.ChannelArgKey.primary_user_agent_string as I mentioned here: https://github.com/grpc/grpc/pull/40278/changes#r2247823758
Values coming from Core tend to be char* (equivalent to bytes) which is the cause of this problem. We should convert them to string when we use them in Python
|
|
||
| @classmethod | ||
| def from_tuple(cls, raw_metadata: tuple): | ||
| def from_tuple(cls, raw_metadata: Union[tuple, Self]): |
There was a problem hiding this comment.
If this function is also accepting a Metadata object, we should probably add a check for the type and return the object itself directly.
The current logic works, but it will be creating a new Metadata object from the given Metadata object, which seems redundant.
There was a problem hiding this comment.
This is actually a very good point. Not sure how we missed this. The name of the method is clearly from_tuple. Is there a test how it'll even work with non-empty Metadata object?
Also, Self is misused here:
Special type to represent the current enclosed class.
— https://docs.python.org/3.14/library/typing.html#typing.Self
The main intent for this is to:
- Annotate
selfargument asself: Self - Indicate the method returns self (f.e. in chaining), and require subclasses from returning superclasses.
- Indicate abstract factory methods that return
cls()
I don't think this was ever intended to be used in arguments.
@asheshvidyut - we should fix this ASAP.
There was a problem hiding this comment.
My bad, looked at the spec, and it's OK to use it in params:
https://typing.python.org/en/latest/spec/generics.html#use-in-parameter-types
There was a problem hiding this comment.
Looks like I missed the last check you added.
There was a problem hiding this comment.
Discussed the plan to address this IRL.
- We'll revert
raw_metadatatype totuple - We'll going create an internal
_createclassmethod that will support instantiating Metadata from- None
- Metadata (Self)
- Iterable[tuple[MetadataKey, MetadataValue]]
- Internally, we'll replace all places that violate the original signature
def from_tuple(cls, raw_metadata: tuple)to the new_createmethod - We'll make the method public with the same gRFC where we introduces type hints to the sync stack.
| def compression_algorithm_to_metadata(compression: grpc.Compression): | ||
| return ( | ||
| cygrpc.GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY, | ||
| grpc._common.decode(cygrpc.GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY), |
There was a problem hiding this comment.
agree, and IIRC I had looked into this and had a similar comment when I first reviewed this PR too. Sadly, looks like the comment disappeared after the force push. Anyways, we also have a similar situation with cygrpc.ChannelArgKey.primary_user_agent_string as I mentioned here: https://github.com/grpc/grpc/pull/40278/changes#r2247823758
Values coming from Core tend to be char* (equivalent to bytes) which is the cause of this problem. We should convert them to string when we use them in Python
This PR adds new create method for `aio.Metadata`, as described in the comment here - #40226 (comment) Reason being `from_tuple` as the name says should actually create only from `tuple` whereas the method actually takes other arguments as well. Hence we introduce the new `_create` method, which is internal as of now. Closes #41678 COPYBARA_INTEGRATE_REVIEW=#41678 from asheshvidyut:fix-api c1a0281 PiperOrigin-RevId: 885107284
This PR adds new create method for `aio.Metadata`, as described in the comment here - grpc#40226 (comment) Reason being `from_tuple` as the name says should actually create only from `tuple` whereas the method actually takes other arguments as well. Hence we introduce the new `_create` method, which is internal as of now. Closes grpc#41678 COPYBARA_INTEGRATE_REVIEW=grpc#41678 from asheshvidyut:fix-api c1a0281 PiperOrigin-RevId: 885107284
…pc#40226) ### Description Last Part of Add Typeguard to AIO stack - runs only in tests, fixes type hints for AIO Stack Change - #### Type Annotation Update in Channel Constructor File: _channel.py (line 330) Before: credentials: `Optional[grpc.ChannelCredentials]`, After: credentials: `Optional[cygrpc.ChannelCredentials]`, Change: Updated type hint from `grpc.ChannelCredentials` to `cygrpc.ChannelCredentials` #### Metadata Class Method Enhancement File: _metadata.py (around line 100-102) Before: `def from_tuple(cls, raw_metadata: tuple):` After: `def from_tuple(cls, raw_metadata: Union[tuple, Self]):` #### `GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY`decode at Cython layer. File: src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi Before: `GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY = ( _GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY)` After: `GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY = ( _GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY).decode()` ### Testing CI ### Related Work - Part 1: grpc#40201 - Part 2: grpc#40215 - Part 3: grpc#40217 - Part 5: grpc#40278 - b/423755915 Closes grpc#40226 COPYBARA_INTEGRATE_REVIEW=grpc#40226 from asheshvidyut:feature/type-hints/typeguard-aio-stack-part-4 70db2d6 PiperOrigin-RevId: 871724432
This PR adds new create method for `aio.Metadata`, as described in the comment here - grpc#40226 (comment) Reason being `from_tuple` as the name says should actually create only from `tuple` whereas the method actually takes other arguments as well. Hence we introduce the new `_create` method, which is internal as of now. Closes grpc#41678 COPYBARA_INTEGRATE_REVIEW=grpc#41678 from asheshvidyut:fix-api c1a0281 PiperOrigin-RevId: 885107284
…1888) Backport of #41678 to v1.80.x. --- This PR adds new create method for `aio.Metadata`, as described in the comment here - #40226 (comment) Reason being `from_tuple` as the name says should actually create only from `tuple` whereas the method actually takes other arguments as well. Hence we introduce the new `_create` method, which is internal as of now. Co-authored-by: Ashesh Vidyut <asheshvidyut@google.com>
This PR adds new create method for `aio.Metadata`, as described in the comment here - grpc#40226 (comment) Reason being `from_tuple` as the name says should actually create only from `tuple` whereas the method actually takes other arguments as well. Hence we introduce the new `_create` method, which is internal as of now. Closes grpc#41678 COPYBARA_INTEGRATE_REVIEW=grpc#41678 from asheshvidyut:fix-api c1a0281 PiperOrigin-RevId: 885107284
### Description First Part of Add Typeguard to SYNC stack - runs only in tests, fixes type hints for SYNC Stack. This has changes from Part 4 #40226 ### Changes TODO ### Testing CI ### Related Work - Part 1: #40201 - Part 2: #40215 - Part 3: #40217 - Part 4: #40226 - Part 6: #40353 - b/423755915 Closes #40278 COPYBARA_INTEGRATE_REVIEW=#40278 from asheshvidyut:feature/type-hints/typeguard-sync-stack-part-1 24987e7 PiperOrigin-RevId: 890732097
…#40278) ### Description First Part of Add Typeguard to SYNC stack - runs only in tests, fixes type hints for SYNC Stack. This has changes from Part 4 grpc#40226 ### Changes TODO ### Testing CI ### Related Work - Part 1: grpc#40201 - Part 2: grpc#40215 - Part 3: grpc#40217 - Part 4: grpc#40226 - Part 6: grpc#40353 - b/423755915 Closes grpc#40278 COPYBARA_INTEGRATE_REVIEW=grpc#40278 from asheshvidyut:feature/type-hints/typeguard-sync-stack-part-1 24987e7 PiperOrigin-RevId: 890732097
Description
Last Part of Add Typeguard to AIO stack - runs only in tests, fixes type hints for AIO Stack
Change -
Type Annotation Update in Channel Constructor
File: _channel.py (line 330)
Before: credentials:
Optional[grpc.ChannelCredentials],After: credentials:
Optional[cygrpc.ChannelCredentials],Change:
Updated type hint from
grpc.ChannelCredentialstocygrpc.ChannelCredentialsMetadata Class Method Enhancement
File: _metadata.py (around line 100-102)
Before:
def from_tuple(cls, raw_metadata: tuple):After:
def from_tuple(cls, raw_metadata: Union[tuple, Self]):GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEYdecode at Cython layer.File: src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi
Before:
GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY = ( _GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY)After:
GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY = ( _GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY).decode()Testing
CI
Related Work