From 7f3803c30e9973270c62bf817b9fa8b6d7b4453e Mon Sep 17 00:00:00 2001 From: Mauricio Villegas <5780272+mauvilsa@users.noreply.github.com> Date: Thu, 30 Jul 2026 07:08:20 +0200 Subject: [PATCH 1/4] Fix dump yaml with comments nested cases --- CHANGELOG.rst | 5 + DOCUMENTATION.rst | 9 +- jsonargparse/_formatters.py | 250 +++++++++-- jsonargparse_tests/conftest.py | 20 +- jsonargparse_tests/test_cli.py | 12 +- jsonargparse_tests/test_core.py | 52 +-- jsonargparse_tests/test_optionals.py | 12 +- jsonargparse_tests/test_yaml_comments.py | 542 +++++++++++++++++++++++ pyproject.toml | 6 +- 9 files changed, 802 insertions(+), 106 deletions(-) create mode 100644 jsonargparse_tests/test_yaml_comments.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 77cd67ae..45322d68 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -31,6 +31,11 @@ Fixed - ``TypedDict`` keys inherited from a base defined in a different module not resolving the types only imported in the ``TYPE_CHECKING`` block of that module (`#936 `__). +- ``dump`` with ``with_comments=True``, i.e. ``--print_config=comments``, not + adding any comment for the ``init_args`` of subclasses and for the fields of + dataclass-like types that are not added as a group, including when nested in + lists and dicts (`#939 + `__). Changed ^^^^^^^ diff --git a/DOCUMENTATION.rst b/DOCUMENTATION.rst index b1e17e13..83aaba4e 100644 --- a/DOCUMENTATION.rst +++ b/DOCUMENTATION.rst @@ -1348,9 +1348,12 @@ Parsers that have an ``action="config"`` argument also include a with a large set of options to create an initial config file including all default values. If the `ruamel.yaml `__ package is installed, the config can be printed having the help descriptions -content as YAML comments by using ``--print_config=comments``. Another option is -``--print_config=skip_unset`` which skips entries whose value is the configured -unset value (see :ref:`unset-values`). +content as YAML comments by using ``--print_config=comments``. The comments +include the descriptions of the groups and arguments of the parser, and for +values that correspond to a class, e.g. the ``init_args`` of a subclass or the +fields of a dataclass, the descriptions from the respective class. Another +option is ``--print_config=skip_unset`` which skips entries whose value is the +configured unset value (see :ref:`unset-values`). From within Python it is also possible to serialize a config object by using either the :meth:`dump <.ArgumentParser.dump>` or :meth:`save diff --git a/jsonargparse/_formatters.py b/jsonargparse/_formatters.py index 7790ea51..13b45b29 100644 --- a/jsonargparse/_formatters.py +++ b/jsonargparse/_formatters.py @@ -24,6 +24,7 @@ from ._common import ( defaults_cache, get_optionals_as_positionals_actions, + is_subclasses_disabled, parent_parser, supports_optionals_as_positionals, ) @@ -50,6 +51,96 @@ class PercentTemplate(Template): """ # type: ignore[assignment] +def get_subparsers(parser: "ArgumentParser", prefix: str = "") -> dict[str | None, "ArgumentParser"]: + """Returns the given parser and all its subcommand parsers, keyed by subcommand key.""" + parsers: dict[str | None, ArgumentParser] = {} + if parser._subparsers is not None: + for key, subparser in parser._subparsers._group_actions[0].choices.items(): # type: ignore[union-attr] + full_key = (prefix + "." if prefix else "") + key + parsers[full_key] = subparser + parsers.update(get_subparsers(subparser, prefix=full_key)) + parsers[None] = parser + return parsers + + +def get_group_titles(parser: "ArgumentParser") -> dict: + """Returns a map of parser keys to the title of the group they belong to.""" + group_titles: dict = {} + for parser_key, subparser in get_subparsers(parser).items(): + group_titles[parser_key] = subparser.description + prefix = "" if parser_key is None else parser_key + "." + for group in subparser._action_groups: + actions = filter_non_parsing_actions(group._group_actions) + skip_types = (_ActionConfigLoad, ActionConfigFile, ActionSubCommands) + actions = [a for a in actions if not isinstance(a, skip_types)] + keys = {re.sub(r"\.?[^.]+$", "", a.dest) for a in actions if "." in a.dest} + for key in keys: + group_titles[prefix + key] = group.title + return group_titles + + +def get_class_group_title(class_parser: "ArgumentParser") -> str | None: + """Returns the title of the group that a class parser adds for the class itself. + + The group for the class is the first one added, the rest correspond to nested keys. + """ + groups = list((class_parser.groups or {}).values()) + return groups[0].title if groups else None + + +def get_class_parser(class_type, action: Action | None) -> "ArgumentParser | None": + """Returns a parser for the arguments of a class, or None if not possible.""" + from ._typehints import ActionTypeHint + + sub_add_kwargs = dict(getattr(action, "sub_add_kwargs", None) or {}) + sub_add_kwargs.pop("linked_targets", None) + try: + return ActionTypeHint.get_class_parser(class_type, sub_add_kwargs=sub_add_kwargs) + except Exception: + return None + + +def get_closed_type_parser(typehint, action: Action | None) -> "ArgumentParser | None": + """Returns a parser for the arguments of a closed type, e.g. a dataclass, given its type hint.""" + from ._typehints import get_subclass_or_closed_types + + if typehint is None: + return None + types = get_subclass_or_closed_types(typehint, also_lists=True, callable_return=True) + if not types or len(types) != 1 or not is_subclasses_disabled(types[0]): + return None + return get_class_parser(types[0], action) + + +def get_mapping_value_typehint(typehint): + """Returns the type of the values of a mapping type hint, or None if not a mapping.""" + from ._typehints import get_optional_arg, get_typehint_origin, mapping_origin_types + + if typehint is None: + return None + typehint = get_optional_arg(typehint) + if get_typehint_origin(typehint) in mapping_origin_types: + args = getattr(typehint, "__args__", ()) + if len(args) == 2: + return args[1] + return None + + +def remove_leading_blank_line(cfg: ruamelCommentedMap) -> None: + """Removes the blank line that comments add before the first key, needed for items of a list.""" + key = next(iter(cfg.keys()), None) + comments = cfg.ca.items.get(key, [None, None])[1] if key is not None else None + if comments and comments[0].value == "\n": + del comments[0] + + +def is_subclass_spec_dict(value) -> bool: + """Tests whether a config object corresponds to a subclass spec.""" + from ._typehints import _subclass_spec_keys + + return isinstance(value.get("class_path"), str) and not (set(value.keys()) - _subclass_spec_keys) + + class YAMLCommentFormatter: """Formatter class for adding YAML comments to configuration files.""" @@ -64,56 +155,127 @@ def add_yaml_comments(self, cfg: str) -> str: yaml = ruyaml.YAML() cfg = yaml.load(cfg) - def get_parsers(parser: ArgumentParser, prefix="") -> dict[str | None, ArgumentParser]: - parsers = {} - if parser._subparsers is not None: - for key, subparser in parser._subparsers._group_actions[0].choices.items(): # type: ignore[union-attr] - full_key = (prefix + "." if prefix else "") + key - parsers[full_key] = subparser - parsers.update(get_parsers(subparser, prefix=full_key)) - parsers[None] = parser - return parsers - parser = parent_parser.get() assert isinstance(parser, ArgumentParser) - parsers = get_parsers(parser) - - group_titles = {} - for parser_n_key, parser_n in parsers.items(): - group_titles[parser_n_key] = parser_n.description - prefix = "" if parser_n_key is None else parser_n_key + "." - for group in parser_n._action_groups: - actions = filter_non_parsing_actions(group._group_actions) - actions = [ - a for a in actions if not isinstance(a, (_ActionConfigLoad, ActionConfigFile, ActionSubCommands)) - ] - keys = {re.sub(r"\.?[^.]+$", "", a.dest) for a in actions if "." in a.dest} - for key in keys: - group_titles[prefix + key] = group.title - - def set_comments(cfg, prefix="", depth=0): - for key in cfg.keys(): - full_key = (prefix + "." if prefix else "") + key - action = find_action(parser, full_key) - text = None - if full_key in group_titles and isinstance(cfg[key], dict): - text = group_titles[full_key] - elif action is not None and action.help != SUPPRESS: - text = self.help_formatter._expand_help(action) - if isinstance(cfg[key], dict): - if text: - self.set_yaml_group_comment(text, cfg, key, depth) - set_comments(cfg[key], full_key, depth + 1) - elif text: - self.set_yaml_argument_comment(text, cfg, key, depth) - - if parser.description is not None: - self.set_yaml_start_comment(parser.description, cfg) - set_comments(cfg) + if isinstance(cfg, dict): + if parser.description is not None: + self.set_yaml_start_comment(parser.description, cfg) + self.set_comments(cfg, parser, get_group_titles(parser)) out = StringIO() yaml.dump(cfg, out) return out.getvalue() + def set_comments( + self, + cfg: ruamelCommentedMap, + parser: "ArgumentParser", + group_titles: dict, + prefix: str = "", + depth: int = 0, + ) -> None: + """Sets the comments for all the keys of a config object. + + Args: + cfg: The ruamel.yaml object. + parser: The parser from which help content is obtained. + group_titles: Map of parser keys to the title of the group they belong to. + prefix: The parser key that corresponds to the given config object. + depth: The nested level of the keys of the config object. + """ + for key in cfg.keys(): + full_key = (prefix + "." if prefix else "") + key + action = find_action(parser, full_key) + value = cfg[key] + text = None + if full_key in group_titles and isinstance(value, dict): + text = group_titles[full_key] + elif action is not None and action.help != SUPPRESS: + text = self.help_formatter._expand_help(action) + if isinstance(value, dict): + if text: + self.set_yaml_group_comment(text, cfg, key, depth) + self.set_dict_comments(value, action, depth + 1, parser, group_titles, full_key) + else: + if text: + self.set_yaml_argument_comment(text, cfg, key, depth) + if isinstance(value, list): + self.set_list_comments(value, action, depth + 1) + + def set_dict_comments( + self, + cfg: ruamelCommentedMap, + action: Action | None, + depth: int, + parser: "ArgumentParser | None" = None, + group_titles: dict | None = None, + prefix: str = "", + typehint=None, + ) -> None: + """Sets the comments for the keys of a config object nested in another one. + + Args: + cfg: The ruamel.yaml object. + action: The action that corresponds to the given config object, if any. + depth: The nested level of the keys of the config object. + parser: The parser in which the keys are searched when not a class type. + group_titles: Map of parser keys to the title of the group they belong to. + prefix: The parser key that corresponds to the given config object. + typehint: The type of the config object, defaults to the type of the action. + """ + if is_subclass_spec_dict(cfg): + self.set_subclass_comments(cfg, action, depth) + return + if typehint is None and isinstance(action, ActionTypeHint): + typehint = action._typehint + class_parser = get_closed_type_parser(typehint, action) + value_typehint = get_mapping_value_typehint(typehint) + if class_parser is not None: + self.set_comments(cfg, class_parser, get_group_titles(class_parser), depth=depth) + elif value_typehint is not None: + for value in cfg.values(): + if isinstance(value, dict): + self.set_dict_comments(value, action, depth + 1, typehint=value_typehint) + elif isinstance(value, list): + self.set_list_comments(value, action, depth + 1, typehint=value_typehint) + elif parser is not None: + assert group_titles is not None + self.set_comments(cfg, parser, group_titles, prefix, depth) + + def set_subclass_comments(self, cfg: ruamelCommentedMap, action: Action | None, depth: int) -> None: + """Sets the comments for the init args of a subclass spec. + + Args: + cfg: The ruamel.yaml object that has the class_path key. + action: The action that corresponds to the given config object, if any. + depth: The nested level of the keys of the config object. + """ + init_args = cfg.get("init_args") + if not isinstance(init_args, dict): + return + class_parser = get_class_parser(cfg["class_path"], action) + if class_parser is None: + return + title = get_class_group_title(class_parser) + if title: + self.set_yaml_group_comment(title, cfg, "init_args", depth) + self.set_comments(init_args, class_parser, get_group_titles(class_parser), depth=depth + 1) + + def set_list_comments(self, cfg: list, action: Action | None, depth: int, typehint=None) -> None: + """Sets the comments for the class types that are items of a list. + + Args: + cfg: The ruamel.yaml object. + action: The action that corresponds to the given list. + depth: The nested level of the keys of the items of the list. + typehint: The type of the list, defaults to the type of the action. + """ + for item in cfg: + if isinstance(item, dict): + self.set_dict_comments(item, action, depth, typehint=typehint) + remove_leading_blank_line(item) + elif isinstance(item, list): + self.set_list_comments(item, action, depth + 1, typehint=typehint) + def set_yaml_start_comment( self, text: str, diff --git a/jsonargparse_tests/conftest.py b/jsonargparse_tests/conftest.py index 4178ce50..a13b5620 100644 --- a/jsonargparse_tests/conftest.py +++ b/jsonargparse_tests/conftest.py @@ -13,7 +13,7 @@ import pytest -from jsonargparse import ArgumentParser, set_parsing_settings +from jsonargparse import SUPPRESS, ActionParser, ArgumentParser, set_parsing_settings from jsonargparse._loaders_dumpers import json_compact_dump, json_load, yaml_dump, yaml_load from jsonargparse._optionals import ( docstring_parser_support, @@ -22,6 +22,7 @@ jsonschema_support, omegaconf_support, pyyaml_available, + ruamel_support, toml_load_available, url_support, ) @@ -71,6 +72,11 @@ reason="docstring-parser package is required", ) +skip_if_yaml_comments_unavailable = pytest.mark.skipif( + not (ruamel_support and docstring_parser_support), + reason="ruamel.yaml and docstring-parser packages are required", +) + skip_if_requests_unavailable = pytest.mark.skipif( not url_support, reason="requests package is required", @@ -160,6 +166,18 @@ def example_parser() -> ArgumentParser: return parser +@pytest.fixture +def print_parser(parser, subparser) -> ArgumentParser: + parser.description = "cli tool" + parser.add_argument("--cfg", action="config") + parser.add_argument("--v0", help=SUPPRESS, default="0") + parser.add_argument("--v1", help="Option v1.", default=1) + parser.add_argument("--g1.v2", help="Option v2.", default="2") + subparser.add_argument("--v3") + parser.add_argument("--g2", action=ActionParser(parser=subparser)) + return parser + + @pytest.fixture def parsing_settings_patch(): with patch.dict("jsonargparse._common.parsing_settings"): diff --git a/jsonargparse_tests/test_cli.py b/jsonargparse_tests/test_cli.py index 13521119..a835aa64 100644 --- a/jsonargparse_tests/test_cli.py +++ b/jsonargparse_tests/test_cli.py @@ -14,9 +14,14 @@ from jsonargparse import CLI, auto_cli, auto_parser, capture_parser, lazy_instance from jsonargparse._namespace import Namespace -from jsonargparse._optionals import docstring_parser_support, ruamel_support +from jsonargparse._optionals import docstring_parser_support from jsonargparse.typing import final -from jsonargparse_tests.conftest import json_or_yaml_dump, json_or_yaml_load, skip_if_docstring_parser_unavailable +from jsonargparse_tests.conftest import ( + json_or_yaml_dump, + json_or_yaml_load, + skip_if_docstring_parser_unavailable, + skip_if_yaml_comments_unavailable, +) def get_cli_stdout(*args, **kwargs) -> str: @@ -399,8 +404,7 @@ def test_function_and_class_print_config_before_subcommands(): assert {"Cmd2": {"i1": "d", "method2": {"m2": 0}}} == json_or_yaml_load(out) -@skip_if_docstring_parser_unavailable -@pytest.mark.skipif(not ruamel_support, reason="ruamel.yaml package is required") +@skip_if_yaml_comments_unavailable def test_function_and_class_print_config_comments(): out = get_cli_stdout([cmd1, Cmd2, cmd3], args=["--print_config=comments", "Cmd2", "method2"]) assert "# Description of Cmd2" in out diff --git a/jsonargparse_tests/test_core.py b/jsonargparse_tests/test_core.py index 12172035..a6e0fb7f 100644 --- a/jsonargparse_tests/test_core.py +++ b/jsonargparse_tests/test_core.py @@ -26,7 +26,7 @@ ) from jsonargparse._formatters import get_env_var from jsonargparse._namespace import NSKeyError -from jsonargparse._optionals import jsonnet_support, jsonschema_support, pyyaml_available, ruamel_support +from jsonargparse._optionals import jsonnet_support, jsonschema_support, pyyaml_available from jsonargparse.typing import Path_fc, Path_fr, path_type from jsonargparse_tests.conftest import ( capture_logs, @@ -37,7 +37,6 @@ json_or_yaml_dump, json_or_yaml_load, responses_activate, - skip_if_docstring_parser_unavailable, skip_if_fsspec_unavailable, skip_if_no_pyyaml, skip_if_not_posix, @@ -635,24 +634,6 @@ def test_dump_order(parser, subtests): assert dump == "\n".join(v + ": " + str(n) for n, v in args.items()) + "\n" -def test_dump_comments_not_supported(parser): - parser.parser_mode = "json" - parser.add_argument("--op", type=int, default=1) - cfg = parser.get_defaults() - with pytest.raises(ValueError, match="Dumping with comments is not supported for format 'json'"): - parser.dump(cfg, with_comments=True) - - -@skip_if_no_pyyaml -def test_dump_comments_missing_ruamel(parser): - parser.add_argument("--op", type=int, default=1) - cfg = parser.get_defaults() - with patch.dict("jsonargparse._loaders_dumpers.dumpers") as dumpers: - dumpers.pop("yaml_comments", None) - with pytest.raises(ValueError, match="ruamel.yaml is required for dumping YAML with comments"): - parser.dump(cfg, with_comments=True) - - @pytest.fixture def parser_schema_jsonnet(parser, example_parser): parser.add_argument("--cfg", action="config") @@ -884,18 +865,6 @@ def test_save_fsspec(example_parser): ctx.match("multifile=True not supported") -@pytest.fixture -def print_parser(parser, subparser): - parser.description = "cli tool" - parser.add_argument("--cfg", action="config") - parser.add_argument("--v0", help=SUPPRESS, default="0") - parser.add_argument("--v1", help="Option v1.", default=1) - parser.add_argument("--g1.v2", help="Option v2.", default="2") - subparser.add_argument("--v3") - parser.add_argument("--g2", action=ActionParser(parser=subparser)) - return parser - - def test_print_config_normal(print_parser): out = get_parse_args_stdout(print_parser, ["--print_config"]) assert json_or_yaml_load(out) == {"g1": {"v2": "2"}, "g2": {"v3": None}, "v1": 1} @@ -906,25 +875,6 @@ def test_print_config_skip_unset(print_parser): assert json_or_yaml_load(out) == {"g1": {"v2": "2"}, "g2": {}, "v1": 1} -@pytest.mark.skipif(not ruamel_support, reason="ruamel.yaml package is required") -@skip_if_docstring_parser_unavailable -def test_print_config_comments(print_parser): - help_str = get_parser_help(print_parser) - assert "comments," in help_str - out = get_parse_args_stdout(print_parser, ["--print_config=comments"]) - assert "# cli tool" in out - assert "# Option v1. (default: 1)" in out - assert "# Option v2. (default: 2)" in out - - -@pytest.mark.skipif(ruamel_support, reason="ruamel.yaml package should not be installed") -def test_print_config_comments_unavailable(print_parser): - help_str = get_parser_help(print_parser) - assert "comments," not in help_str - with pytest.raises(ArgumentError, match='Invalid option "comments"'): - get_parse_args_stdout(print_parser, ["--print_config=comments"]) - - def test_print_config_invalid_flag(print_parser): with pytest.raises(ArgumentError) as ctx: print_parser.parse_args(["--print_config=invalid"]) diff --git a/jsonargparse_tests/test_optionals.py b/jsonargparse_tests/test_optionals.py index afe0126e..af5071eb 100644 --- a/jsonargparse_tests/test_optionals.py +++ b/jsonargparse_tests/test_optionals.py @@ -4,7 +4,7 @@ import pytest -from jsonargparse import set_parsing_settings +from jsonargparse import ArgumentError, set_parsing_settings from jsonargparse._optionals import ( _get_config_read_mode, docstring_parser_support, @@ -24,6 +24,8 @@ ) from jsonargparse.typing import is_final_class from jsonargparse_tests.conftest import ( + get_parse_args_stdout, + get_parser_help, skip_if_docstring_parser_unavailable, skip_if_fsspec_unavailable, skip_if_requests_unavailable, @@ -141,6 +143,14 @@ def test_ruamel_support_false(): ctx.match("test_ruamel_support_false") +@pytest.mark.skipif(ruamel_support, reason="ruamel.yaml package should not be installed") +def test_print_config_comments_unavailable(print_parser): + help_str = get_parser_help(print_parser) + assert "comments," not in help_str + with pytest.raises(ArgumentError, match='Invalid option "comments"'): + get_parse_args_stdout(print_parser, ["--print_config=comments"]) + + # config read mode tests diff --git a/jsonargparse_tests/test_yaml_comments.py b/jsonargparse_tests/test_yaml_comments.py new file mode 100644 index 00000000..05b5e0f3 --- /dev/null +++ b/jsonargparse_tests/test_yaml_comments.py @@ -0,0 +1,542 @@ +from __future__ import annotations + +from dataclasses import dataclass, field +from textwrap import dedent, indent +from typing import Any, Callable, Dict, List, Optional, Union +from unittest.mock import patch + +import pytest + +from jsonargparse import ArgumentParser +from jsonargparse_tests.conftest import ( + get_parse_args_stdout, + get_parser_help, + json_or_yaml_load, + skip_if_yaml_comments_unavailable, +) + +pytestmark = skip_if_yaml_comments_unavailable + + +def test_dump_comments_not_supported(parser): + parser.parser_mode = "json" + parser.add_argument("--op", type=int, default=1) + cfg = parser.get_defaults() + with pytest.raises(ValueError, match="Dumping with comments is not supported for format 'json'"): + parser.dump(cfg, with_comments=True) + + +def test_dump_comments_missing_ruamel(parser): + parser.add_argument("--op", type=int, default=1) + cfg = parser.get_defaults() + with patch.dict("jsonargparse._loaders_dumpers.dumpers") as dumpers: + dumpers.pop("yaml_comments", None) + with pytest.raises(ValueError, match="ruamel.yaml is required for dumping YAML with comments"): + parser.dump(cfg, with_comments=True) + + +def test_print_config_comments(print_parser): + help_str = get_parser_help(print_parser) + assert "comments," in help_str + out = get_parse_args_stdout(print_parser, ["--print_config=comments"]) + assert "# cli tool" in out + assert "# Option v1. (default: 1)" in out + assert "# Option v2. (default: 2)" in out + + +def get_dump(parser: ArgumentParser, args: list) -> str: + cfg = parser.parse_args(args) + dump = parser.dump(cfg, with_comments=True) + assert json_or_yaml_load(dump) == json_or_yaml_load(parser.dump(cfg)) + return dump + + +def block(text: str, depth: int = 0) -> str: + """Dedents an expected dump block and indents it to the given nested level.""" + return indent(dedent(text), " " * depth) + + +class Optimizer: + def __init__(self, lr: float = 0.1): + """Base optimizer. + + Args: + lr: Learning rate. + """ + self.lr = lr # pragma: no cover + + +class SGD(Optimizer): + def __init__(self, momentum: float = 0.9, **kwargs): + """Stochastic gradient descent. + + Args: + momentum: Momentum factor. + """ + super().__init__(**kwargs) # pragma: no cover + + +class NoParams(Optimizer): + """Optimizer without parameters.""" + + def __init__(self): + pass # pragma: no cover + + +class NoDescription(Optimizer): + def __init__(self, gamma: float = 0.1, **kwargs): + """ + Args: + gamma: Decay factor. + """ + super().__init__(**kwargs) # pragma: no cover + + +@dataclass +class Data: + """Data settings. + + Args: + path: Path to the data. + batch_size: Number of samples per batch. + """ + + path: str = "data" + batch_size: int = 4 + + +@dataclass +class Nested: + """Nested settings. + + Args: + data: The data settings. + seed: Random seed. + """ + + data: Data = field(default_factory=Data) + seed: int = 1 + + +class Model: + def __init__(self, optimizer: Optimizer = SGD(), name: str = "model"): + """A model. + + Args: + optimizer: The optimizer to use. + name: Name of the model. + """ + + +class Trainer: + def __init__(self, data: Data = Data(), epochs: int = 1): + """A trainer. + + Args: + data: The data settings. + epochs: Number of epochs. + """ + + +# subclass tests + + +def test_subclass_init_args(parser): + parser.add_argument("--optimizer", type=Optimizer, help="The optimizer.") + dump = get_dump(parser, [f"--optimizer={__name__}.SGD"]) + expected = block( + f""" + optimizer: + class_path: {__name__}.SGD + + # Stochastic gradient descent + init_args: + + # Momentum factor. (type: float, default: 0.9) + momentum: 0.9 + + # Learning rate. (type: float, default: 0.1) + lr: 0.1 + """ + ) + assert expected in dump + assert "# The optimizer. (type: " in dump + + +def test_subclass_without_init_args(parser): + parser.add_argument("--optimizer", type=Optimizer, help="The optimizer.") + dump = get_dump(parser, [f"--optimizer={__name__}.NoParams"]) + assert "init_args" not in dump + assert "class_path:" in dump + + +def test_subclass_nested_in_init_args(parser): + parser.add_argument("--model", type=Model, help="The model.") + dump = get_dump(parser, [f"--model={__name__}.Model"]) + expected = block( + f""" + model: + class_path: {__name__}.Model + + # A model + init_args: + """ + ) + assert expected in dump + expected = block( + f""" + optimizer: + class_path: {__name__}.SGD + + # Stochastic gradient descent + init_args: + + # Momentum factor. (type: float, default: 0.9) + momentum: 0.9 + + # Learning rate. (type: float, default: 0.1) + lr: 0.1 + + # Name of the model. (type: str, default: model) + name: model + """, + depth=2, + ) + assert expected in dump + + +def test_subclass_in_list(parser): + parser.add_argument("--optimizers", type=List[Optimizer], help="The optimizers.") + dump = get_dump(parser, [f"--optimizers=[{__name__}.SGD,{__name__}.Optimizer]"]) + assert "# The optimizers. (type: " in dump + expected = block( + f""" + optimizers: + - class_path: {__name__}.SGD + + # Stochastic gradient descent + init_args: + + # Momentum factor. (type: float, default: 0.9) + momentum: 0.9 + + # Learning rate. (type: float, default: 0.1) + lr: 0.1 + - class_path: {__name__}.Optimizer + + # Base optimizer + init_args: + + # Learning rate. (type: float, default: 0.1) + lr: 0.1 + """ + ) + assert expected in dump + + +def test_subclass_in_nested_list(parser): + parser.add_argument("--optimizers", type=List[List[Optimizer]], help="The optimizers.") + dump = get_dump(parser, [f'--optimizers=[[{{"class_path": "{__name__}.SGD", "init_args": {{"momentum": 0.5}}}}]]']) + expected = block( + f""" + optimizers: + - - class_path: {__name__}.SGD + + # Stochastic gradient descent + init_args: + + # Momentum factor. (type: float, default: 0.9) + momentum: 0.5 + """ + ) + assert expected in dump + + +def test_subclass_in_dict(parser): + parser.add_argument("--optimizers", type=Dict[str, Optimizer], help="The optimizers.") + dump = get_dump(parser, [f'--optimizers={{"one": "{__name__}.SGD"}}']) + expected = block( + f""" + optimizers: + one: + class_path: {__name__}.SGD + + # Stochastic gradient descent + init_args: + + # Momentum factor. (type: float, default: 0.9) + momentum: 0.9 + + # Learning rate. (type: float, default: 0.1) + lr: 0.1 + """ + ) + assert expected in dump + + +def test_subclass_in_dict_of_lists(parser): + parser.add_argument("--optimizers", type=Dict[str, List[Optimizer]], help="The optimizers.") + spec = f'{{"class_path": "{__name__}.SGD", "init_args": {{"momentum": 0.5}}}}' + dump = get_dump(parser, [f'--optimizers={{"one": [{spec}]}}']) + expected = block( + f""" + optimizers: + one: + - class_path: {__name__}.SGD + + # Stochastic gradient descent + init_args: + + # Momentum factor. (type: float, default: 0.9) + momentum: 0.5 + + # Learning rate. (type: float, default: 0.1) + lr: 0.1 + """ + ) + assert expected in dump + + +def test_subclass_without_class_description(parser): + parser.add_argument("--optimizer", type=Optimizer, help="The optimizer.") + dump = get_dump(parser, [f"--optimizer={__name__}.NoDescription"]) + expected = block( + f""" + optimizer: + class_path: {__name__}.NoDescription + + # + init_args: + + # Decay factor. (type: float, default: 0.1) + gamma: 0.1 + + # Learning rate. (type: float, default: 0.1) + lr: 0.1 + """ + ) + assert expected in dump + + +def test_subclass_in_union(parser): + parser.add_argument("--optimizer", type=Union[int, Optimizer], help="The optimizer.") + dump = get_dump(parser, [f"--optimizer={__name__}.SGD"]) + assert "# Stochastic gradient descent\n init_args:" in dump + assert "# Momentum factor. (type: float, default: 0.9)" in dump + + +def test_subclass_in_any(parser): + parser.add_argument("--optimizer", type=Any, help="The optimizer.") + dump = get_dump(parser, [f'--optimizer={{"class_path": "{__name__}.SGD"}}']) + assert "# Stochastic gradient descent\n init_args:" in dump + assert "# Momentum factor. (type: float, default: 0.9)" in dump + + +def test_add_subclass_arguments(parser): + parser.add_subclass_arguments(Optimizer, "optimizer", help="The optimizer.") + dump = get_dump(parser, [f"--optimizer={__name__}.SGD"]) + assert "# The optimizer. (type: " in dump + assert "# Stochastic gradient descent\n init_args:" in dump + assert "# Momentum factor. (type: float, default: 0.9)" in dump + + +def test_callable_return_type(parser): + parser.add_argument("--optimizer", type=Callable[[float], Optimizer], help="The optimizer.") + dump = get_dump(parser, [f"--optimizer={__name__}.SGD"]) + expected = block( + f""" + optimizer: + class_path: {__name__}.SGD + + # Stochastic gradient descent + init_args: + + # Learning rate. (type: float, default: 0.1) + lr: 0.1 + """ + ) + assert expected in dump + assert "momentum" not in dump + + +def test_subclass_skipped_init_args(parser): + parser.add_subclass_arguments(Optimizer, "optimizer", skip={"lr"}, help="The optimizer.") + dump = get_dump(parser, [f"--optimizer={__name__}.SGD"]) + assert "# Momentum factor. (type: float, default: 0.9)" in dump + assert "lr" not in dump + + +def test_unresolvable_class_path(parser): + parser.add_argument("--data", type=dict, help="Some dict.") + dump = get_dump(parser, ['--data={"class_path": "not.a.real.Class", "init_args": {"x": 1}}']) + assert "# Some dict. (type: " in dump + expected = block( + """ + data: + class_path: not.a.real.Class + init_args: + x: 1 + """ + ) + assert expected in dump + + +# dataclass-like tests + + +def test_dataclass_as_type(parser): + parser.add_argument("--data", type=Data, help="The data.") + dump = get_dump(parser, []) + expected = block( + """ + # The data + data: + + # Path to the data. (type: str, default: data) + path: data + + # Number of samples per batch. (type: int, default: 4) + batch_size: 4 + """ + ) + assert expected in dump + + +def test_dataclass_in_optional(parser): + parser.add_argument("--data", type=Optional[Data], help="The data.") + dump = get_dump(parser, ["--data={}"]) + expected = block( + """ + data: + + # Path to the data. (type: str, default: data) + path: data + + # Number of samples per batch. (type: int, default: 4) + batch_size: 4 + """ + ) + assert expected in dump + assert "# The data. (type: " in dump + + +def test_dataclass_in_list(parser): + parser.add_argument("--data", type=List[Data], help="The data.") + dump = get_dump(parser, ["--data=[{}]"]) + expected = block( + """ + data: + - + # Path to the data. (type: str, default: data) + path: data + + # Number of samples per batch. (type: int, default: 4) + batch_size: 4 + """ + ) + assert expected in dump + + +def test_dataclass_in_dict(parser): + parser.add_argument("--data", type=Dict[str, Data], help="The data.") + dump = get_dump(parser, ['--data={"one": {}}']) + expected = block( + """ + data: + one: + + # Path to the data. (type: str, default: data) + path: data + + # Number of samples per batch. (type: int, default: 4) + batch_size: 4 + """ + ) + assert expected in dump + + +def test_dataclass_nested_in_dataclass(parser): + parser.add_argument("--nested", type=Optional[Nested], help="The nested.") + dump = get_dump(parser, ["--nested={}"]) + expected = block( + """ + nested: + + # The data settings + data: + + # Path to the data. (type: str, default: data) + path: data + + # Number of samples per batch. (type: int, default: 4) + batch_size: 4 + + # Random seed. (type: int, default: 1) + seed: 1 + """ + ) + assert expected in dump + + +def test_dataclass_in_subclass_init_args(parser): + parser.add_argument("--trainer", type=Trainer, help="The trainer.") + dump = get_dump(parser, [f"--trainer={__name__}.Trainer"]) + expected = block( + """ + # A trainer + init_args: + + # The data settings + data: + + # Path to the data. (type: str, default: data) + path: data + + # Number of samples per batch. (type: int, default: 4) + batch_size: 4 + + # Number of epochs. (type: int, default: 1) + epochs: 1 + """, + depth=1, + ) + assert expected in dump + + +# subcommand tests + + +def test_subcommand_subclass(parser, subparser): + parser.description = "the tool" + subparser.description = "fit command" + subparser.add_argument("--optimizer", type=Optimizer, help="The optimizer.") + subcommands = parser.add_subcommands() + subcommands.add_subcommand("fit", subparser) + dump = get_dump(parser, ["fit", f"--optimizer={__name__}.SGD"]) + expected = block( + f""" + optimizer: + class_path: {__name__}.SGD + + # Stochastic gradient descent + init_args: + + # Momentum factor. (type: float, default: 0.9) + momentum: 0.9 + + # Learning rate. (type: float, default: 0.1) + lr: 0.1 + """, + depth=1, + ) + assert dump.startswith("# the tool\n") + assert "\n# fit command\nfit:\n" in dump + assert expected in dump + + +def test_print_config_comments_subclass(parser): + parser.add_argument("--cfg", action="config") + parser.add_argument("--optimizer", type=Optimizer, help="The optimizer.") + out = get_parse_args_stdout(parser, [f"--optimizer={__name__}.SGD", "--print_config=comments"]) + assert "# Stochastic gradient descent\n init_args:" in out + assert "# Momentum factor. (type: float, default: 0.9)" in out diff --git a/pyproject.toml b/pyproject.toml index 3e95605a..af576dfe 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -224,7 +224,8 @@ commands = [testenv:pydantic-v1] extras = coverage -deps = ./jsonargparse_tests +# editable, so that changes in the tests source are picked up without recreating the env +deps = -e ./jsonargparse_tests commands = # Test with pydantic<2 python -c "\ @@ -256,7 +257,8 @@ commands = [testenv:without-future-annotations] extras = test,coverage,all -deps = ./jsonargparse_tests +# editable, so that changes in the tests source are picked up without recreating the env +deps = -e ./jsonargparse_tests allowlist_externals = sh commands = sh -c "\ From 51e3bc392eb3a87ae7e9917cccc9e32ba60ed3c6 Mon Sep 17 00:00:00 2001 From: Mauricio Villegas <5780272+mauvilsa@users.noreply.github.com> Date: Thu, 30 Jul 2026 22:00:49 +0200 Subject: [PATCH 2/4] Add support for subconfigs in list and dict items (#940) --- CHANGELOG.rst | 4 + DOCUMENTATION.rst | 109 ++++++++++++++++ jsonargparse/_signatures.py | 6 +- jsonargparse/_typehints.py | 70 +++++++++- jsonargparse_tests/test_paths.py | 215 ++++++++++++++++++++++++++++++- 5 files changed, 398 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 45322d68..65ca054e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -22,6 +22,10 @@ Added not a valid one, the parsing fails instead of silently ignoring it. When disabled (the default), a debug log now informs about the ignored invalid subclass spec (`#938 `__). +- Items of a list of classes and values of a dict of classes can now be given as + paths to sub-config files, instead of this only being supported for the value + of an entire argument (`#940 + `__). Fixed ^^^^^ diff --git a/DOCUMENTATION.rst b/DOCUMENTATION.rst index 83aaba4e..c27a0b72 100644 --- a/DOCUMENTATION.rst +++ b/DOCUMENTATION.rst @@ -2102,6 +2102,115 @@ be accepted. In this case the config would be like: type a class. The accepted ``init_args`` would be the parameters of that function. + +.. _sub-config-files: + +Sub-config files +---------------- + +Instead of writing a subclass spec inline, a path to a config file that holds it +can be given. This makes it possible to split a large config into smaller +reusable files. It requires that the argument was added with +``sub_configs=True``, which is the default in :func:`.auto_cli` and is accepted +by :meth:`add_argument <.ArgumentParser.add_argument>` and the +``add_*_arguments`` methods. + +This also works for the items of a list of classes and for the values of a dict +of classes, which is useful when each component is defined in its own config +file. For example, take the following classes: + +.. testcode:: sub_config_files + + class Hook: + def __init__(self, verbose: bool = False): + self.verbose = verbose + + + class LogHook(Hook): + def __init__(self, log_file: str = "run.log", **kwargs): + super().__init__(**kwargs) + self.log_file = log_file + + + class CheckpointHook(Hook): + def __init__(self, every_n_steps: int = 100, **kwargs): + super().__init__(**kwargs) + self.every_n_steps = every_n_steps + +.. testcode:: sub_config_files + :hide: + + doctest_mock_class_in_main(LogHook) + doctest_mock_class_in_main(CheckpointHook) + +And a config in which each hook is a separate file: + +.. code-block:: yaml + + # File: hooks.yaml + hooks: + - log_hook.yaml + - checkpoint_hook.yaml + +.. code-block:: yaml + + # File: log_hook.yaml + class_path: LogHook + init_args: + log_file: train.log + +.. code-block:: yaml + + # File: checkpoint_hook.yaml + class_path: CheckpointHook + init_args: + every_n_steps: 500 + +.. testsetup:: sub_config_files + + cwd = os.getcwd() + tmpdir = tempfile.mkdtemp(prefix="_jsonargparse_doctest_") + os.chdir(tmpdir) + pathlib.Path("hooks.yaml").write_text("hooks:\n- log_hook.yaml\n- checkpoint_hook.yaml\n") + pathlib.Path("log_hook.yaml").write_text("class_path: LogHook\ninit_args:\n log_file: train.log\n") + pathlib.Path("checkpoint_hook.yaml").write_text("class_path: CheckpointHook\ninit_args:\n every_n_steps: 500\n") + +.. testcleanup:: sub_config_files + + os.chdir(cwd) + shutil.rmtree(tmpdir) + +Then in Python: + +.. doctest:: sub_config_files + + >>> parser = ArgumentParser() + >>> parser.add_argument("--hooks", type=list[Hook], sub_configs=True) # doctest: +IGNORE_RESULT + + >>> cfg = parser.parse_path("hooks.yaml") + >>> cfg.hooks[0].class_path + '__main__.LogHook' + >>> cfg.hooks[0].init_args.log_file + 'train.log' + >>> cfg.hooks[1].init_args.every_n_steps + 500 + + >>> init = parser.instantiate(cfg) + >>> isinstance(init.hooks[1], CheckpointHook) + True + +The same is accepted from command line, i.e. ``--hooks=[log_hook.yaml, +checkpoint_hook.yaml]``, or appending one item at a time as explained in +:ref:`list-append`, i.e. ``--hooks+=log_hook.yaml +--hooks+=checkpoint_hook.yaml``. + +Relative paths inside a sub-config file are resolved with respect to the +directory of that sub-config file, such that a group of config files can be +moved around without needing to modify them. Furthermore, :meth:`save +<.ArgumentParser.save>` with ``multifile=True`` writes back each sub-config to +its own file, preserving the original structure. + + .. _instance-factories: Instance factories diff --git a/jsonargparse/_signatures.py b/jsonargparse/_signatures.py index a3b845c9..4460b8b7 100644 --- a/jsonargparse/_signatures.py +++ b/jsonargparse/_signatures.py @@ -30,6 +30,7 @@ get_subclass_names, is_list_pathlike, is_optional, + is_subclass_container_typehint, not_required_types, replace_unresolved_forward_refs, sequence_origin_types, @@ -450,7 +451,10 @@ def _add_signature_parameter( else: register_pydantic_type(annotation) enable_path = sub_configs and ( - is_subclass_typehint or is_return_subclass_typehint or is_list_pathlike(annotation) + is_subclass_typehint + or is_return_subclass_typehint + or is_list_pathlike(annotation) + or is_subclass_container_typehint(annotation) ) args = ActionTypeHint.prepare_add_argument( args=args, diff --git a/jsonargparse/_typehints.py b/jsonargparse/_typehints.py index 785f2e9f..0d38bd89 100644 --- a/jsonargparse/_typehints.py +++ b/jsonargparse/_typehints.py @@ -645,6 +645,8 @@ def _check_type(self, value, append=False, cfg=None, mode=None): except get_loader_exceptions(): config_path = None path_meta = val.pop("__path__", None) if isinstance(val, dict) else None + # a single sub-config appended to a list becomes one more list item + appended_subconfig = append and config_path is not None and not isinstance(val, list) unset_sentinel = get_parsing_setting("unset_sentinel") prev_val = cfg.get(self.dest) if cfg else unset_sentinel @@ -684,10 +686,13 @@ def _check_type(self, value, append=False, cfg=None, mode=None): if ex: raise ex - if path_meta is not None: - val["__path__"] = path_meta - if isinstance(val, (Namespace, dict)) and config_path is not None: - val["__path__"] = config_path + if isinstance(val, (Namespace, dict)): + if path_meta is not None: + val["__path__"] = path_meta + if config_path is not None: + val["__path__"] = config_path + elif appended_subconfig and isinstance(val, list) and isinstance(val[-1], (Namespace, dict)): + val[-1]["__path__"] = config_path value[num] = val except (TypeError, ValueError) as ex: if self._is_valid_string(val): @@ -787,6 +792,56 @@ def is_list_pathlike(typehint) -> bool: return False +def is_subclass_container_typehint(typehint) -> bool: + """Whether a container type, e.g. list or dict, has classes as items.""" + typehint = get_unaliased_type(typehint) + subtypehints = getattr(typehint, "__args__", None) + if not subtypehints: + return False + typehint_origin = get_typehint_origin(typehint) + if typehint_origin == Union: + return any(is_subclass_container_typehint(s) for s in subtypehints) + if typehint_origin in sequence_or_mapping_origin_types: + return any( + ActionTypeHint.is_subclass_typehint(s, all_subtypes=False) + or ActionTypeHint.is_return_subclass_typehint(s) + or is_subclass_container_typehint(s) + for s in subtypehints + ) + return False + + +# sentinel returned by adapt_subconfig_path when the value is not a path to a config file +not_a_subconfig_path = object() + + +def adapt_subconfig_path(val, typehint, adapt_kwargs): + """Loads and adapts a sub-config when val is a path to a config file. + + Only relevant for types that expect a class, since for these a string is + otherwise interpreted as a class path. Makes it possible for items in a + list or dict of classes to be given as paths to sub-config files. + """ + if not adapt_kwargs.get("enable_path") or not isinstance(val, str): + return not_a_subconfig_path + from ._optionals import _get_config_read_mode + + try: + path = Path(val, mode=_get_config_read_mode()) + except TypeError: + return not_a_subconfig_path + try: + with load_config_path_context(path), path.relative_path_context(): + subconfig = load_value(path.read_text()) + except get_loader_exceptions() as ex: + raise_unexpected_value(f"Invalid content in sub-config file {val}: {ex}", exception=ex) + with load_config_path_context(path), change_to_path_dir(path): + val = adapt_typehints(subconfig, typehint, **adapt_kwargs) + if isinstance(val, (Namespace, dict)): + val["__path__"] = path + return val + + def raise_unexpected_value(message: str, val: Any = inspect._empty, exception: Exception | None = None) -> NoReturn: if val is not inspect._empty: message += f". Got value: {val}" @@ -1147,6 +1202,9 @@ def adapt_typehints( else: val = object_path_serializer(val) else: + adapted = adapt_subconfig_path(val, typehint, adapt_kwargs) + if adapted is not not_a_subconfig_path: + return adapted try: val_input = val if isinstance(val, str): @@ -1201,6 +1259,10 @@ def adapt_typehints( if serialize and isinstance(val, str): return val + adapted = adapt_subconfig_path(val, typehint, adapt_kwargs) + if adapted is not not_a_subconfig_path: + return adapted + prev_implicit_defaults = False if prev_val is unset_sentinel and not inspect.isabstract(typehint) and not is_protocol(typehint): with suppress(ValueError): diff --git a/jsonargparse_tests/test_paths.py b/jsonargparse_tests/test_paths.py index b2997c69..414b8138 100644 --- a/jsonargparse_tests/test_paths.py +++ b/jsonargparse_tests/test_paths.py @@ -6,7 +6,7 @@ import stat import zipfile from io import StringIO -from typing import Any, Dict, List, Optional, Union +from typing import Any, Callable, Dict, List, Optional, Union from unittest.mock import patch import pytest @@ -578,6 +578,219 @@ def test_sub_configs_subclass(parser, tmp_cwd): assert isinstance(init["cls"], Base) +# sub_configs for items in a list of subclasses tests + + +class ItemBase: + def __init__(self, x: int = 1): + self.x = x + + +class ItemSub(ItemBase): + def __init__(self, y: str = "-", **kwargs): + super().__init__(**kwargs) + self.y = y + + +class ItemNested(ItemBase): + def __init__(self, sub: Optional[ItemBase] = None, **kwargs): + super().__init__(**kwargs) + self.sub = sub + + +class ItemPath(ItemBase): + def __init__(self, file: Optional[Path_fr] = None, **kwargs): + super().__init__(**kwargs) + self.file = file + + +class ItemsMain: + def __init__(self, objects: List[ItemBase] = []): + self.objects = objects + + +class ItemsDictMain: + def __init__(self, objects: Optional[Dict[str, ItemBase]] = None): + self.objects = objects + + +item1_spec = {"class_path": f"{__name__}.ItemSub", "init_args": {"x": 2, "y": "a"}} +item2_spec = {"class_path": f"{__name__}.ItemBase", "init_args": {"x": 3}} + + +@pytest.fixture +def item_subconfigs(tmp_cwd): + pathlib.Path("item1.yaml").write_text(json_or_yaml_dump(item1_spec)) + pathlib.Path("item2.yaml").write_text(json_or_yaml_dump(item2_spec)) + return tmp_cwd + + +def assert_items(items): + assert len(items) == 2 + assert items[0].class_path == f"{__name__}.ItemSub" + assert items[0].init_args == Namespace(x=2, y="a") + assert items[1].class_path == f"{__name__}.ItemBase" + assert items[1].init_args == Namespace(x=3) + assert [str(item["__path__"]) for item in items] == ["item1.yaml", "item2.yaml"] + + +def test_sub_configs_list_subclass_in_config(parser, item_subconfigs): + pathlib.Path("config.yaml").write_text(json_or_yaml_dump({"objects": ["item1.yaml", "item2.yaml"]})) + + parser.add_argument("--cfg", action="config") + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + cfg = parser.parse_args(["--cfg=config.yaml"]) + assert_items(cfg.objects) + init = parser.instantiate(cfg) + assert isinstance(init.objects[0], ItemSub) + assert isinstance(init.objects[1], ItemBase) + assert (init.objects[0].x, init.objects[0].y) == (2, "a") + assert init.objects[1].x == 3 + + +def test_sub_configs_list_subclass_command_line(parser, item_subconfigs): + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + cfg = parser.parse_args(['--objects=["item1.yaml", "item2.yaml"]']) + assert_items(cfg.objects) + + +def test_sub_configs_list_subclass_append(parser, item_subconfigs): + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + cfg = parser.parse_args(["--objects+=item1.yaml", "--objects+=item2.yaml"]) + assert_items(cfg.objects) + + +def test_sub_configs_list_subclass_mixed_with_specs(parser, item_subconfigs): + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + cfg = parser.parse_args([f'--objects=["item1.yaml", {json.dumps(item2_spec)}]']) + assert len(cfg.objects) == 2 + assert str(cfg.objects[0]["__path__"]) == "item1.yaml" + assert "__path__" not in cfg.objects[1] + assert cfg.objects[1].init_args == Namespace(x=3) + + +def test_sub_configs_list_subclass_from_signature(parser, item_subconfigs): + parser.add_class_arguments(ItemsMain, "main", sub_configs=True) + + cfg = parser.parse_args(['--main.objects=["item1.yaml", "item2.yaml"]']) + assert_items(cfg.main.objects) + + +def test_sub_configs_list_subclass_paths_relative_to_subconfig(parser, tmp_cwd): + subdir = tmp_cwd / "subdir" + subdir.mkdir() + (subdir / "data.txt").touch() + item = {"class_path": f"{__name__}.ItemPath", "init_args": {"file": "data.txt"}} + (subdir / "item.yaml").write_text(json_or_yaml_dump(item)) + + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + cfg = parser.parse_args(['--objects=["subdir/item.yaml"]']) + assert str(cfg.objects[0]["__path__"]) == "subdir/item.yaml" + assert str(cfg.objects[0].init_args.file) == "data.txt" + init = parser.instantiate(cfg) + assert init.objects[0].file.absolute == str(subdir / "data.txt") + + +def test_sub_configs_list_subclass_nested_subconfig(parser, tmp_cwd): + subdir = tmp_cwd / "subdir" + subdir.mkdir() + outer = {"class_path": f"{__name__}.ItemNested", "init_args": {"sub": "inner.yaml"}} + inner = {"class_path": f"{__name__}.ItemSub", "init_args": {"y": "n"}} + (subdir / "outer.yaml").write_text(json_or_yaml_dump(outer)) + (subdir / "inner.yaml").write_text(json_or_yaml_dump(inner)) + + parser.add_class_arguments(ItemsMain, "main", sub_configs=True) + + cfg = parser.parse_args(['--main.objects=["subdir/outer.yaml"]']) + assert str(cfg.main.objects[0]["__path__"]) == "subdir/outer.yaml" + assert cfg.main.objects[0].class_path == f"{__name__}.ItemNested" + assert cfg.main.objects[0].init_args.sub.class_path == f"{__name__}.ItemSub" + assert cfg.main.objects[0].init_args.sub.init_args.y == "n" + init = parser.instantiate(cfg) + assert isinstance(init.main.objects[0].sub, ItemSub) + + +def test_sub_configs_list_subclass_loop_detected(parser, tmp_cwd): + pathlib.Path("objects.yaml").write_text(json_or_yaml_dump(["objects.yaml"])) + + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(["--objects=objects.yaml"]) + ctx.match("Config file loop detected") + + +def test_sub_configs_dict_subclass_values(parser, item_subconfigs): + parser.add_argument("--objects", type=Dict[str, ItemBase], sub_configs=True) + + cfg = parser.parse_args(['--objects={"a": "item1.yaml", "b": "item2.yaml"}']) + assert_items([cfg.objects["a"], cfg.objects["b"]]) + + +def test_sub_configs_dict_subclass_values_from_signature(parser, item_subconfigs): + parser.add_class_arguments(ItemsDictMain, "main", sub_configs=True) + + cfg = parser.parse_args(['--main.objects={"a": "item1.yaml", "b": "item2.yaml"}']) + assert_items([cfg.main.objects["a"], cfg.main.objects["b"]]) + + +def test_sub_configs_list_subclass_path_not_exist(parser, item_subconfigs): + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(['--objects=["does-not-exist.yaml"]']) + ctx.match("Unexpected import path format: does-not-exist.yaml") + + +def test_sub_configs_list_subclass_invalid_class_path(parser, tmp_cwd): + pathlib.Path("item.yaml").write_text('{"class_path": "not.a.class"}') + + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(['--objects=["item.yaml"]']) + ctx.match("Problem with given class_path 'not.a.class'") + + +def test_sub_configs_list_subclass_unparsable_content(parser, tmp_cwd): + pathlib.Path("item.yaml").write_text("class_path: [not: valid") + + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(['--objects=["item.yaml"]']) + ctx.match("Invalid content in sub-config file item.yaml") + + +def test_sub_configs_list_callable_return_subclass(parser, item_subconfigs): + parser.add_argument("--objects", type=List[Callable[[], ItemBase]], sub_configs=True) + + cfg = parser.parse_args(['--objects=["item1.yaml", "item2.yaml"]']) + assert_items(cfg.objects) + + +def test_sub_configs_list_subclass_save_multifile(parser, item_subconfigs): + main = {"objects": ["item1.yaml", "item2.yaml"]} + pathlib.Path("config.yaml").write_text(json_or_yaml_dump(main)) + out_dir = item_subconfigs / "out" + out_dir.mkdir() + + parser.add_argument("--cfg", action="config") + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + cfg = parser.parse_args(["--cfg=config.yaml"]) + parser.save(cfg, out_dir / "config.yaml", multifile=True) + + assert json_or_yaml_load((out_dir / "config.yaml").read_text()) == main + assert json_or_yaml_load((out_dir / "item1.yaml").read_text()) == item1_spec + assert json_or_yaml_load((out_dir / "item2.yaml").read_text()) == item2_spec + + def test_sub_configs_list_path_fr(parser, tmp_cwd, mock_stdin, subtests): tmpdir = tmp_cwd / "subdir" tmpdir.mkdir() From d049b43e15c2130a10c2764fe84279db01810c6d Mon Sep 17 00:00:00 2001 From: Mauricio Villegas <5780272+mauvilsa@users.noreply.github.com> Date: Fri, 31 Jul 2026 10:30:28 +0200 Subject: [PATCH 3/4] Improve protocol implementation check to be based on signature compatibility (#941) --- CHANGELOG.rst | 9 + DOCUMENTATION.rst | 6 +- jsonargparse/_typehints.py | 141 ++++++++- jsonargparse_tests/test_subclasses.py | 406 ++++++++++++++++++++++++++ 4 files changed, 551 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 65ca054e..09bdad9f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -52,6 +52,15 @@ Changed longer shown in the help. Now they must agree with whether the argument is required, otherwise adding the argument fails (`#937 `__). +- Whether a class implements a ``Protocol`` is now decided by checking that its + methods can be called in all the ways that the protocol methods can be called, + similar to what static type checkers do, instead of requiring the parameter + lists to be identical. Among others, this means that names of positional-only + parameters are ignored, ``*args``/``**kwargs`` in the implementation can stand + in for protocol parameters, and extra optional parameters in the + implementation are accepted. Parameter and return types must still match + exactly, except when the protocol has no annotation or ``Any`` (`#941 + `__). v4.50.0 (2026-07-22) diff --git a/DOCUMENTATION.rst b/DOCUMENTATION.rst index c27a0b72..8d8d4b9b 100644 --- a/DOCUMENTATION.rst +++ b/DOCUMENTATION.rst @@ -554,7 +554,11 @@ Some notes about this support are: - ``Protocol`` types are also supported the same as subclasses. The protocols are not required to be ``runtime_checkable``. But the accepted classes must - match exactly the signature of the protocol's public methods. + implement all of the protocol's public methods with a compatible signature, + i.e. the methods must be callable in all the ways that the protocol's methods + can be called, similar to what static type checkers verify. Parameter and + return types must match exactly, subtypes are not accepted, except when the + protocol has no annotation or ``Any``, which accept any type. - ``dataclasses``, final classes, attrs' ``define``, pydantic's ``dataclass`` and pydantic's ``BaseModel`` are supported even when nested. By default they diff --git a/jsonargparse/_typehints.py b/jsonargparse/_typehints.py index 0d38bd89..72e7d103 100644 --- a/jsonargparse/_typehints.py +++ b/jsonargparse/_typehints.py @@ -56,6 +56,7 @@ lenient_check, nested_links, parent_parser, + parse_logger, parser_context, validating_defaults, ) @@ -1364,12 +1365,134 @@ def adapt_typehints( } -def implements_protocol(value, protocol) -> bool: - from jsonargparse._parameter_resolvers import get_signature_parameters - from jsonargparse._postponed_annotations import get_return_type +def get_protocol_method_signature(class_type, name, logger): + """Returns the parameters (excluding self) and return type of a method, with annotations resolved. + + In contrast to get_signature_parameters, the signature is taken as declared, i.e. ``*args`` and + ``**kwargs`` are not resolved into the parameters that they might accept, since for protocols + what matters is how the method can be called. + """ + from jsonargparse._parameter_resolvers import ParamData, parameter_attributes + from jsonargparse._postponed_annotations import evaluate_postponed_annotations, get_return_type + + method = inspect.getattr_static(class_type, name) + skip_self = not isinstance(method, staticmethod) + if isinstance(method, (staticmethod, classmethod)): + method = method.__func__ + if not inspect.isfunction(method): + raise ValueError(f"Expected {class_type.__name__}.{name} to be a function, but got {method}.") + + signature = inspect.signature(method) + params = [ParamData(**{a: getattr(p, a) for a in parameter_attributes}) for p in signature.parameters.values()] + evaluate_postponed_annotations(params, method, None, logger) + return (params[1:] if skip_self else params), get_return_type(method, logger) + + +def protocol_type_matches(proto_annotation, value_annotation, value_any_accepted: bool = False) -> bool: + """Whether a type in an implementation is accepted for the corresponding type in a protocol.""" + if proto_annotation is inspect.Parameter.empty or proto_annotation == Any: + return True + if value_any_accepted and (value_annotation is inspect.Parameter.empty or value_annotation == Any): + return True + return proto_annotation == value_annotation + + +def protocol_var_param_matches(proto_param, value_var_param) -> bool: + """Whether an implementation *args/**kwargs can stand in for a protocol parameter.""" + return value_var_param is not None and protocol_type_matches( + proto_param.annotation, value_var_param.annotation, value_any_accepted=True + ) + + +def split_signature_params(params): + kinds = inspect.Parameter + positional = [p for p in params if p.kind in (kinds.POSITIONAL_ONLY, kinds.POSITIONAL_OR_KEYWORD)] + keyword_only = {p.name: p for p in params if p.kind is kinds.KEYWORD_ONLY} + var_positional = next((p for p in params if p.kind is kinds.VAR_POSITIONAL), None) + var_keyword = next((p for p in params if p.kind is kinds.VAR_KEYWORD), None) + return positional, keyword_only, var_positional, var_keyword + + +def protocol_params_match(proto_params, value_params) -> bool: + """Whether a method can be called in all the ways that a protocol method can be called. + + Types are required to match exactly, except when the protocol has no annotation or ``Any``, in + which case any type in the implementation is accepted. + """ + proto_pos, proto_kw, proto_args, proto_kwargs = split_signature_params(proto_params) + value_pos, value_kw, value_args, value_kwargs = split_signature_params(value_params) + empty = inspect.Parameter.empty + + # arbitrary extra arguments accepted by the protocol must also be accepted by the implementation + if proto_args and not protocol_var_param_matches(proto_args, value_args): + return False + if proto_kwargs and not protocol_var_param_matches(proto_kwargs, value_kwargs): + return False + + matched: set = set() # indexes of value_pos already accounted for + + # parameters that the protocol accepts positionally + for num, proto_param in enumerate(proto_pos): + if num >= len(value_pos): + # only *args, or *args and **kwargs when also accepted by keyword, can stand in + if not protocol_var_param_matches(proto_param, value_args): + return False + if proto_param.kind is inspect.Parameter.POSITIONAL_OR_KEYWORD and not protocol_var_param_matches( + proto_param, value_kwargs + ): + return False + continue + value_param = value_pos[num] + if proto_param.kind is inspect.Parameter.POSITIONAL_OR_KEYWORD and ( + value_param.kind is not inspect.Parameter.POSITIONAL_OR_KEYWORD or value_param.name != proto_param.name + ): + return False # names only irrelevant when the protocol accepts the parameter positionally only + if not protocol_type_matches(proto_param.annotation, value_param.annotation): + return False + if proto_param.default is not empty and value_param.default is empty: + return False + matched.add(num) + + # parameters that the protocol only accepts by keyword + for name, proto_param in proto_kw.items(): + value_param = value_kw.get(name) + if value_param is None: + # a parameter accepted both positionally and by keyword also works + num = next( + ( + n + for n, p in enumerate(value_pos) + if p.name == name and n not in matched and p.kind is inspect.Parameter.POSITIONAL_OR_KEYWORD + ), + None, + ) + if num is None: + if not protocol_var_param_matches(proto_param, value_kwargs): + return False + continue + value_param = value_pos[num] + matched.add(num) + if not protocol_type_matches(proto_param.annotation, value_param.annotation): + return False + if proto_param.default is not empty and value_param.default is empty: + return False + + # parameters only in the implementation must be optional and accept what the protocol might give + for num, value_param in enumerate(value_pos): + if num in matched: + continue + if value_param.default is empty: + return False + if proto_args and not protocol_type_matches(proto_args.annotation, value_param.annotation): + return False + return all(p.default is not empty for n, p in value_kw.items() if n not in proto_kw) + +def implements_protocol(value, protocol) -> bool: if not inspect.isclass(value) or value is object or not is_protocol(protocol): return False + + logger = parse_logger(True, "implements_protocol") members = 0 for name, _ in inspect.getmembers(protocol, predicate=inspect.isfunction): is_dunder = name.startswith("__") and name.endswith("__") @@ -1379,15 +1502,13 @@ def implements_protocol(value, protocol) -> bool: return False members += 1 try: - value_params = get_signature_parameters(value, name) - except ValueError: + value_params, value_return = get_protocol_method_signature(value, name, logger) + except (ValueError, TypeError): return False - proto_params = get_signature_parameters(protocol, name) - if [(p.name, p.annotation) for p in proto_params] != [(p.name, p.annotation) for p in value_params]: + proto_params, proto_return = get_protocol_method_signature(protocol, name, logger) + if not protocol_params_match(proto_params, value_params): return False - proto_return = get_return_type(inspect.getattr_static(protocol, name)) - value_return = get_return_type(inspect.getattr_static(value, name)) - if proto_return != value_return: + if not protocol_type_matches(proto_return, value_return): return False return True if members else False diff --git a/jsonargparse_tests/test_subclasses.py b/jsonargparse_tests/test_subclasses.py index d668e7cc..8ff6f508 100644 --- a/jsonargparse_tests/test_subclasses.py +++ b/jsonargparse_tests/test_subclasses.py @@ -1529,6 +1529,412 @@ def test_implements_protocol(expected, value): assert implements_protocol(value, Interface) is expected +# protocol method signature matching tests + + +class PositionalOnlyInterface(Protocol): + def run(self, a: int, b: str, /) -> None: ... + + +class PositionalOnlyRenamed: + def run(self, x: int, y: str, /) -> None: ... + + +class PositionalOnlyAsPositionalOrKeyword: + def run(self, x: int, y: str) -> None: ... + + +class PositionalOnlyMissingOne: + def run(self, a: int, /) -> None: ... + + +class PositionalOnlySwappedTypes: + def run(self, a: str, b: int, /) -> None: ... + + +class PositionalOnlyExtraRequired: + def run(self, a: int, b: str, c: float, /) -> None: ... + + +class PositionalOnlyExtraOptional: + def run(self, a: int, b: str, c: float = 0.0, /) -> None: ... + + +class PositionalOnlyAsKeywordOnly: + def run(self, *, a: int, b: str) -> None: ... + + +class PositionalOnlyAsVarPositional: + def run(self, *args) -> None: ... + + +class PositionalOnlyAsVarKeyword: + def run(self, **kwargs) -> None: ... + + +@pytest.mark.parametrize( + "expected, value", + [ + (True, PositionalOnlyRenamed), + (True, PositionalOnlyAsPositionalOrKeyword), + (True, PositionalOnlyExtraOptional), + (True, PositionalOnlyAsVarPositional), + (False, PositionalOnlyMissingOne), + (False, PositionalOnlySwappedTypes), + (False, PositionalOnlyExtraRequired), + (False, PositionalOnlyAsKeywordOnly), + (False, PositionalOnlyAsVarKeyword), + ], +) +def test_implements_protocol_positional_only(expected, value): + assert implements_protocol(value, PositionalOnlyInterface) is expected + + +class PositionalOrKeywordInterface(Protocol): + def run(self, a: int, b: str) -> None: ... + + +class PositionalOrKeywordSameNames: + def run(self, a: int, b: str) -> None: ... + + +class PositionalOrKeywordRenamed: + def run(self, a: int, x: str) -> None: ... + + +class PositionalOrKeywordReordered: + def run(self, b: str, a: int) -> None: ... + + +class PositionalOrKeywordAsPositionalOnly: + def run(self, a: int, b: str, /) -> None: ... + + +class PositionalOrKeywordAsKeywordOnly: + def run(self, *, a: int, b: str) -> None: ... + + +class PositionalOrKeywordAsVarPositionalAndKeyword: + def run(self, *args, **kwargs) -> None: ... + + +class PositionalOrKeywordAsVarPositional: + def run(self, *args) -> None: ... + + +@pytest.mark.parametrize( + "expected, value", + [ + (True, PositionalOrKeywordSameNames), + (True, PositionalOrKeywordAsVarPositionalAndKeyword), + (False, PositionalOrKeywordRenamed), + (False, PositionalOrKeywordReordered), + (False, PositionalOrKeywordAsPositionalOnly), + (False, PositionalOrKeywordAsKeywordOnly), + (False, PositionalOrKeywordAsVarPositional), + ], +) +def test_implements_protocol_positional_or_keyword(expected, value): + assert implements_protocol(value, PositionalOrKeywordInterface) is expected + + +class KeywordOnlyInterface(Protocol): + def run(self, *, a: int, b: str) -> None: ... + + +class KeywordOnlySameNames: + def run(self, *, a: int, b: str) -> None: ... + + +class KeywordOnlyAsPositionalOrKeyword: + def run(self, a: int, b: str) -> None: ... + + +class KeywordOnlyRenamed: + def run(self, *, a: int, x: str) -> None: ... + + +class KeywordOnlyWrongType: + def run(self, *, a: int, b: int) -> None: ... + + +class KeywordOnlyAsPositionalOnly: + def run(self, a: int = 0, b: str = "-", /) -> None: ... + + +class KeywordOnlyAsVarKeyword: + def run(self, **kwargs) -> None: ... + + +class KeywordOnlyPartialVarKeyword: + def run(self, *, a: int, **kwargs) -> None: ... + + +class KeywordOnlyTypedVarKeyword: + def run(self, **kwargs: int) -> None: ... + + +class KeywordOnlyAnyVarKeyword: + def run(self, **kwargs: Any) -> None: ... + + +@pytest.mark.parametrize( + "expected, value", + [ + (True, KeywordOnlySameNames), + (True, KeywordOnlyAsPositionalOrKeyword), + (True, KeywordOnlyAsVarKeyword), + (True, KeywordOnlyPartialVarKeyword), + (True, KeywordOnlyAnyVarKeyword), + (False, KeywordOnlyRenamed), + (False, KeywordOnlyWrongType), + (False, KeywordOnlyAsPositionalOnly), + (False, KeywordOnlyTypedVarKeyword), + ], +) +def test_implements_protocol_keyword_only(expected, value): + assert implements_protocol(value, KeywordOnlyInterface) is expected + + +class VarKeywordInterface(Protocol): + def run(self, a: int, **kwargs) -> None: ... + + +class VarKeywordAccepted: + def run(self, a: int, **kwargs) -> None: ... + + +class VarKeywordNotAccepted: + def run(self, a: int) -> None: ... + + +class VarKeywordExtraOptional: + def run(self, a: int, *, b: str = "-", **kwargs) -> None: ... + + +class VarPositionalInterface(Protocol): + def run(self, *args: int) -> None: ... + + +class VarPositionalAccepted: + def run(self, *args: int) -> None: ... + + +class VarPositionalNotAccepted: + def run(self, a: int = 0) -> None: ... + + +class VarPositionalWrongType: + def run(self, *args: str) -> None: ... + + +class VarPositionalExtraOptional: + def run(self, a: int = 0, *args: int) -> None: ... + + +class VarPositionalExtraOptionalWrongType: + def run(self, a: str = "-", *args: int) -> None: ... + + +@pytest.mark.parametrize( + "expected, protocol, value", + [ + (True, VarKeywordInterface, VarKeywordAccepted), + (True, VarKeywordInterface, VarKeywordExtraOptional), + (False, VarKeywordInterface, VarKeywordNotAccepted), + (True, VarPositionalInterface, VarPositionalAccepted), + (True, VarPositionalInterface, VarPositionalExtraOptional), + (False, VarPositionalInterface, VarPositionalNotAccepted), + (False, VarPositionalInterface, VarPositionalWrongType), + (False, VarPositionalInterface, VarPositionalExtraOptionalWrongType), + ], +) +def test_implements_protocol_var_parameters(expected, protocol, value): + assert implements_protocol(value, protocol) is expected + + +class ExtraParamsInterface(Protocol): + def run(self, a: int) -> None: ... + + +class ExtraOptionalKeywordOnly: + def run(self, a: int, *, extra: bool = False) -> None: ... + + +class ExtraRequiredKeywordOnly: + def run(self, a: int, *, extra: bool) -> None: ... + + +class ExtraOptionalPositional: + def run(self, a: int, extra: bool = False) -> None: ... + + +class ExtraRequiredPositional: + def run(self, a: int, extra: bool) -> None: ... + + +class ExtraVarParams: + def run(self, a: int, *args, **kwargs) -> None: ... + + +@pytest.mark.parametrize( + "expected, value", + [ + (True, ExtraOptionalKeywordOnly), + (True, ExtraOptionalPositional), + (True, ExtraVarParams), + (False, ExtraRequiredKeywordOnly), + (False, ExtraRequiredPositional), + ], +) +def test_implements_protocol_extra_parameters(expected, value): + assert implements_protocol(value, ExtraParamsInterface) is expected + + +class DefaultsInterface(Protocol): + def run(self, a: int = 0) -> None: ... + + +class DefaultsSameValue: + def run(self, a: int = 0) -> None: ... + + +class DefaultsOtherValue: + def run(self, a: int = 3) -> None: ... + + +class DefaultsRequired: + def run(self, a: int) -> None: ... + + +class KeywordOnlyDefaultsInterface(Protocol): + def run(self, *, a: int = 0) -> None: ... + + +class KeywordOnlyDefaultsOptional: + def run(self, *, a: int = 3) -> None: ... + + +class KeywordOnlyDefaultsRequired: + def run(self, *, a: int) -> None: ... + + +@pytest.mark.parametrize( + "expected, protocol, value", + [ + (True, DefaultsInterface, DefaultsSameValue), + (True, DefaultsInterface, DefaultsOtherValue), + (False, DefaultsInterface, DefaultsRequired), + (True, KeywordOnlyDefaultsInterface, KeywordOnlyDefaultsOptional), + (False, KeywordOnlyDefaultsInterface, KeywordOnlyDefaultsRequired), + ], +) +def test_implements_protocol_defaults(expected, protocol, value): + assert implements_protocol(value, protocol) is expected + + +class AnyTypesInterface(Protocol): + def run(self, a: Any, b): + """No annotation for b nor for the return.""" + + +class AnyTypesAnnotated: + def run(self, a: int, b: str) -> float: + return 0.0 # pragma: no cover + + +class AnyTypesUnannotated: + def run(self, a, b): ... + + +class ExactTypesInterface(Protocol): + def run(self, a: List[float]) -> List[float]: ... + + +class ExactTypesUnannotatedParam: + def run(self, a) -> List[float]: + return [] # pragma: no cover + + +class ExactTypesSupertypeParam: + def run(self, a: Iterable[float]) -> List[float]: + return [] # pragma: no cover + + +class ExactTypesUnannotatedReturn: + def run(self, a: List[float]): + return [] # pragma: no cover + + +@pytest.mark.parametrize( + "expected, protocol, value", + [ + (True, AnyTypesInterface, AnyTypesAnnotated), + (True, AnyTypesInterface, AnyTypesUnannotated), + (False, ExactTypesInterface, ExactTypesUnannotatedParam), + (False, ExactTypesInterface, ExactTypesSupertypeParam), + (False, ExactTypesInterface, ExactTypesUnannotatedReturn), + ], +) +def test_implements_protocol_type_hints(expected, protocol, value): + assert implements_protocol(value, protocol) is expected + + +class MultipleMethodsInterface(Protocol): + def one(self, a: int) -> None: ... + + def two(self, b: str) -> None: ... + + +class MultipleMethodsImplemented: + def one(self, a: int) -> None: ... + + def two(self, b: str) -> None: ... + + +class MultipleMethodsOneMismatch: + def one(self, a: int) -> None: ... + + def two(self, b: int) -> None: ... + + +class MultipleMethodsOneMissing: + def one(self, a: int) -> None: ... + + +class StaticAndClassMethodsInterface(Protocol): + def make(self, a: int) -> None: ... + + +class StaticMethodImplements: + @staticmethod + def make(a: int) -> None: ... + + +class ClassMethodImplements: + @classmethod + def make(cls, a: int) -> None: ... + + +class NotAMethodImplements: + make = "not a method" + + +@pytest.mark.parametrize( + "expected, protocol, value", + [ + (True, MultipleMethodsInterface, MultipleMethodsImplemented), + (False, MultipleMethodsInterface, MultipleMethodsOneMismatch), + (False, MultipleMethodsInterface, MultipleMethodsOneMissing), + (True, StaticAndClassMethodsInterface, StaticMethodImplements), + (True, StaticAndClassMethodsInterface, ClassMethodImplements), + (False, StaticAndClassMethodsInterface, NotAMethodImplements), + ], +) +def test_implements_protocol_methods(expected, protocol, value): + assert implements_protocol(value, protocol) is expected + + @pytest.mark.parametrize( "expected, value", [ From cfb98fd343623f80692b894b89c9b0a94b178b92 Mon Sep 17 00:00:00 2001 From: Mauricio Villegas <5780272+mauvilsa@users.noreply.github.com> Date: Mon, 3 Aug 2026 06:46:07 +0200 Subject: [PATCH 4/4] Remove duplications and simplify code --- jsonargparse/_formatters.py | 46 ++++++++++++++----------------------- jsonargparse/_typehints.py | 12 ++++++---- 2 files changed, 24 insertions(+), 34 deletions(-) diff --git a/jsonargparse/_formatters.py b/jsonargparse/_formatters.py index 13b45b29..c291c7ce 100644 --- a/jsonargparse/_formatters.py +++ b/jsonargparse/_formatters.py @@ -24,6 +24,7 @@ from ._common import ( defaults_cache, get_optionals_as_positionals_actions, + get_unaliased_type, is_subclasses_disabled, parent_parser, supports_optionals_as_positionals, @@ -34,7 +35,13 @@ from ._optionals import import_ruamel from ._subcommands import ActionSubCommands, find_action from ._type_checking import ArgumentParser, ruamelCommentedMap -from ._typehints import ActionTypeHint, type_to_str +from ._typehints import ( + ActionTypeHint, + get_optional_arg, + get_subclass_or_closed_types, + is_subclass_spec, + type_to_str, +) __all__ = ["DefaultHelpFormatter"] @@ -54,8 +61,8 @@ class PercentTemplate(Template): def get_subparsers(parser: "ArgumentParser", prefix: str = "") -> dict[str | None, "ArgumentParser"]: """Returns the given parser and all its subcommand parsers, keyed by subcommand key.""" parsers: dict[str | None, ArgumentParser] = {} - if parser._subparsers is not None: - for key, subparser in parser._subparsers._group_actions[0].choices.items(): # type: ignore[union-attr] + if parser._subcommands_action is not None: + for key, subparser in parser._subcommands_action._name_parser_map.items(): full_key = (prefix + "." if prefix else "") + key parsers[full_key] = subparser parsers.update(get_subparsers(subparser, prefix=full_key)) @@ -90,8 +97,6 @@ def get_class_group_title(class_parser: "ArgumentParser") -> str | None: def get_class_parser(class_type, action: Action | None) -> "ArgumentParser | None": """Returns a parser for the arguments of a class, or None if not possible.""" - from ._typehints import ActionTypeHint - sub_add_kwargs = dict(getattr(action, "sub_add_kwargs", None) or {}) sub_add_kwargs.pop("linked_targets", None) try: @@ -102,28 +107,18 @@ def get_class_parser(class_type, action: Action | None) -> "ArgumentParser | Non def get_closed_type_parser(typehint, action: Action | None) -> "ArgumentParser | None": """Returns a parser for the arguments of a closed type, e.g. a dataclass, given its type hint.""" - from ._typehints import get_subclass_or_closed_types - - if typehint is None: - return None types = get_subclass_or_closed_types(typehint, also_lists=True, callable_return=True) - if not types or len(types) != 1 or not is_subclasses_disabled(types[0]): - return None - return get_class_parser(types[0], action) + if types and len(types) == 1 and is_subclasses_disabled(types[0]): + return get_class_parser(types[0], action) + return None def get_mapping_value_typehint(typehint): """Returns the type of the values of a mapping type hint, or None if not a mapping.""" - from ._typehints import get_optional_arg, get_typehint_origin, mapping_origin_types - - if typehint is None: + if not ActionTypeHint.is_mapping_typehint(typehint): return None - typehint = get_optional_arg(typehint) - if get_typehint_origin(typehint) in mapping_origin_types: - args = getattr(typehint, "__args__", ()) - if len(args) == 2: - return args[1] - return None + args = getattr(get_optional_arg(get_unaliased_type(typehint)), "__args__", ()) + return args[1] if len(args) == 2 else None def remove_leading_blank_line(cfg: ruamelCommentedMap) -> None: @@ -134,13 +129,6 @@ def remove_leading_blank_line(cfg: ruamelCommentedMap) -> None: del comments[0] -def is_subclass_spec_dict(value) -> bool: - """Tests whether a config object corresponds to a subclass spec.""" - from ._typehints import _subclass_spec_keys - - return isinstance(value.get("class_path"), str) and not (set(value.keys()) - _subclass_spec_keys) - - class YAMLCommentFormatter: """Formatter class for adding YAML comments to configuration files.""" @@ -222,7 +210,7 @@ def set_dict_comments( prefix: The parser key that corresponds to the given config object. typehint: The type of the config object, defaults to the type of the action. """ - if is_subclass_spec_dict(cfg): + if is_subclass_spec(cfg): self.set_subclass_comments(cfg, action, depth) return if typehint is None and isinstance(action, ActionTypeHint): diff --git a/jsonargparse/_typehints.py b/jsonargparse/_typehints.py index 72e7d103..de3c55a5 100644 --- a/jsonargparse/_typehints.py +++ b/jsonargparse/_typehints.py @@ -1542,11 +1542,13 @@ def is_instance_factory_protocol(class_type, logger=None): def is_subclass_spec(val): - is_class = isinstance(val, (dict, Namespace)) and "class_path" in val - if is_class: - keys = getattr(val, "__dict__", val).keys() - is_class = len(set(keys) - _subclass_spec_keys) == 0 - return is_class + if isinstance(val, Namespace): + keys = val.__dict__.keys() # only the top level keys of the namespace + elif isinstance(val, dict): + keys = val.keys() + else: + return False + return "class_path" in keys and len(set(keys) - _subclass_spec_keys) == 0 def subclass_spec_as_namespace(val, prev_val=None):