robertwb commented on code in PR #30117:
URL: https://github.com/apache/beam/pull/30117#discussion_r1467117005
##########
sdks/python/apache_beam/yaml/yaml_combine.md:
##########
@@ -147,6 +147,27 @@ parameter.
n: 10
```
+For custom aggregation functions that use python callables as parameters,
+nesting the function parameter values under a `callable` tag will tell the
+transform to treat the value as a callable.
Review Comment:
evaluate the value as a callable rather than a literal string.
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -139,6 +139,21 @@ def as_provider_list(name, lst):
return [as_provider(name, x) for x in lst]
+def parse_callable_kwargs(spec: Optional[Mapping[str, Any]]):
+ def _parse_callable_kwargs_rec(args, top_level=False):
+ parsed_args = {}
+ for arg, val in args.items():
+ parsed_args[arg] = val
+ if isinstance(val, dict):
Review Comment:
What about lists?
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -407,9 +427,11 @@ def fn_takes_side_inputs(fn):
class InlineProvider(Provider):
- def __init__(self, transform_factories, no_input_transforms=()):
+ def __init__(
+ self, transform_factories, no_input_transforms=(),
custom_provider=False):
Review Comment:
What does "custom provider" mean?
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -139,6 +139,21 @@ def as_provider_list(name, lst):
return [as_provider(name, x) for x in lst]
+def parse_callable_kwargs(spec: Optional[Mapping[str, Any]]):
+ def _parse_callable_kwargs_rec(args, top_level=False):
+ parsed_args = {}
Review Comment:
Nit: Prefere dict comprehensions. This can also make it clearer exactly the
pattern you're looking for/handling.
E.g.
```
def parse_marked_callables(o):
if isinstance(o, dict):
if len(o) == 1 and next(iter(o.keys()) == 'callable'
return
python_callable.PythonCallableWithSource.load_from_source(next(iter(o.values()))
else:
return {k: parse_marked_callables(v) for (k, v) in o.items()}
elif isinstance(o, list):
return [parse_marked_callables(e) for e in o]
else:
return o
```
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -139,6 +139,21 @@ def as_provider_list(name, lst):
return [as_provider(name, x) for x in lst]
+def parse_callable_kwargs(spec: Optional[Mapping[str, Any]]):
+ def _parse_callable_kwargs_rec(args, top_level=False):
+ parsed_args = {}
+ for arg, val in args.items():
+ parsed_args[arg] = val
Review Comment:
Nit: Prefer to write once, rather than assign and then re-assign (e.g. put
this in an else).
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -139,6 +139,21 @@ def as_provider_list(name, lst):
return [as_provider(name, x) for x in lst]
+def parse_callable_kwargs(spec: Optional[Mapping[str, Any]]):
+ def _parse_callable_kwargs_rec(args, top_level=False):
Review Comment:
Why do we distinguish between top-level vs. not?
--
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]
