tvalentyn commented on code in PR #35656:
URL: https://github.com/apache/beam/pull/35656#discussion_r2274585143
##########
sdks/python/apache_beam/internal/code_object_pickler.py:
##########
@@ -95,13 +105,16 @@ def _extend_path(prefix, suffix):
return prefix + '.' + suffix
-def _search(callable, node, qual_name_parts):
+def _search(
+ callable: types.FunctionType, node: object, qual_name_parts: list[str]):
"""Searches an object to create a stable reference code path.
Recursively searches the tree of objects starting from node and looking for
callable and returns a string to uniquely identify the path from node to the
callable.
+ Example of qual_name_parts: ['MyClass', 'process', '<locals>', '<lambda>']
Review Comment:
As a reader of this code my first initial question was: why are we passing
both `callable` and `callable.__qualname__.split('.')` as arguments to _search
, why not to pass only callable, and figure out qual_name_parts within the
function.
The docstring doesn't answer that. Looks like it play some part in the
recursion further down in the callstack, where we chip-away pieces of qual name
starting from the beginning. This can be understood only from reading the
details of the code. Do you think it would be possible to capture the nature of
using qual_name_parts here? I also wonder if it is otherwise possible to not
have this argument.
##########
sdks/python/apache_beam/internal/code_object_pickler.py:
##########
@@ -95,13 +105,16 @@ def _extend_path(prefix, suffix):
return prefix + '.' + suffix
-def _search(callable, node, qual_name_parts):
+def _search(
+ callable: types.FunctionType, node: object, qual_name_parts: list[str]):
"""Searches an object to create a stable reference code path.
Recursively searches the tree of objects starting from node and looking for
callable and returns a string to uniquely identify the path from node to the
callable.
+ Example of qual_name_parts: ['MyClass', 'process', '<locals>', '<lambda>']
Review Comment:
As a reader of this code my first initial question was: why are we passing
both `callable` and `callable.__qualname__.split('.')` as arguments to _search
, why not to pass only callable, and figure out qual_name_parts within the
function.
The docstring doesn't answer that. Looks like it plays some part in the
recursion further down in the callstack, where we chip-away pieces of qual name
starting from the beginning. This can be understood only from reading the
details of the code. Do you think it would be possible to capture the nature of
using qual_name_parts here? I also wonder if it is otherwise possible to not
have this argument.
--
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]
