From 8943362afdbf561a069792c8004e134985a6bc30 Mon Sep 17 00:00:00 2001 From: Mauricio Villegas <5780272+mauvilsa@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:26:20 +0200 Subject: [PATCH 1/2] dict_kwargs restricted to classes with an unresolved **kwargs --- CHANGELOG.rst | 8 ++ DOCUMENTATION.rst | 41 +++++--- jsonargparse/_actions.py | 2 + jsonargparse/_completions_jsonschema.py | 30 +++--- jsonargparse/_core.py | 1 + jsonargparse/_parameter_resolvers.py | 96 +++++++++++++------ jsonargparse/_signatures.py | 18 +++- jsonargparse/_typehints.py | 11 ++- .../test_completions_jsonschema.py | 65 ++++++++++++- .../test_parameter_resolvers.py | 77 ++++++++++++++- jsonargparse_tests/test_pydantic.py | 9 ++ jsonargparse_tests/test_stubs_resolver.py | 9 ++ jsonargparse_tests/test_subclasses.py | 57 +++++++++++ 13 files changed, 365 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ff7d2e8e..f12d8dfb 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -104,6 +104,14 @@ Changed - Relative paths in config files are now resolved without changing the process working directory (`#979 `__). +- ``dict_kwargs`` in a subclass spec is now restricted to classes that have a + ``**kwargs`` the parameter resolvers are unable to resolve, which the help of + the class notes. For other classes, a ``dict_kwargs`` key that is not a + parameter of the class fails, instead of being silently ignored or only + noticed as a ``TypeError`` when the class is instantiated. Resolved parameters + are meant to be given in ``init_args``, and the ``jsonschema`` completion type + only describes them there, see :ref:`unresolved-parameters` (`#981 + `__). Removed ^^^^^^^ diff --git a/DOCUMENTATION.rst b/DOCUMENTATION.rst index 7fe8baaf..5e702b7e 100644 --- a/DOCUMENTATION.rst +++ b/DOCUMENTATION.rst @@ -1612,7 +1612,9 @@ A wide range of type hints is supported for signature parameters, see unless they are required. - The ``skip`` parameter excludes arguments, e.g. - ``parser.add_method_arguments(MyClass, 'mymethod', skip={'baz'})``. + ``parser.add_method_arguments(MyClass, 'mymethod', skip={'baz'})``. In a + subclass spec, a skipped parameter can still be given in ``dict_kwargs``, see + :ref:`unresolved-parameters`. .. note:: @@ -1772,6 +1774,8 @@ assumptions resolver, based on assumptions about class inheritance, is the fallback for when AST fails. The stubs resolver, which uses ``*.pyi`` stub files, is applied on top of both. +.. _unresolved-parameters: + Unresolved parameters ^^^^^^^^^^^^^^^^^^^^^ @@ -1795,8 +1799,7 @@ Take for example the following parsing and instantiation: class MyClass: def __init__(self, foo: int = 0, **kwargs): - super().__init__(**kwargs) - ... + self.kwargs = kwargs MyClass.__module__ = "jsonargparse_tests" @@ -1811,8 +1814,8 @@ Take for example the following parsing and instantiation: cfg = parser.parse_args() cfg_init = parser.instantiate(cfg) -If ``MyClass.__init__`` has ``**kwargs`` with some unresolved parameters, the -following could be a valid config file: +Since the resolvers can't determine where the ``**kwargs`` of +``MyClass.__init__`` go, the following is a valid config file: .. code-block:: yaml @@ -1823,7 +1826,18 @@ following could be a valid config file: bar: 2 The value for ``bar`` is not validated, but the class is instantiated as -``MyClass(foo=1, bar=2)``. +``MyClass(foo=1, bar=2)``. The help of a class, e.g. ``--myclass.help=MyClass``, +notes when it accepts extra keyword arguments through ``dict_kwargs``. + +Resolved parameters are meant to be given in ``init_args``. When a class has no +unresolved ``**kwargs``, a ``dict_kwargs`` key that is not one of its parameters +fails during parsing. Keys that the class does accept are moved to +``init_args``, so that configs keep working when an improvement of the resolvers +turns an unresolved parameter into a resolved one. + +A parameter excluded with ``skip`` is still a parameter of the class, so +``dict_kwargs`` accepts it. This is how to give a value to a parameter that had +to be skipped, e.g. an untyped mandatory one. Assumptions resolver ^^^^^^^^^^^^^^^^^^^^ @@ -3358,12 +3372,15 @@ Subclasses and types that are used in more than one place are added once to The schema is meant to accept what the parser accepts, but for subclass types it is stricter. A string is accepted, since it can be a class path or a path to a sub-config file. An object is only accepted for the known subclasses, i.e. one -with ``class_path``, ``init_args`` (required only for the subclasses that have a -required init parameter) and ``dict_kwargs``. Accepting any ``class_path`` would -keep tools from suggesting the known subclasses and from pointing out a class -path that has a typo or is not the accepted import path, and its ``init_args`` -would go undescribed. Any ``class_path`` is accepted only when a type has no -known subclass, and then its ``init_args`` are not described. +with ``class_path`` and ``init_args`` (required only for the subclasses that +have a required init parameter). Accepting any ``class_path`` would keep tools +from suggesting the known subclasses and from pointing out a class path that has +a typo or is not the accepted import path, and its ``init_args`` would go +undescribed. Any ``class_path`` is accepted only when a type has no known +subclass, and then its ``init_args`` are not described. Likewise, +``dict_kwargs`` is only accepted for subclasses that have an unresolved +``**kwargs``, so a skipped parameter given there is rejected even though the +parser accepts it, see :ref:`unresolved-parameters`. A union that has a subtype accepting anything, i.e. ``Any`` or an unvalidated type, is kept as ``{"anyOf": [..., {}]}`` instead of the equivalent ``{}``, so diff --git a/jsonargparse/_actions.py b/jsonargparse/_actions.py index 56831542..3442daaf 100644 --- a/jsonargparse/_actions.py +++ b/jsonargparse/_actions.py @@ -418,6 +418,8 @@ def print_help(self, call_args): if partial_skip_args: sub_add_kwargs.setdefault("skip", set()).update(partial_skip_args) subparser.add_class_arguments(val_class, dest, **sub_add_kwargs) + if subparser._accepted_kwargs[dest] is True: + subparser.epilog = "Extra keyword arguments are accepted through dict_kwargs." subparser._inner_parser = True remove_actions(subparser, (_HelpAction, _ActionPrintConfig, _ActionConfigLoad)) args = self.get_args_after_opt(parser.args) diff --git a/jsonargparse/_completions_jsonschema.py b/jsonargparse/_completions_jsonschema.py index 481986c4..41ed7b45 100644 --- a/jsonargparse/_completions_jsonschema.py +++ b/jsonargparse/_completions_jsonschema.py @@ -544,14 +544,12 @@ def subclass_def(self, class_type, action) -> dict: def class_path_schema(self, class_path: str, action) -> dict: schema = new_object(get_doc_short_description(import_object(class_path))) - init_args_schema = self.class_parser_schema(class_path, action) - schema["properties"].update( - { - "class_path": {"const": class_path}, - "init_args": init_args_schema, - "dict_kwargs": {"type": "object"}, - } - ) + class_parser = self.get_class_parser(class_path, action) + init_args_schema = {"type": "object"} if class_parser is None else self.parser_schema(class_parser) + schema["properties"].update({"class_path": {"const": class_path}, "init_args": init_args_schema}) + # resolved parameters are only described in init_args, even though parsing also takes them from dict_kwargs + if class_parser is None or class_parser._accepted_kwargs[None] is True: + schema["properties"]["dict_kwargs"] = {"type": "object"} # init_args can only be omitted when none of the init parameters is required schema["required"] = ["class_path"] + (["init_args"] if init_args_schema.get("required") else []) return schema @@ -569,14 +567,22 @@ def unknown_class_path_schema(self) -> dict: schema["required"] = ["class_path"] return schema - def class_parser_schema(self, class_type, action, description: Optional[str] = None) -> dict: + def get_class_parser(self, class_type, action): sub_add_kwargs = dict(getattr(action, "sub_add_kwargs", None) or {}) sub_add_kwargs.pop("linked_targets", None) try: - class_parser = ActionTypeHint.get_class_parser(class_type, sub_add_kwargs=sub_add_kwargs) + return ActionTypeHint.get_class_parser(class_type, sub_add_kwargs=sub_add_kwargs) except Exception as ex: action.logger.debug(f"Unable to get schema for init args of '{class_type}': {ex}") - return {"type": "object"} + return None + + def parser_schema(self, parser, description: Optional[str] = None) -> dict: schema = new_object(description) - self.add_properties(class_parser, schema) + self.add_properties(parser, schema) return schema + + def class_parser_schema(self, class_type, action, description: Optional[str] = None) -> dict: + class_parser = self.get_class_parser(class_type, action) + if class_parser is None: + return {"type": "object"} + return self.parser_schema(class_parser, description) diff --git a/jsonargparse/_core.py b/jsonargparse/_core.py index 9c663a57..c966a526 100644 --- a/jsonargparse/_core.py +++ b/jsonargparse/_core.py @@ -138,6 +138,7 @@ class ActionsContainer(ArgumentLinking, InstantiateMethod, SignatureArguments, a def __init__(self, *args, **kwargs) -> None: """Initializer for ActionsContainer instance.""" super().__init__(*args, **kwargs) + self._accepted_kwargs = {} self.register("type", None, identity) self.register("action", "parsers", ActionSubCommands) self.register("action", "config", ActionConfigFile) diff --git a/jsonargparse/_parameter_resolvers.py b/jsonargparse/_parameter_resolvers.py index cbb0b916..3dc1e39a 100644 --- a/jsonargparse/_parameter_resolvers.py +++ b/jsonargparse/_parameter_resolvers.py @@ -12,7 +12,7 @@ from functools import partial, partialmethod from importlib import import_module from types import MethodType -from typing import Any, Union +from typing import Any, Literal, Union from ._common import ( LoggerProperty, @@ -267,7 +267,7 @@ def remove_given_parameters(node, params, removed_params: set | None = None): given_args = set(ast_get_call_positional_indexes(node)) given_kwargs = set(ast_get_call_keyword_names(node)) input_params = params - params = [p for n, p in enumerate(params) if n not in given_args] + params = [p for n, p in enumerate(params) if n not in given_args or p.kind is kinds.VAR_KEYWORD] params = [p for p in params if p.name not in given_kwargs] if removed_params is not None and len(params) < len(input_params): removed_params.update(p.name for p in input_params if p.name in given_kwargs) @@ -278,6 +278,31 @@ def get_arg_kind_index(params, kind): return next((n for n, p in enumerate(params) if p.kind == kind), -1) +def get_unresolved_kwargs_param(component, parent) -> ParamData: + """Parameter that stands for a ``**kwargs`` that the resolvers were unable to resolve. + + Its presence means that keyword arguments other than the resolved ones are accepted. The name + is not a valid identifier so that it can't collide with a real parameter. + """ + return ParamData(name="**", annotation=inspect._empty, kind=kinds.VAR_KEYWORD, component=component, parent=parent) + + +def accepts_unresolved_kwargs(params: ParamList) -> bool: + """Whether the parameters include an unresolved ``**kwargs``.""" + return any(p.kind is kinds.VAR_KEYWORD for p in params) + + +def remove_unresolved_kwargs(params: ParamList) -> ParamList: + return [p for p in params if p.kind is not kinds.VAR_KEYWORD] + + +def get_accepted_kwargs(params: ParamList) -> set[str] | Literal[True]: + """Names of the keyword arguments accepted, or True when any name is accepted.""" + if accepts_unresolved_kwargs(params): + return True + return {n for p in params for n in (p.name, *(p.aliases or ()))} + + def get_signature_parameters_and_indexes(component, parent, logger): signature_source = component if is_classmethod(parent, component): @@ -436,7 +461,7 @@ def is_param_subclass_instance_default(param: ParamData) -> bool: def split_args_and_kwargs(params: ParamList) -> tuple[ParamList, ParamList]: args = [p for p in params if p.kind == kinds.POSITIONAL_ONLY] - kwargs = [p for p in params if p.kind in {kinds.KEYWORD_ONLY, kinds.POSITIONAL_OR_KEYWORD}] + kwargs = [p for p in params if p.kind in {kinds.KEYWORD_ONLY, kinds.POSITIONAL_OR_KEYWORD, kinds.VAR_KEYWORD}] return args, kwargs @@ -757,7 +782,9 @@ def match_call_that_uses_attr(self, node, source, attr_name): get_param_args = self.get_node_component(node, source) if get_param_args: try: - params = get_signature_parameters(*get_param_args, logger=self.logger) + params = get_signature_parameters( + *get_param_args, logger=self.logger, include_var_keyword=True + ) except Exception: self.log_debug(f"failed to get parameters for call that uses attr: {get_param_args}") params = remove_given_parameters(node, params) @@ -835,10 +862,11 @@ def get_kwargs_pop_or_get_parameter(self, node, component, parent, doc_params): origin=param_kwargs_pop_or_get + self.get_node_origin(node), ) - def get_parameters_args_and_kwargs(self) -> tuple[ParamList, ParamList]: + def get_parameters_args_and_kwargs(self, kwargs_idx: int) -> tuple[ParamList, ParamList]: self.parse_source_tree() args_name = getattr(self.component_node.args.vararg, "arg", None) - kwargs_name = getattr(self.component_node.args.kwarg, "arg", None) + # kwargs_idx is negative when the **kwargs was already replaced, i.e. an Unpack[TypedDict] + kwargs_name = getattr(self.component_node.args.kwarg, "arg", None) if kwargs_idx >= 0 else None values_to_find = {} if args_name: values_to_find[args_name] = ast_variable_load(args_name) @@ -846,15 +874,17 @@ def get_parameters_args_and_kwargs(self) -> tuple[ParamList, ParamList]: values_to_find[kwargs_name] = ast_variable_load(kwargs_name) values_found = self.find_values_usage(values_to_find) - if not values_found: - return [], [] + kwargs_uses = [(v, s) for k, v, s in values_found if k == kwargs_name] if kwargs_name else [] + # a **kwargs not used in the body is discarded by the component, but still accepted + kwargs_unresolved = bool(kwargs_name) and not kwargs_uses params_list = [] removed_params: set[str] = set() pop_or_get_params: set[str] = set() kwargs_value = kwargs_name and values_to_find[kwargs_name] kwargs_value_dump = kwargs_value and ast.dump(kwargs_value) - for node, source in [(v, s) for k, v, s in values_found if k == kwargs_name]: + for node, source in kwargs_uses: + params = None # None means that where the kwargs go could not be determined if isinstance(node, ast.Call): if ast_is_kwargs_pop_or_get(node, kwargs_value_dump): param = self.get_kwargs_pop_or_get_parameter(node, self.component, self.parent, self.doc_params) @@ -862,39 +892,39 @@ def get_parameters_args_and_kwargs(self) -> tuple[ParamList, ParamList]: params_list.append([param]) continue kwarg = ast_get_call_kwarg_with_value(node, kwargs_value) - params = [] if kwarg.arg: self.log_debug(f"kwargs given as keyword parameter not supported: {ast.unparse(node)}") elif self.parent and ast_is_super_call(node): if ast_is_supported_super_call(node, self.self_name, self.log_debug): params = get_mro_parameters( node.func.attr, # type: ignore[attr-defined] - get_signature_parameters, + partial(get_signature_parameters, include_var_keyword=True), self.logger, ) else: get_param_args = self.get_node_component(node, source) if get_param_args: - params = get_signature_parameters(*get_param_args, logger=self.logger) - params = remove_given_parameters(node, params, removed_params) - if params: - self.add_node_origins(params, node) - params_list.append(params) - elif isinstance(node, ast_assign_type): - self_attr = self.parent and ast_is_attr_assign(node, self.self_name) - if self_attr: - params = self.get_parameters_attr_use_in_members(self_attr) - if params: - self.add_node_origins(params, node) - params_list.append(params) - else: - self.log_debug(f"unsupported type of assign: {ast.unparse(node)}") + params = get_signature_parameters(*get_param_args, logger=self.logger, include_var_keyword=True) + if params is not None: + params = remove_given_parameters(node, params, removed_params) + elif self.parent and (self_attr := ast_is_attr_assign(node, self.self_name)): + params = self.get_parameters_attr_use_in_members(self_attr) or None + else: + self.log_debug(f"unsupported type of assign: {ast.unparse(node)}") + if params is None: + kwargs_unresolved = True + elif params: + self.add_node_origins(params, node) + params_list.append(params) params = group_parameters(params_list) # a pop/get from kwargs means the parameter is accepted, even if the value is then given explicitly removed_params -= pop_or_get_params params = [p for p in params if p.name not in removed_params] - return split_args_and_kwargs(params) + args, kwargs = split_args_and_kwargs(params) + if kwargs_unresolved and not accepts_unresolved_kwargs(kwargs): + kwargs.append(get_unresolved_kwargs_param(self.component, self.parent)) + return args, kwargs def get_parameters_attr_use_in_members(self, attr_name) -> ParamList: attr_value = ast_attribute_load(self.self_name, attr_name) @@ -955,7 +985,7 @@ def get_parameters(self) -> ParamList: if args_idx >= 0 or kwargs_idx >= 0: self.doc_params = doc_params with mro_context(self.parent): - args, kwargs = self.get_parameters_args_and_kwargs() + args, kwargs = self.get_parameters_args_and_kwargs(kwargs_idx) params = replace_args_and_kwargs(params, args, kwargs) add_stub_types(stubs, params, self.component) params = self.remove_ignore_parameters(params) @@ -1198,7 +1228,8 @@ def get_parameters_from_stubs( if stub_import: origin = get_parameter_origins(component, parent) aliases = resolver.get_aliases(stub_import) - arg_asts = stub_import.info.ast.args.args + stub_import.info.ast.args.kwonlyargs + args_ast = stub_import.info.ast.args + arg_asts = args_ast.args + args_ast.kwonlyargs params = [] for num, arg_ast in enumerate(arg_asts): if parent and num == 0: @@ -1218,6 +1249,8 @@ def get_parameters_from_stubs( origin=origin, ) ) + if args_ast.kwarg: + params.append(get_unresolved_kwargs_param(component, parent)) return params @@ -1236,7 +1269,7 @@ def get_parameters_by_assumptions( args, kwargs = split_args_and_kwargs(subparams) params = replace_args_and_kwargs(params, args, kwargs) - params = replace_args_and_kwargs(params, [], []) + params = replace_args_and_kwargs(params, [], [get_unresolved_kwargs_param(component, parent)]) add_stub_types(stubs, params, component) return params @@ -1245,6 +1278,7 @@ def get_signature_parameters( function_or_class: Callable | type, method_or_property: str | None = None, logger: bool | str | dict | logging.Logger = True, + include_var_keyword: bool = False, ) -> ParamList: """Get parameters by inspecting ASTs, stubs or by inheritance assumptions. @@ -1258,6 +1292,8 @@ def get_signature_parameters( which to get the signature parameters. If not provided it returns the parameters for ``__init__``. logger: Useful for debugging. Only logs at ``DEBUG`` level. + include_var_keyword: Whether to include a ``**kwargs`` that could not be resolved, see + :func:`get_unresolved_kwargs_param`. """ from ._typehints import is_namedtuple, is_typed_dict @@ -1295,4 +1331,6 @@ def get_signature_parameters( attr = inspect.getattr_static(get_generic_origin(parent), method_name) if is_partial_method(attr) and component is attr.func: params = apply_partial_method(params, attr) + if not include_var_keyword: + params = remove_unresolved_kwargs(params) return params diff --git a/jsonargparse/_signatures.py b/jsonargparse/_signatures.py index aa22d4c0..10a8f4ba 100644 --- a/jsonargparse/_signatures.py +++ b/jsonargparse/_signatures.py @@ -25,7 +25,13 @@ is_attrs_class, is_pydantic_model, ) -from ._parameter_resolvers import ParamData, get_parameter_origins, get_signature_parameters +from ._parameter_resolvers import ( + ParamData, + get_accepted_kwargs, + get_parameter_origins, + get_signature_parameters, + remove_unresolved_kwargs, +) from ._required import set_required from ._typehints import ( ActionTypeHint, @@ -61,6 +67,11 @@ def validate_fail_untyped(fail_untyped) -> None: class SignatureArguments(LoggerProperty): """Methods to add arguments based on signatures to an :class:`ArgumentParser` instance.""" + # Names of the keyword arguments that a signature accepts, keyed by its nested_key. True means + # that keyword arguments other than the resolved ones are also accepted, i.e. the signature has + # a **kwargs that the parameter resolvers were unable to resolve. + _accepted_kwargs: dict[str | None, set[str] | Literal[True]] + def add_class_arguments( self, class_type: type, @@ -291,7 +302,10 @@ def _add_signature_arguments( ValueError: When there are parameters without a type that fail_untyped requires to have one. """ validate_fail_untyped(fail_untyped) - params = get_signature_parameters(function_or_class, method_name, logger=self.logger) + params = get_signature_parameters(function_or_class, method_name, logger=self.logger, include_var_keyword=True) + parser = self.parser if hasattr(self, "parser") else self + parser._accepted_kwargs[nested_key] = get_accepted_kwargs(params) + params = remove_unresolved_kwargs(params) skip_positionals = [s for s in (skip or []) if isinstance(s, int) and s != 0] if skip_positionals: diff --git a/jsonargparse/_typehints.py b/jsonargparse/_typehints.py index 38d752e1..58b9cb2c 100644 --- a/jsonargparse/_typehints.py +++ b/jsonargparse/_typehints.py @@ -2572,8 +2572,17 @@ def adapt_class_type( else: if isinstance(dict_kwargs, dict): for key in list(dict_kwargs): - if find_action(parser, key): + # an extra key of a pydantic model that accepts them is also given through init_args + if find_action(parser, key) or parser._accepts_extra_key(key): init_args[key] = dict_kwargs.pop(key) + accepted_kwargs = parser._accepted_kwargs[None] + unexpected = [] if accepted_kwargs is True else [k for k in dict_kwargs if k not in accepted_kwargs] + if unexpected: + raise ValueError( + f"{value['class_path']} does not have an unresolved **kwargs, thus dict_kwargs only " + f"accepts keys that are parameters of its signature. Unexpected keys: " + f"{iter_to_set_str(unexpected)}" + ) elif dict_kwargs: init_args["dict_kwargs"] = dict_kwargs dict_kwargs = None diff --git a/jsonargparse_tests/test_completions_jsonschema.py b/jsonargparse_tests/test_completions_jsonschema.py index 12cab10d..1f724f4d 100644 --- a/jsonargparse_tests/test_completions_jsonschema.py +++ b/jsonargparse_tests/test_completions_jsonschema.py @@ -44,6 +44,7 @@ Email, Path_fr, PositiveInt, + final, register_type, restricted_number_type, ) @@ -566,7 +567,7 @@ def test_subclass_type(parser): for entry in entries.values(): assert entry["type"] == "object" assert entry["additionalProperties"] is False - assert entry["properties"]["dict_kwargs"] == {"type": "object"} + assert "dict_kwargs" not in entry["properties"] # all init parameters are resolved init_args = {path: entry["properties"]["init_args"] for path, entry in entries.items()} assert set(config_properties(init_args[base_paths[0]])) == {"base"} assert set(config_properties(init_args[base_paths[1]])) == {"sub"} @@ -612,7 +613,6 @@ def test_subclass_type_validation(parser): # all forms that the parser accepts validate(schema, {"cls": {"class_path": f"{__name__}.Sub", "init_args": {"sub": 2}}}) validate(schema, {"cls": {"class_path": f"{__name__}.Sub"}}) - validate(schema, {"cls": {"class_path": f"{__name__}.Sub", "dict_kwargs": {"extra": 1}}}) validate(schema, {"cls": f"{__name__}.Sub"}) validate(schema, {"cls": "Sub"}) validate(schema, {"cls": "sub_config.yaml"}) @@ -620,11 +620,56 @@ def test_subclass_type_validation(parser): assert iter_errors(schema, {"cls": {"class_path": f"{__name__}.Sub", "init_args": {"flag": True}}}) assert iter_errors(schema, {"cls": {"class_path": f"{__name__}.RequiredSub"}}) assert iter_errors(schema, {"cls": {"class_path": f"{__name__}.Sub", "bogus": 1}}) + assert iter_errors(schema, {"cls": {"class_path": f"{__name__}.Sub", "dict_kwargs": {"extra": 1}}}) assert iter_errors(schema, {"cls": {"init_args": {"sub": 2}}}) # stricter than the parser, so that the known subclasses are suggested and validated assert iter_errors(schema, {"cls": {"class_path": "not_imported.Class", "init_args": {"anything": 1}}}) +class ResolvedKwargs: + def __init__(self, p1: int = 1, **kwargs): + resolved_kwargs_target(**kwargs) # pragma: no cover + + +def resolved_kwargs_target(p2: str = "2"): + pass # pragma: no cover + + +class UnresolvedKwargs: + def __init__(self, p1: int = 1, **kwargs): + pass # pragma: no cover + + +def test_subclass_dict_kwargs_excluded_when_resolved(parser): + parser.add_argument("--cls", type=ResolvedKwargs) + entry = class_path_entries(get_schema(parser)["$defs"]["ResolvedKwargs"])[f"{__name__}.ResolvedKwargs"] + assert set(config_properties(entry)) == {"class_path", "init_args"} + assert set(config_properties(entry["properties"]["init_args"])) == {"p1", "p2"} + + +def test_subclass_dict_kwargs_excluded_for_skipped_parameter(parser): + # parsing accepts p1 in dict_kwargs, but the schema only describes what init_args accepts + parser.add_subclass_arguments(ResolvedKwargs, "cls", skip={"p1"}) + entry = class_path_entries(get_schema(parser)["$defs"]["ResolvedKwargs"])[f"{__name__}.ResolvedKwargs"] + assert set(config_properties(entry)) == {"class_path", "init_args"} + assert set(config_properties(entry["properties"]["init_args"])) == {"p2"} + + +def test_subclass_dict_kwargs_any_key_when_unresolved(parser): + parser.add_argument("--cls", type=UnresolvedKwargs) + entry = class_path_entries(get_schema(parser)["$defs"]["UnresolvedKwargs"])[f"{__name__}.UnresolvedKwargs"] + assert entry["properties"]["dict_kwargs"] == {"type": "object"} + + +@skip_if_jsonschema_unavailable +def test_subclass_dict_kwargs_validation(parser): + parser.add_argument("--cls", type=Union[ResolvedKwargs, UnresolvedKwargs]) + schema = get_schema(parser) + validate(schema, {"cls": {"class_path": f"{__name__}.ResolvedKwargs", "init_args": {"p2": "x"}}}) + validate(schema, {"cls": {"class_path": f"{__name__}.UnresolvedKwargs", "dict_kwargs": {"extra": 1}}}) + assert iter_errors(schema, {"cls": {"class_path": f"{__name__}.ResolvedKwargs", "dict_kwargs": {"p2": "x"}}}) + + def test_defs_discarded_when_unreachable(parser): # the nested key turns the --cls schema into an object, so the Base def must not be kept parser.add_argument("--cls", type=Optional[Base]) @@ -935,6 +980,22 @@ def test_class_without_resolvable_init_args(logger): with capture_logs(logger) as logs: entries = class_path_entries(get_schema(parser)["$defs"]["UntypedBase"]) assert entries[f"{__name__}.UntypedSub"]["properties"]["init_args"] == {"type": "object"} + assert entries[f"{__name__}.UntypedSub"]["properties"]["dict_kwargs"] == {"type": "object"} + assert "Unable to get schema for init args" in logs.getvalue() + + +@final +class UntypedFinal: + def __init__(self, param, num: int = 1): # untyped required param, so a parser for the init args fails + pass # pragma: no cover + + +def test_closed_class_without_resolvable_init_args(logger): + parser = ArgumentParser(exit_on_error=False, logger=logger) + parser.add_argument("--cls", type=Optional[UntypedFinal]) # optional, so the class is not added as a group + with capture_logs(logger) as logs: + schema = get_schema(parser) + assert schema["$defs"]["UntypedFinal"] == {"type": "object"} assert "Unable to get schema for init args" in logs.getvalue() diff --git a/jsonargparse_tests/test_parameter_resolvers.py b/jsonargparse_tests/test_parameter_resolvers.py index 65761010..550743f6 100644 --- a/jsonargparse_tests/test_parameter_resolvers.py +++ b/jsonargparse_tests/test_parameter_resolvers.py @@ -6,7 +6,7 @@ import xml.dom from functools import partialmethod from random import shuffle -from typing import Any, Callable, Dict, List, Optional, Protocol, Union +from typing import Any, Callable, Dict, List, Optional, Protocol, TypedDict, Union from unittest.mock import patch import pytest @@ -16,10 +16,12 @@ from jsonargparse._parameter_resolvers import ( ConditionalDefault, ParamData, + accepts_unresolved_kwargs, is_lambda, is_param_subclass_instance_default, ) from jsonargparse._parameter_resolvers import get_signature_parameters as get_params +from jsonargparse._typehints import Unpack from jsonargparse_tests.conftest import BaseClass, capture_logs, source_unavailable, wrap_fn @@ -1122,6 +1124,79 @@ def test_get_params_non_existent_call(logger): assert "does_not_exist" in logs.getvalue() +# unresolved kwargs tests + + +def accepts_extra_kwargs(component) -> bool: + return accepts_unresolved_kwargs(get_params(component, include_var_keyword=True)) + + +if Unpack: + UnpackParams = TypedDict("UnpackParams", {"ku1": int}) + + class ClassUnpackTypedDict: + def __init__(self, *args, **kwargs: Unpack[UnpackParams]): + self.args = args # pragma: no cover + + +class ClassExtraPositionals: + def __init__(self, ke1: int = 1, *args, **kwargs): + self.args = args # pragma: no cover + + +def function_extra_positionals(**kwargs): # pragma: no cover + return ClassExtraPositionals(1, 2, 3, **kwargs) + + +@pytest.mark.parametrize( + "component", + [ + ClassA, # kwargs not used + ClassB, # kwargs forwarded to a class that accepts extra kwargs + ClassU1, # unsupported type of assign + ClassU2, # kwargs given as keyword parameter + ClassU3, # unsupported super call + ClassU4, # self attribute not used in members + ClassU5, # kwargs attribute given as keyword parameter + ClassExtraPositionals, # kwargs not used + function_unsupported_component, # call to a component that can't be determined + function_with_bug, # call to a name that does not exist + function_extra_positionals, # call with more positionals than the resolved parameters + ], +) +def test_accepts_extra_kwargs_true(component): + assert accepts_extra_kwargs(component) is True + + +@pytest.mark.parametrize( + "component", + [ + ClassE1, # kwargs used in an attribute forwarded to a function without kwargs + ClassG, # kwargs used in methods without kwargs + ClassM1, # no kwargs in the signature + ClassP, # kwargs used in a property + conditional_calls, # kwargs forwarded to functions without kwargs + func_given_kwargs, # kwargs forwarded to a function without kwargs + function_no_args_no_kwargs, # no kwargs in the signature + function_pop_get_from_kwargs, # kwargs only popped and forwarded to a function without kwargs + function_with_kwargs, # kwargs forwarded to a function without kwargs + ], +) +def test_accepts_extra_kwargs_false(component): + assert accepts_extra_kwargs(component) is False + + +@pytest.mark.skipif(not Unpack, reason="Unpack introduced in python 3.11 or backported in typing_extensions") +def test_accepts_extra_kwargs_unpack_typed_dict(): + assert accepts_extra_kwargs(ClassUnpackTypedDict) is False + + +def test_accepts_extra_kwargs_from_assumptions(): + with source_unavailable(): + assert accepts_extra_kwargs(ClassA) is True + assert accepts_extra_kwargs(ClassM1) is False + + # failure cases diff --git a/jsonargparse_tests/test_pydantic.py b/jsonargparse_tests/test_pydantic.py index b6a39e37..5b541d7d 100644 --- a/jsonargparse_tests/test_pydantic.py +++ b/jsonargparse_tests/test_pydantic.py @@ -986,6 +986,15 @@ def test_pydantic_extra_allow_in_subclass_init_args(parser): assert init.cls.model.p3 == "y" +def test_pydantic_extra_allow_in_subclass_dict_kwargs(parser): + parser.add_argument("--model", type=Optional[PydanticExtraAllow]) + value = {"class_path": f"{__name__}.PydanticExtraAllow", "dict_kwargs": {"p1": "x", "p3": "y"}} + cfg = parser.parse_args([f"--model={json.dumps(value)}"]) + assert cfg.model == Namespace(p1="x", p2=3, p3="y") + init = parser.instantiate(cfg) + assert init.model.p3 == "y" + + def test_pydantic_extra_ignore_group_argument(parser): parser.add_argument("--model", type=PydanticExtraIgnore, default=PydanticExtraIgnore(p1="a")) cfg = parser.parse_object({"model": {"p1": "x", "p3": "y"}}) diff --git a/jsonargparse_tests/test_stubs_resolver.py b/jsonargparse_tests/test_stubs_resolver.py index f4293130..c4cbbc27 100644 --- a/jsonargparse_tests/test_stubs_resolver.py +++ b/jsonargparse_tests/test_stubs_resolver.py @@ -9,6 +9,7 @@ from email.headerregistry import DateHeader from importlib.util import find_spec from ipaddress import ip_network +from json import dumps as json_dumps from random import Random, SystemRandom, uniform from tarfile import TarFile from time import localtime, struct_time @@ -19,6 +20,7 @@ import pytest from jsonargparse import ArgumentError, set_parsing_settings +from jsonargparse._parameter_resolvers import accepts_unresolved_kwargs from jsonargparse._parameter_resolvers import get_signature_parameters as get_params from jsonargparse._stubs_resolver import get_arg_type, get_mro_method_parent, get_stubs_resolver from jsonargparse_tests.conftest import ( @@ -318,6 +320,13 @@ def test_get_params_inspect_signature_failure_function(logger): assert "get_parameters_by_assumptions failed" in logs.getvalue() +def test_get_params_inspect_signature_failure_var_keyword(): + with inspect_signature_failure(json_dumps): + params = get_params(json_dumps, include_var_keyword=True) + assert "obj" == params[0].name + assert accepts_unresolved_kwargs(params) + + def test_get_params_inspect_signature_failure_method(logger): with inspect_signature_failure(Random.randint), capture_logs(logger) as logs: params = get_params(Random, "randint", logger=logger) diff --git a/jsonargparse_tests/test_subclasses.py b/jsonargparse_tests/test_subclasses.py index 2c820208..1a5c5d09 100644 --- a/jsonargparse_tests/test_subclasses.py +++ b/jsonargparse_tests/test_subclasses.py @@ -1533,6 +1533,10 @@ def test_subclass_unresolved_parameters(parser, subtests): data = json_or_yaml_load(out)["cls"] assert data == expected.as_dict() + with subtests.test("help notes that dict_kwargs is accepted"): + help_str = get_parse_args_stdout(parser, [f"--cls.help={__name__}.UnresolvedParams"]) + assert "Extra keyword arguments are accepted through dict_kwargs." in help_str + with subtests.test("invalid dict_kwargs"): with pytest.raises(ArgumentError): parser.parse_args(["--cls=UnresolvedParams", "--cls.dict_kwargs=1"]) @@ -1556,6 +1560,59 @@ def test_subclass_unresolved_parameters_name_clash(parser): assert cfg.cls.dict_kwargs == {"p1": 3} +class ResolvedParams: + def __init__(self, p1: int = 1, **kwargs): + resolved_params_target(**kwargs) # pragma: no cover + + +def resolved_params_target(p2: str = "2"): + pass # pragma: no cover + + +dict_kwargs_not_accepted = ( + f"{__name__}.ResolvedParams does not have an unresolved " + r"\*\*kwargs, thus dict_kwargs only accepts keys that are parameters of its signature. Unexpected keys: " +) + + +def test_subclass_dict_kwargs_not_accepted(parser, subtests, tmp_cwd): + parser.add_argument("--cfg", action="config") + parser.add_argument("--cls", type=ResolvedParams) + + with subtests.test("resolved kwargs given in dict_kwargs are moved to init_args"): + cfg = parser.parse_args([f"--cls={__name__}.ResolvedParams", "--cls.dict_kwargs.p2=x"]) + assert cfg.cls.init_args == Namespace(p1=1, p2="x") + assert "dict_kwargs" not in cfg.cls + + with subtests.test("args"): + with pytest.raises(ArgumentError, match=dict_kwargs_not_accepted + "p9"): + parser.parse_args([f"--cls={__name__}.ResolvedParams", "--cls.dict_kwargs.p9=1"]) + + with subtests.test("config"): + # more than one unexpected key is listed in the order given + config = {"cls": {"class_path": f"{__name__}.ResolvedParams", "dict_kwargs": {"p9": 1, "p8": 2}}} + Path("config.yaml").write_text(json_or_yaml_dump(config)) + with pytest.raises(ArgumentError, match=dict_kwargs_not_accepted + r"\{p9,p8\}"): + parser.parse_args(["--cfg=config.yaml"]) + + with subtests.test("help does not note that dict_kwargs is accepted"): + help_str = get_parse_args_stdout(parser, [f"--cls.help={__name__}.ResolvedParams"]) + assert "dict_kwargs" not in help_str + + +def test_subclass_dict_kwargs_skipped_parameter(parser): + parser.add_subclass_arguments(ResolvedParams, "cls", skip={"p1"}) + + value = {"class_path": f"{__name__}.ResolvedParams", "dict_kwargs": {"p1": 3}} + cfg = parser.parse_args([f"--cls={json.dumps(value)}"]) + assert cfg.cls.init_args == Namespace(p2="2") + assert cfg.cls.dict_kwargs == {"p1": 3} + + value["dict_kwargs"]["p9"] = 4 + with pytest.raises(ArgumentError, match=dict_kwargs_not_accepted + "p9"): + parser.parse_args([f"--cls={json.dumps(value)}"]) + + # add_subclass_arguments tests From a2340f41f98fc05409cfa1c61e8abd6530ebccca Mon Sep 17 00:00:00 2001 From: Mauricio Villegas <5780272+mauvilsa@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:50:30 +0200 Subject: [PATCH 2/2] Address comments --- CHANGELOG.rst | 4 ++-- jsonargparse/_parameter_resolvers.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index f12d8dfb..500dda32 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -105,8 +105,8 @@ Changed working directory (`#979 `__). - ``dict_kwargs`` in a subclass spec is now restricted to classes that have a - ``**kwargs`` the parameter resolvers are unable to resolve, which the help of - the class notes. For other classes, a ``dict_kwargs`` key that is not a + ``**kwargs`` the parameter resolvers are unable to resolve, which is noted in + the class help. For other classes, a ``dict_kwargs`` key that is not a parameter of the class fails, instead of being silently ignored or only noticed as a ``TypeError`` when the class is instantiated. Resolved parameters are meant to be given in ``init_args``, and the ``jsonschema`` completion type diff --git a/jsonargparse/_parameter_resolvers.py b/jsonargparse/_parameter_resolvers.py index 3dc1e39a..25dfcf88 100644 --- a/jsonargparse/_parameter_resolvers.py +++ b/jsonargparse/_parameter_resolvers.py @@ -267,7 +267,7 @@ def remove_given_parameters(node, params, removed_params: set | None = None): given_args = set(ast_get_call_positional_indexes(node)) given_kwargs = set(ast_get_call_keyword_names(node)) input_params = params - params = [p for n, p in enumerate(params) if n not in given_args or p.kind is kinds.VAR_KEYWORD] + params = [p for n, p in enumerate(params) if n not in given_args or p.kind == kinds.VAR_KEYWORD] params = [p for p in params if p.name not in given_kwargs] if removed_params is not None and len(params) < len(input_params): removed_params.update(p.name for p in input_params if p.name in given_kwargs) @@ -289,11 +289,11 @@ def get_unresolved_kwargs_param(component, parent) -> ParamData: def accepts_unresolved_kwargs(params: ParamList) -> bool: """Whether the parameters include an unresolved ``**kwargs``.""" - return any(p.kind is kinds.VAR_KEYWORD for p in params) + return any(p.kind == kinds.VAR_KEYWORD for p in params) def remove_unresolved_kwargs(params: ParamList) -> ParamList: - return [p for p in params if p.kind is not kinds.VAR_KEYWORD] + return [p for p in params if p.kind != kinds.VAR_KEYWORD] def get_accepted_kwargs(params: ParamList) -> set[str] | Literal[True]: