From 2cd6fb8d7587e2d9e62c4ce00b92f4c8b4521b66 Mon Sep 17 00:00:00 2001 From: Robert Bradshaw Date: Mon, 16 Sep 2024 15:27:50 -0700 Subject: [PATCH 1/3] Better error message for incorrect pipeline options flags. This prevents obscure errors later on when this single string is "iterated" over into its characters. --- sdks/python/apache_beam/options/pipeline_options.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/sdks/python/apache_beam/options/pipeline_options.py b/sdks/python/apache_beam/options/pipeline_options.py index 0f8457a40a7b..bf9e71ca8798 100644 --- a/sdks/python/apache_beam/options/pipeline_options.py +++ b/sdks/python/apache_beam/options/pipeline_options.py @@ -26,6 +26,7 @@ from typing import Any from typing import Callable from typing import Dict +from typing import Iterable from typing import List from typing import Optional from typing import Type @@ -185,7 +186,7 @@ def _add_argparse_args(cls, parser): By default the options classes will use command line arguments to initialize the options. """ - def __init__(self, flags=None, **kwargs): + def __init__(self, flags: Optional[Iterable[str]] = None, **kwargs): # type: (Optional[List[str]], **Any) -> None """Initialize an options class. @@ -216,6 +217,12 @@ def __init__(self, flags=None, **kwargs): # self._flags stores a list of not yet parsed arguments, typically, # command-line flags. This list is shared across different views. # See: view_as(). + if isinstance(flags, str): + # Unfortunately a single str passes the Iterable[str] test, as it is + # an iterable of single characters. This is almost certainly not the + # intent... + raise ValueError( + "Flags must be an iterable of of strings, not a single string.") self._flags = flags # Build parser that will parse options recognized by the [sub]class of From e3f8c20eeeae9e3c6721b67e00d5d0df343268f0 Mon Sep 17 00:00:00 2001 From: Robert Bradshaw Date: Mon, 16 Sep 2024 16:04:20 -0700 Subject: [PATCH 2/3] More conservative args parsing. --- sdks/python/apache_beam/options/pipeline_options.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sdks/python/apache_beam/options/pipeline_options.py b/sdks/python/apache_beam/options/pipeline_options.py index bf9e71ca8798..a13e30a727ec 100644 --- a/sdks/python/apache_beam/options/pipeline_options.py +++ b/sdks/python/apache_beam/options/pipeline_options.py @@ -362,6 +362,9 @@ def get_all_options( 'used for internal purposes.' % (','.join(unknown_args))) i = 0 while i < len(unknown_args): + # End of argument parsing. + if unknown_args[i] == '--': + break # Treat all unary flags as booleans, and all binary argument values as # strings. if not unknown_args[i].startswith('-'): From e5f454b04b35f3d14a264b617331d2b155e167cf Mon Sep 17 00:00:00 2001 From: Robert Bradshaw Date: Tue, 17 Sep 2024 14:53:50 -0700 Subject: [PATCH 3/3] Fix mypy strictness. --- sdks/python/apache_beam/options/pipeline_options.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/sdks/python/apache_beam/options/pipeline_options.py b/sdks/python/apache_beam/options/pipeline_options.py index a13e30a727ec..522fe11c1395 100644 --- a/sdks/python/apache_beam/options/pipeline_options.py +++ b/sdks/python/apache_beam/options/pipeline_options.py @@ -26,9 +26,9 @@ from typing import Any from typing import Callable from typing import Dict -from typing import Iterable from typing import List from typing import Optional +from typing import Sequence from typing import Type from typing import TypeVar @@ -186,9 +186,7 @@ def _add_argparse_args(cls, parser): By default the options classes will use command line arguments to initialize the options. """ - def __init__(self, flags: Optional[Iterable[str]] = None, **kwargs): - # type: (Optional[List[str]], **Any) -> None - + def __init__(self, flags: Optional[Sequence[str]] = None, **kwargs) -> None: """Initialize an options class. The initializer will traverse all subclasses, add all their argparse