tvalentyn commented on a change in pull request #16715:
URL: https://github.com/apache/beam/pull/16715#discussion_r799930547
##########
File path: sdks/python/apache_beam/options/pipeline_options.py
##########
@@ -55,6 +55,18 @@
_LOGGER = logging.getLogger(__name__)
+# These options have no dest and store_false in the argparse.
+# When parsing these options in a dict using PipelineOptions,
+# We either ignore/discard if these options are specified.
+# Defining a map with their dest would maintain consistency
+# across PipelineOptions(**dict), PipelineOptions.from_dictionary(dict),
+# and argparse.
+_PARSE_OPTIONS_WITH_PREFIX_NO = {
Review comment:
I think what we care about here are options with different dest, that
have action: store_false. Is that correct?
If so, how about:
1) we call this: _NEGATIVE_OPTIONS_WITH_DIFFERENT_DEST
2) we add a test that looks up options defined in all classes that inherit
from PipelineOptions, and verifies that:
- all options that have a dest that's different from opition_name and a
store_false action are in this map.
- there are no options that have a dest that's different from option_name
and store_true value (that would be even more confusing).
##########
File path: sdks/python/apache_beam/options/pipeline_options.py
##########
@@ -236,10 +253,13 @@ def from_dictionary(cls, options):
A PipelineOptions object representing the given arguments.
"""
flags = []
+ params = {}
for k, v in options.items():
if isinstance(v, bool):
if v:
flags.append('--%s' % k)
+ else:
+ params[k] = v
Review comment:
Likewise, here I think we should only allow 'True' values, and print a
warning to the user when this is not the case.
##########
File path: sdks/python/apache_beam/options/pipeline_options.py
##########
@@ -219,6 +231,11 @@ def __init__(self, flags=None, **kwargs):
self._all_options[option_name] = getattr(
self._visible_options, option_name)
+ # Override flags dependent on each other
+ for option_name, option_value in _PARSE_OPTIONS_WITH_PREFIX_NO.items():
+ if option_name in self._all_options:
+ self._all_options[option_value] = (not self._all_options[option_name])
Review comment:
I think we should only expect inputs where
`self._all_options[option_name] == True`, and print a warning that the False
value is ignored otherwise. Then, we can assign `False` to the dest. This would
be consistent with command-line parsing
##########
File path: sdks/python/apache_beam/options/pipeline_options.py
##########
@@ -219,6 +231,11 @@ def __init__(self, flags=None, **kwargs):
self._all_options[option_name] = getattr(
self._visible_options, option_name)
+ # Override flags dependent on each other
+ for option_name, option_value in _PARSE_OPTIONS_WITH_PREFIX_NO.items():
Review comment:
this sounds like you are talking about options and their values.
how about: option_name, option_dest
--
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]
