[Python] [Typeguard] Part 5 - Add Typeguard SYNC Stack in tests#40278
[Python] [Typeguard] Part 5 - Add Typeguard SYNC Stack in tests#40278asheshvidyut wants to merge 253 commits intogrpc:masterfrom
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Typeguard for the SYNC stack and includes several type hint improvements across the codebase. The changes are generally positive, enhancing type safety and fixing potential issues like an UnboundLocalError in a test.
I've identified a couple of areas for improvement:
- A redundant check in
src/python/grpcio/grpc/aio/_call.pycan be removed to simplify the code. - A related test case in
src/python/grpcio_tests/tests_aio/unit/_metadata_test.pythat was removed should be restored to maintain test coverage forNonehandling.
Overall, this is a good step towards better type safety in the project.
| if not raw_metadata_tuple: | ||
| return Metadata() | ||
| return Metadata.from_tuple(raw_metadata_tuple) |
There was a problem hiding this comment.
This check for raw_metadata_tuple is redundant. The Metadata.from_tuple class method is already designed to handle falsy values (like None or an empty tuple) by returning a new empty Metadata instance. Removing this explicit check simplifies the code without changing its behavior.
return Metadata.from_tuple(raw_metadata_tuple)|
|
||
| def test_metadata_from_tuple(self): | ||
| scenarios = ( | ||
| (None, Metadata()), |
There was a problem hiding this comment.
This test case for None input to Metadata.from_tuple is valid and should be kept. The from_tuple method is designed to handle None gracefully. Restoring this test case ensures that this behavior remains tested, especially if the redundant check in aio/_call.py is removed.
…0226) ### 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: #40201 - Part 2: #40215 - Part 3: #40217 - Part 5: #40278 - b/423755915 Closes #40226 COPYBARA_INTEGRATE_REVIEW=#40226 from asheshvidyut:feature/type-hints/typeguard-aio-stack-part-4 70db2d6 PiperOrigin-RevId: 871724432
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully integrates Typeguard into the gRPC Python sync stack for testing purposes. The changes improve type safety and resolve several type-related issues, such as circular imports and incorrect type hints. I have identified a few issues in src/python/grpcio/grpc/_compression.py where an internal module is accessed incorrectly and a type hint might cause runtime resolution errors. Overall, the changes are a solid step towards better type coverage in the codebase.
I am having trouble creating individual review comments. Click here to see my feedback.
src/python/grpcio/grpc/_compression.py (42)
The access to grpc._common will fail at runtime because _common is not imported in this file and is not automatically an attribute of the grpc package. Furthermore, cygrpc.GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY is already decoded to a string in the Cython layer (see records.pyx.pxi), so the decode() call is redundant.
cygrpc.GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY,
src/python/grpcio/grpc/_compression.py (56)
Using grpc.aio.Metadata directly in the type hint may cause Typeguard to fail at runtime because grpc.aio is not imported in this module. Since this is a sync-stack file, it is better to use a string forward reference to avoid forcing the import of the async stack or causing resolution errors during runtime type checking.
metadata: Optional[Union[MetadataType, "grpc.aio.Metadata"]],
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces typeguard for the synchronous gRPC stack within the test environment, which is a valuable step towards improving type safety. The changes also include several related fixes to type hints across different modules, such as _channel.py, _compression.py, and _plugin_wrapping.py, making the type annotations more accurate. Additionally, there are minor but good improvements like making OperationType an IntEnum and fixing a potential UnboundLocalError in a test. Overall, the changes are correct and enhance the codebase's quality and maintainability. I have no further suggestions.
| import logging | ||
| import threading | ||
| from typing import Callable, Optional, Type | ||
| from typing import AnyStr, Callable, Optional, Type |
There was a problem hiding this comment.
Apparently AnyStr is deprecated in 3.13 and will be removed in 3.18:
https://docs.python.org/3.14/library/typing.html#typing.AnyStr
| def __call__(self, service_url: str, method_name: str, callback: Callable): | ||
| def __call__( | ||
| self, service_url: AnyStr, method_name: AnyStr, callback: Callable | ||
| ): |
There was a problem hiding this comment.
I'm not convinced about this change. If this is only caught in a single place, I suspect something could be wrong with the test itself. Perhaps it's mocking these types incorrectly?
I see this after adding print statements to the _plugin_wrapping._Plugin:
service_url=b'https://foo.test.google.fr/grpc.testing.TestService'
method_name=b'FullDuplexCall'
We need to see if the test server is set correctly. https://github.com/grpc/grpc/blob/master/src/python/grpcio_tests/tests_aio/unit/_test_server.py
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