Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-13709] Inconsistent behavior when parsing boolean flags across different APIs in Python SDK #16929

Merged
merged 7 commits into from
Mar 8, 2022
Merged

[BEAM-13709] Inconsistent behavior when parsing boolean flags across different APIs in Python SDK #16929

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7d122bf
Map to define relation between flags
AnandInguva Feb 8, 2022
8ac7657
Add warnings to the discarded flags
AnandInguva Feb 23, 2022
3af5a68
Fixup: lint
AnandInguva Feb 23, 2022
10cff75
Fixup: docstring
AnandInguva Feb 23, 2022
eedce2f
Refactor docstring, unittest
AnandInguva Mar 7, 2022
cb24183
Update docstring
AnandInguva Mar 7, 2022
078f58b
Change Map name and update comments
AnandInguva Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 25 additions & 48 deletions sdks/python/apache_beam/options/pipeline_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,10 @@

_LOGGER = logging.getLogger(__name__)

# These options have no dest and action is store_false in the
# argparse and default is None. 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.
_STORE_FALSE_OPTIONS_WITH_DIFFERENT_DEST = {
'use_public_ips': 'no_use_public_ips'
}
# Map the boolean option with the flag_name for the flags that have a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the following for readability:

# Map of Boolean options to flag names for options that have a destination(dest)
# in parser.add_argument() different from the flag name 
# and whose default value  is `None`. 
_FLAG_THAT_SETS_FALSE_VALUE =  {...}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable name sounds good. Changed the comments a little bit

# destination(dest) different from the flag name and the
# default value is None in parser.add_argument().
_FLAGS_WITH_DIFFERENT_DEST = {'use_public_ips': 'no_use_public_ips'}


def _static_value_provider_of(value_type):
Expand Down Expand Up @@ -190,14 +185,15 @@ def __init__(self, flags=None, **kwargs):
flags: An iterable of command line arguments to be used. If not specified
then sys.argv will be used as input for parsing arguments.

**kwargs: Add overrides for arguments passed in flags. For kwargs,
please pass the option names instead of flag names.
**kwargs: Add overrides for arguments passed in flags. For overrides
of arguments, please pass the `option names` instead of
flag names.
Option names: These are defined as dest in the
parser.add_argument(). Passing flag names like
{no_use_public_ips: True}, which is not defined to any
destination(dest) in parser, would be discarded/ignored.
Instead, pass the dest of the flag
(dest of no_use_public_ips is use_public_ips),
parser.add_argument() for each flag. Passing flags
like {no_use_public_ips: True}, for which the flag name
defined to a different destination(dest) in parser,
would be discarded. Instead, pass the dest of
the flag(dest of no_use_public_ips is use_public_ips),
Eg: {use_public_ips: False} to get the desired behavior.

