gemini-code-assist[bot] commented on code in PR #38275: URL: https://github.com/apache/beam/pull/38275#discussion_r3131417602
########## sdks/python/apache_beam/typehints/typehints.py: ########## @@ -66,6 +66,7 @@ # pytype: skip-file import copy +import functools Review Comment:  The `functools` import is only used for `lru_cache` on the `_is_beartype_disabled` helper. If that cache is removed (as suggested below), this import should also be removed. ########## CHANGES.md: ########## @@ -74,6 +74,7 @@ compatible. Both coders can decode encoded bytes from the other coder ([#38139](https://github.com/apache/beam/issues/38139)). * (Python) Added type alias for with_exception_handling to be used for typehints. ([#38173](https://github.com/apache/beam/issues/38173)). +* (Python) Made Beartype the default fallback type checking tool. This can be disabled with the `--disable_beartype` pipeline option. ([#38275](https://github.com/apache/beam/issues/38275)) ## Breaking Changes Review Comment:  This change modifies the default behavior of type checking and may cause previously passing pipelines to fail if their type hints are not strictly consistent according to Beartype. As such, it should be listed under the **Breaking Changes** section to alert users. ```suggestion * (Python) Added type alias for with_exception_handling to be used for typehints. ([#38173](https://github.com/apache/beam/issues/38173)). ## Breaking Changes * (Python) Made Beartype the default fallback type checking tool. This can be disabled with the `--disable_beartype` pipeline option. ([#38275](https://github.com/apache/beam/issues/38275)) ``` ########## sdks/python/apache_beam/typehints/typehints.py: ########## @@ -1485,7 +1486,14 @@ def normalize(x, none_as_type=False): }) -def is_consistent_with(sub, base, use_beartype: bool = False) -> bool: [email protected]_cache(maxsize=128) +def _is_beartype_disabled(options): + from apache_beam.options.pipeline_options import TypeOptions + return options.view_as(TypeOptions).disable_beartype Review Comment:  The `lru_cache` on `_is_beartype_disabled` is potentially problematic because `PipelineOptions` objects are mutable. If the `disable_beartype` flag is modified on an options instance after it has been cached, the stale value will be returned. Given that the lookup is a simple attribute access on a proxy object, the cache provides negligible benefit and should be removed to avoid potential bugs. Furthermore, this helper can be inlined into `is_consistent_with` to simplify the code. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
