From 0830d85d80a774fe7965b9c220b1f861db9b07e2 Mon Sep 17 00:00:00 2001 From: Romain Cledat Date: Wed, 15 Jan 2025 00:54:34 -0800 Subject: [PATCH] Fix an issue with configs that were None If a config was None (ie: no default, default value and not required), in certain circumstances, Metaflow would crash. Fixes #2191 --- metaflow/parameters.py | 2 +- metaflow/runner/click_api.py | 2 ++ metaflow/user_configs/config_options.py | 11 +++++++---- metaflow/user_configs/config_parameters.py | 14 +++++++------- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/metaflow/parameters.py b/metaflow/parameters.py index 8d4c86cf5f7..214769a03ef 100644 --- a/metaflow/parameters.py +++ b/metaflow/parameters.py @@ -316,7 +316,7 @@ class MyFlow(FlowSpec): help : str, optional, default None Help text to show in `run --help`. required : bool, optional, default None - Require that the user specified a value for the parameter. Note that if + Require that the user specifies a value for the parameter. Note that if a default is provide, the required flag is ignored. A value of None is equivalent to False. show_default : bool, optional, default None diff --git a/metaflow/runner/click_api.py b/metaflow/runner/click_api.py index 23e9d326c9f..4d10dcdc8a2 100644 --- a/metaflow/runner/click_api.py +++ b/metaflow/runner/click_api.py @@ -371,6 +371,8 @@ def execute(self) -> List[str]: else: components.append(v) for k, v in options.items(): + if v is None: + continue if isinstance(v, list): for i in v: if isinstance(i, tuple): diff --git a/metaflow/user_configs/config_options.py b/metaflow/user_configs/config_options.py index 2c78f9399ab..341e775dd99 100644 --- a/metaflow/user_configs/config_options.py +++ b/metaflow/user_configs/config_options.py @@ -169,7 +169,7 @@ def get_config(cls, config_name: str) -> Optional[Dict[Any, Any]]: "Please contact support." ) cls.loaded_configs = all_configs - return cls.loaded_configs.get(config_name, None) + return cls.loaded_configs[config_name] def process_configs( self, @@ -326,6 +326,8 @@ def process_configs( for name, val in merged_configs.items(): if val is None: missing_configs.add(name) + to_return[name] = None + flow_cls._flow_state[_FlowState.CONFIGS][name] = None continue if val.startswith(_CONVERTED_DEFAULT_NO_FILE): no_default_file.append(name) @@ -339,15 +341,16 @@ def process_configs( val = val[len(_DEFAULT_PREFIX) :] if val.startswith("kv."): # This means to load it from a file - read_value = self.get_config(val[3:]) - if read_value is None: + try: + read_value = self.get_config(val[3:]) + except KeyError as e: exc = click.UsageError( "Could not find configuration '%s' in INFO file" % val ) if click_obj: click_obj.delayed_config_exception = exc return None - raise exc + raise exc from e flow_cls._flow_state[_FlowState.CONFIGS][name] = read_value to_return[name] = ConfigValue(read_value) else: diff --git a/metaflow/user_configs/config_parameters.py b/metaflow/user_configs/config_parameters.py index 074a44810ee..a430285c9d5 100644 --- a/metaflow/user_configs/config_parameters.py +++ b/metaflow/user_configs/config_parameters.py @@ -290,17 +290,17 @@ class Config(Parameter, collections.abc.Mapping): default : Union[str, Callable[[ParameterContext], str], optional, default None Default path from where to read this configuration. A function implies that the value will be computed using that function. - You can only specify default or default_value. + You can only specify default or default_value, not both. default_value : Union[str, Dict[str, Any], Callable[[ParameterContext, Union[str, Dict[str, Any]]], Any], optional, default None Default value for the parameter. A function implies that the value will be computed using that function. - You can only specify default or default_value. + You can only specify default or default_value, not both. help : str, optional, default None Help text to show in `run --help`. required : bool, optional, default None - Require that the user specified a value for the configuration. Note that if - a default is provided, the required flag is ignored. A value of None is - equivalent to False. + Require that the user specifies a value for the configuration. Note that if + a default or default_value is provided, the required flag is ignored. + A value of None is equivalent to False. parser : Union[str, Callable[[str], Dict[Any, Any]]], optional, default None If a callable, it is a function that can parse the configuration string into an arbitrarily nested dictionary. If a string, the string should refer to @@ -330,13 +330,13 @@ def __init__( **kwargs: Dict[str, str] ): - if default and default_value: + if default is not None and default_value is not None: raise MetaflowException( "For config '%s', you can only specify default or default_value, not both" % name ) self._default_is_file = default is not None - kwargs["default"] = default or default_value + kwargs["default"] = default if default is not None else default_value super(Config, self).__init__( name, required=required, help=help, type=str, **kwargs )