tvalentyn commented on code in PR #35904:
URL: https://github.com/apache/beam/pull/35904#discussion_r2350255226
##########
sdks/python/apache_beam/internal/cloudpickle/cloudpickle.py:
##########
@@ -1326,7 +1343,12 @@ def dump(self, obj):
raise
def __init__(
- self, file, protocol=None, buffer_callback=None, config=DEFAULT_CONFIG):
+ self,
+ file,
+ protocol=None,
+ buffer_callback=None,
+ config=DEFAULT_CONFIG,
+ enable_lambda_name=False):
Review Comment:
1. If we must pass configs to cloudpickle, we should adjust settings via
the `config` param instead of adding new params here.
2. We should try to think how this change could potentially be ported into
cloudpickle. Ideally, we wouldn't have the imports like
```
from apache_beam.internal.code_object_pickler import get_code_from_identifier
from apache_beam.internal.code_object_pickler import
get_code_object_identifier
```
possible approaches:
1) We monkey-patch `cloudpickle._make_function` from
`apache_beam/internal/cloudpickle_pickler.py`, when a certain setting is set.
2) We pass callables implementing some functionality in the config, instead
of monkey-patching, for example:
`config.custom_make_function=apache_beam/internal/cloudpickle_pickler.make_function`
3) We move necessary functionality into cloudpickle.py, and then change
behaviors based on config.enable_some_cloudpickle_setting
If the customized pickling functionality could be useful for other
cloudpickle users, then i'd opt for 3. If other cloudpickle user might have a
different implementation of their own for make_function, then 2. 1 is a
quick-and-dirty option we could choose to avoid overthinking for now.
##########
sdks/python/apache_beam/internal/cloudpickle/cloudpickle.py:
##########
@@ -1336,6 +1358,7 @@ def __init__(
self.globals_ref = {}
self.proto = int(protocol)
self.config = config
+ self.enable_lambda_name = enable_lambda_name
Review Comment:
can we think of a better name for this config that would make it easier to
communicate to an unfamiliar reader what pickling behavior is being customized?
Do you have any other naming suggestions? How would you describe this setting
in a docstring?
##########
sdks/python/apache_beam/internal/cloudpickle/cloudpickle.py:
##########
@@ -1266,7 +1274,11 @@ def _dynamic_function_reduce(self, func):
"""Reduce a function that is not pickleable via attribute lookup."""
newargs = self._function_getnewargs(func)
state = _function_getstate(func)
- return (_make_function, newargs, state, None, None, _function_setstate)
+ if type(newargs[0]) == str:
Review Comment:
1. for my education what's the nature of this condition (why this particular
condition?) ` if type(newargs[0]) == str:`
2. I think this change should be configurable (when certain setting is
enabled behavior is changed, otherwise keep the original behavior).
--
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]
