gemini-code-assist[bot] commented on code in PR #38331:
URL: https://github.com/apache/beam/pull/38331#discussion_r3163489772
##########
sdks/python/apache_beam/internal/dill_pickler.py:
##########
@@ -315,7 +313,7 @@ def save_module(pickler, obj):
# Pickle module dictionaries (commonly found in lambda's globals)
# by referencing their module.
old_save_module_dict = dill.dill.save_module_dict
- known_module_dicts: Dict[int, Tuple[types.ModuleType, Dict[str, Any]]] = {}
+ known_module_dicts: dict[int, tuple[types.ModuleType, dict[str, Any]]] = {}
Review Comment:
Using built-in generics like `dict[...]` and `tuple[...]` (PEP 585) in type
annotations will cause a `TypeError` at runtime on Python 3.8 unless `from
__future__ import annotations` is present at the top of the file. Since Apache
Beam still supports Python 3.8, please ensure the future import is present or
continue using `typing.Dict` and `typing.Tuple`.
##########
sdks/python/apache_beam/internal/dill_pickler.py:
##########
@@ -50,7 +48,7 @@
settings = {'dill_byref': None}
-patch_save_code = sys.version_info >= (3, 10) and dill.__version__ == "0.3.1.1"
+patch_save_code = dill.__version__ == "0.3.1.1"
Review Comment:
The removal of the `sys.version_info >= (3, 10)` check is likely a
regression. This monkey-patch was specifically introduced to fix `dill 0.3.1.1`
on Python 3.10+, where `types.CodeType` underwent significant changes (e.g.,
the introduction of `co_linetable`). On Python 3.8 and 3.9, `dill 0.3.1.1`
works correctly without this patch. Applying this 3.10-specific patch on older
Python versions will likely cause a `TypeError` due to incompatible constructor
arguments for code objects. Since Apache Beam still supports Python 3.8 and
3.9, this version check should be retained.
```suggestion
patch_save_code = sys.version_info >= (3, 10) and dill.__version__ ==
"0.3.1.1"
```
##########
sdks/python/apache_beam/internal/metrics/metric.py:
##########
@@ -59,7 +57,7 @@ class Metrics(object):
@staticmethod
def counter(
urn: str,
- labels: Optional[Dict[str, str]] = None,
+ labels: Optional[dict[str, str]] = None,
Review Comment:
Using `dict[str, str]` as a type hint will cause a `TypeError` on Python 3.8
unless `from __future__ import annotations` is present at the top of the file.
Without the future import, built-in types are not subscriptable in Python 3.8.
##########
sdks/python/apache_beam/internal/util.py:
##########
@@ -68,9 +64,9 @@ def __hash__(self):
def remove_objects_from_args(
args: Iterable[Any],
- kwargs: Dict[str, Any],
- pvalue_class: Union[Type[T], Tuple[Type[T], ...]]
-) -> Tuple[List[Any], Dict[str, Any], List[T]]:
+ kwargs: dict[str, Any],
+ pvalue_class: Union[type[T], tuple[type[T], ...]]
+) -> tuple[list[Any], dict[str, Any], list[T]]:
Review Comment:
This change breaks compatibility with Python 3.8. Built-in generics like
`dict[...]` and `tuple[...]` (PEP 585), as well as subscripting
`collections.abc.Iterable`, are only supported in Python 3.9+. For Python 3.8
compatibility, these require `from __future__ import annotations` to be present
at the top of the file. Additionally, if these types are evaluated at runtime
(e.g., via `typing.get_type_hints()`), they will still cause a `TypeError` on
Python 3.8 even with the future import.
--
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]