"""
Expand Down Expand Up @@ -231,15 +227,6 @@ def __init__(self, flags=None, **kwargs):
# Users access this dictionary store via __getattr__ / __setattr__ methods.
self._all_options = kwargs

if self.__class__ != PipelineOptions:
_invalid_options = {}
for option_name, option_value in self._all_options.items():
if option_name not in self._visible_option_list():
_invalid_options[option_name] = option_value

if _invalid_options:
_LOGGER.warning("Discarding invalid overrides: %s", _invalid_options)

# Initialize values of keys defined by this class.
for option_name in self._visible_option_list():
# Note that options specified in kwargs will not be overwritten.
Expand All @@ -262,27 +249,25 @@ def from_dictionary(cls, options):

Returns:
A PipelineOptions object representing the given arguments.

Note: If a boolean flag is True in the dictionary,
implicitly the method assumes the boolean flag is
specified as a command line argument. If the
boolean flag is False, this method simply discards
them.
Eg: {no_auth: True} is similar to python your_file.py --no_auth
{no_auth: False} is similar to python your_file.py.
"""
flags = []
for k, v in options.items():
# Note: If a boolean flag is True in the dictionary,
# implicitly the method assumes the boolean flag is
# specified as a command line argument. If the
# boolean flag is False, this method simply discards them.
# Eg: {no_auth: True} is similar to python your_file.py --no_auth
# {no_auth: False} is similar to python your_file.py.
if isinstance(v, bool):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move the Note: here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if v:
flags.append('--%s' % k)
# capture boolean flags with 3 values
# {default=None, True, False}
elif k in _STORE_FALSE_OPTIONS_WITH_DIFFERENT_DEST:
_LOGGER.warning(
"Instead of %s=%s, please provide %s=%s" %
(k, v, _STORE_FALSE_OPTIONS_WITH_DIFFERENT_DEST[k], True))
flags.append('--%s' % _STORE_FALSE_OPTIONS_WITH_DIFFERENT_DEST[k])
elif k in _FLAGS_WITH_DIFFERENT_DEST:
# Capture overriding flags, which have a different dest
# from the flag name defined in the parser.add_argument
# Eg: no_use_public_ips, which has the dest=use_public_ips
# different from flag name
flag_that_disables_the_option = (_FLAGS_WITH_DIFFERENT_DEST[k])
flags.append('--%s' % flag_that_disables_the_option)
elif isinstance(v, list):
for i in v:
flags.append('--%s=%s' % (k, i))
Expand Down Expand Up @@ -397,14 +382,6 @@ def view_as(self, cls):
"""
view = cls(self._flags)

_invalid_options = {}
for option_name, option_value in self._all_options.items():
if option_name not in self._visible_option_list():
_invalid_options[option_name] = option_value

if _invalid_options:
_LOGGER.warning("Discarding invalid overrides: %s", _invalid_options)

for option_name in view._visible_option_list():
# Initialize values of keys defined by a cls.
#
Expand Down
61 changes: 27 additions & 34 deletions sdks/python/apache_beam/options/pipeline_options_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,53 +656,46 @@ def test_options_store_false_with_different_dest(self):
cls._add_argparse_args(parser)

actions = parser._actions.copy()
dest_to_options = {}
options_to_flags = {}
options_diff_dest_store_true = {}

for i in range(len(actions)):
options_name = actions[i].option_strings
dest = actions[i].dest
flag_names = actions[i].option_strings
option_name = actions[i].dest

if isinstance(actions[i].const, bool):
for option_name in options_name:
option_name = option_name.strip(
'--') if '--' in option_name else option_name
if option_name != dest:
for flag_name in flag_names:
flag_name = flag_name.strip('-')
if flag_name != option_name:
# Capture flags which has store_action=True and has a
# different dest. This behavior would be confusing.
if actions[i].const:
options_diff_dest_store_true[option_name] = dest
options_diff_dest_store_true[flag_name] = option_name
continue
# check the flags like no_use_public_ips
# default is None, action is {True, False}
if actions[i].default is None:
dest_to_options[dest] = option_name

assert len(options_diff_dest_store_true) == 0, (
_LOGGER.error("There should be no flags that have a dest "
"different from flag name and action as "
"store_true. It would be confusing "
"to the user. Please specify the dest as the "
"flag_name instead.")
)
options_to_flags[option_name] = flag_name

self.assertEqual(
len(options_diff_dest_store_true),
0,
_LOGGER.error(
"There should be no flags that have a dest "
"different from flag name and action as "
"store_true. It would be confusing "
"to the user. Please specify the dest as the "
"flag_name instead."))
from apache_beam.options.pipeline_options import (
_STORE_FALSE_OPTIONS_WITH_DIFFERENT_DEST)

def get_options_not_present_in_map(d1, d2):
d = {}
for k in d1:
if k not in d2:
d[k] = d1[k]
return d

assert _STORE_FALSE_OPTIONS_WITH_DIFFERENT_DEST == dest_to_options, (
"If you are adding a new boolean flag with default=None,"
" with dest different from flag name, please add the flag and "
"dest of the flag: %s to variable "
" _STORE_FALSE_OPTIONS_WITH_DIFFERENT_DEST in PipelineOptions.py" % (
get_options_not_present_in_map(dest_to_options,
_STORE_FALSE_OPTIONS_WITH_DIFFERENT_DEST))
)
_FLAGS_WITH_DIFFERENT_DEST)

self.assertDictEqual(
_FLAGS_WITH_DIFFERENT_DEST,
options_to_flags,
"If you are adding a new boolean flag with default=None,"
" with different dest/option_name from the flag name, please add "
"the dest and the flag name to the map "
"_FLAGS_WITH_DIFFERENT_DEST in PipelineOptions.py")


if __name__ == '__main__':
Expand Down