diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0c23e453..79e7e7d5 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -72,6 +72,9 @@ Added file, e.g. ``pyproject.toml#tool.myapp``, in default config files, the config argument and ``parse_path``. Default config files without the key are skipped (`#1022 `__). +- ``Optional[list[]]`` with ``sub_configs=True`` also accepts a file + with a list of paths (`#1023 + `__). Fixed ^^^^^ @@ -204,6 +207,15 @@ Fixed defined in ``_io``, only showing the defining path, as if it was the one imported, and not saying which path to allow (`#1020 `__). +- Plain text files listing paths, i.e. ``list[]`` with + ``sub_configs=True``, failing with an unclear ``Expected type ... Got value`` + error when the file is not in the working directory or a listed path does not + exist. Also, a file listing a single existing file giving the lines of that + file instead (`#1023 `__). +- ``add_argument`` with ``sub_configs=True`` and a class type not accepting + sub-config files for the parameters of nested classes, failing with + ``Unexpected import path format`` (`#1023 + `__). Changed ^^^^^^^ diff --git a/DOCUMENTATION.rst b/DOCUMENTATION.rst index 0529ff74..9a553960 100644 --- a/DOCUMENTATION.rst +++ b/DOCUMENTATION.rst @@ -837,8 +837,9 @@ The content of the file is read with the :py:meth:`.Path.read_text` method, e.g. An argument with a path type can be given ``nargs='+'`` to accept multiple paths, i.e. ``--files file1 file2``. To instead read a list of paths from a -plain text file or from stdin, add the argument with type ``list[]`` -and ``sub_configs=True``. The special string ``'-'`` means stdin: +plain text file or from stdin, add the argument with type ``list[]``, +optionally ``Optional``, and ``sub_configs=True``. Relative paths in the file +are relative to its location. The special string ``'-'`` means stdin: .. testsetup:: path_list @@ -2499,7 +2500,8 @@ Instead of writing a subclass spec inline, a path to a config file that holds it can be given. This splits a large config into smaller reusable files. It requires the argument to be 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. +<.ArgumentParser.add_argument>` and the ``add_*_arguments`` methods. It also +applies to the parameters of nested classes. This also works for the items of a list of classes and for the values of a dict of classes, useful when each component has its own config file. For example, diff --git a/jsonargparse/_core.py b/jsonargparse/_core.py index 32d33757..830ea9c2 100644 --- a/jsonargparse/_core.py +++ b/jsonargparse/_core.py @@ -205,6 +205,8 @@ def add_argument(self, *args, sub_configs: bool = False, **kwargs): kwargs["choices"] = tuple(kwargs["choices"]) action = super().add_argument(*args, **kwargs) action.logger = self._logger # type: ignore[attr-defined] + if sub_configs and isinstance(action, ActionTypeHint): + action.sub_add_kwargs = {"sub_configs": True} # also for nested classes ActionConfigFile._add_print_config_argument(self, action) ActionJsonnet._check_ext_vars_action(parser, action) if is_meta_key(action.dest): diff --git a/jsonargparse/_formatters.py b/jsonargparse/_formatters.py index 6cd0dcdf..11197d03 100644 --- a/jsonargparse/_formatters.py +++ b/jsonargparse/_formatters.py @@ -376,6 +376,10 @@ def _find_position(source, cache: dict) -> tuple[int, int, str] | None: """Returns the line number, column and line of the config file where a value came from, if known.""" from ._loaders_dumpers import get_config_mode + if source.line is not None: + if id(source.origin) not in cache: + cache[id(source.origin)] = ({}, source.origin.read_text().splitlines()) + return source.line, 0, cache[id(source.origin)][1][source.line - 1].rstrip() if source.key is None or get_config_mode(source.origin, source.mode) not in line_number_modes: return None if id(source.origin) not in cache: # each config file is read only once diff --git a/jsonargparse/_namespace.py b/jsonargparse/_namespace.py index f42fa832..a4eecea5 100644 --- a/jsonargparse/_namespace.py +++ b/jsonargparse/_namespace.py @@ -24,6 +24,7 @@ class ValueSource(NamedTuple): key: str | None = None # the key in a config file that the value corresponds to implied_by: "ValueSource | None" = None # for implicit values, e.g. a class_path, the source of what implied it selector: str | None = None # the key selected in a config file with path#key, of which key is relative + line: int | None = None # the line number in a plain text file, e.g. a list of paths, which has no keys def __deepcopy__(self, memo): return self # immutable, so copies of namespaces can share it, which is much faster, e.g. for list items diff --git a/jsonargparse/_typehints.py b/jsonargparse/_typehints.py index ee9f13d1..c04ac973 100644 --- a/jsonargparse/_typehints.py +++ b/jsonargparse/_typehints.py @@ -748,6 +748,13 @@ def _check_type(self, value, append=False, cfg=None, mode=None): config_path = None if isinstance(val, str) and val.strip()[:1] in {"[", "{"}: load_error = ex # included in the error, since it seems intended as a list or dict + list_file = ( + config_path is not None + and isinstance(val, (str, NoneType)) # None for a file listing a path named null + and is_list_pathlike(self._typehint) + ) + if list_file: # a plain text list of paths, read when adapting, relative to the current directory + val, config_path = orig_val, 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) @@ -773,8 +780,14 @@ def _check_type(self, value, append=False, cfg=None, mode=None): except ValueError as ex: if orig_val == "-" and isinstance(getattr(ex, "parent", None), PathError): raise ex + item_ex: BaseException | None = ex + while item_ex is not None and not hasattr(item_ex, "list_index"): # e.g. under a union error + item_ex = item_ex.__cause__ + if config_path is not None and not append and isinstance(val, list) and item_ex is not None: + source = ValueSource("config file", config_path, mode, key=f"[{item_ex.list_index}]") + item_ex.value_source = getattr(item_ex, "value_source", source) try: - if isinstance(orig_val, str): + if isinstance(orig_val, str) and not list_file: # without load_config_path_context, since orig_val could be the path it would load with path_dir_context(config_path): val = adapt_typehints(orig_val, self._typehint, default=self.default, **kwargs) @@ -915,6 +928,8 @@ def is_pathlike(typehint) -> bool: def is_list_pathlike(typehint) -> bool: + if is_optional(typehint): + typehint = next(a for a in typehint.__args__ if a is not NoneType) typehint_origin = get_typehint_origin(typehint) if typehint_origin in sequence_origin_types and hasattr(typehint, "__args__"): return is_pathlike(typehint.__args__[0]) @@ -1730,8 +1745,14 @@ def adapt_typehints( adapt_kwargs_n = {**deepcopy(copied), **shared, "prev_val": prev_val[n]} else: adapt_kwargs_n = {**deepcopy(copied), **shared} - with path_dir_context(list_path): - val[n] = adapt_typehints(v, subtypehints[0], **adapt_kwargs_n) + try: + with path_dir_context(list_path): + val[n] = adapt_typehints(v, subtypehints[0], **adapt_kwargs_n) + except ValueError as ex: + if list_path is not None: + ex.value_source = ValueSource("list of paths file", list_path, line=n + 1) + ex.list_index = n # the outermost list's, for its position in a file + raise if typehint_origin is deque: val = list(val) if serialize else deque(val) elif typehint_origin is abc.Iterator and instantiate_classes: @@ -3218,7 +3239,7 @@ def typehint_metavar(typehint): enum = typehint.__args__[0] metavar = iter_to_set_str(list(enum.__members__) + ["null"]) elif is_list_pathlike(typehint): - metavar = "'[\"PATH1\",...]' | LIST_OF_PATHS_FILE | -" + metavar = "'[\"PATH1\",...]' | LIST_OF_PATHS_FILE | -" + (" | null" if is_optional(typehint) else "") elif typehint_origin in tuple_set_origin_types or typehint_origin in sequence_origin_types: metavar = "[ITEM,...]" return metavar diff --git a/jsonargparse_tests/test_paths.py b/jsonargparse_tests/test_paths.py index 1ea63b93..af903bec 100644 --- a/jsonargparse_tests/test_paths.py +++ b/jsonargparse_tests/test_paths.py @@ -29,6 +29,7 @@ responses_activate, responses_available, skip_if_fsspec_unavailable, + skip_if_no_pyyaml, skip_if_requests_unavailable, skip_if_responses_unavailable, skip_if_running_as_root, @@ -971,6 +972,26 @@ def test_sub_configs_list_subclass_nested_subconfig(parser, tmp_cwd): assert isinstance(init.main.objects[0].sub, ItemSub) +def test_sub_configs_add_argument_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_argument("--object", type=ItemBase, sub_configs=True) + parser.add_argument("--objects", type=List[ItemBase], sub_configs=True) + + cfg = parser.parse_args(["--object=subdir/outer.yaml", '--objects=["subdir/outer.yaml"]']) + for obj in [cfg.object, cfg.objects[0]]: + assert obj.init_args.sub.class_path == f"{__name__}.ItemSub" + assert obj.init_args.sub.init_args.y == "n" + init = parser.instantiate(cfg) + assert isinstance(init.object.sub, ItemSub) + assert isinstance(init.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"])) @@ -1203,6 +1224,66 @@ def test_sub_configs_list_path_fr(parser, tmp_cwd, mock_stdin, subtests): ctx.match("Expected a path but no-such-file is either not accessible or invalid") +def test_sub_configs_list_path_fr_relative_to_list_file(parser, tmp_cwd, subtests): + subdir = tmp_cwd / "subdir" + subdir.mkdir() + (subdir / "file1").touch() + (subdir / "file2").write_text("file1\n") # an existing path, which must not be read as a list + (subdir / "files.lst").write_text("file1\nfile2\n") + (subdir / "single.lst").write_text("file2\n") + (subdir / "missing.lst").write_text("file1\nfile3\n") + pathlib.Path("config.yaml").write_text(json_or_yaml_dump({"list": "subdir/files.lst"})) + pathlib.Path("config_missing.yaml").write_text(json_or_yaml_dump({"list": "subdir/missing.lst"})) + + parser.add_argument("--cfg", action="config") + parser.add_argument("--list", type=List[Path_fr], sub_configs=True) + + def resolved(paths): + return [pathlib.Path(os.fspath(p)).resolve() for p in paths] + + with subtests.test("list file from command line"): + cfg = parser.parse_args(["--list=subdir/files.lst"]) + assert resolved(cfg.list) == resolved([subdir / "file1", subdir / "file2"]) + + with subtests.test("list file from config file"): + cfg = parser.parse_args(["--cfg=config.yaml"]) + assert resolved(cfg.list) == resolved([subdir / "file1", subdir / "file2"]) + + with subtests.test("list file with a single path"): + cfg = parser.parse_args(["--list=subdir/single.lst"]) + assert resolved(cfg.list) == resolved([subdir / "file2"]) + + list_source = "Source: list of paths file subdir/missing.lst:2\n 2 | file3" + + with subtests.test("path not exist from command line"): + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(["--list=subdir/missing.lst"]) + ctx.match("File does not exist") + assert list_source in str(ctx.value) + + with subtests.test("path not exist from config file"): + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(["--cfg=config_missing.yaml"]) + ctx.match("File does not exist") + assert list_source in str(ctx.value) + + +@skip_if_no_pyyaml +def test_sub_configs_list_path_fr_yaml_list_path_not_exist(parser, tmp_cwd): + subdir = tmp_cwd / "subdir" + subdir.mkdir() + (subdir / "file1").touch() + (subdir / "missing.yaml").write_text("- file1\n- file3\n") + parser.add_argument("--list", type=List[Path_fr], sub_configs=True) + parser.add_argument("--optional", type=Optional[List[Path_fr]], sub_configs=True) + + for arg in ["--list", "--optional"]: + with pytest.raises(ArgumentError) as ctx: + parser.parse_args([f"{arg}=subdir/missing.yaml"]) + ctx.match("File does not exist") + assert "Source: config file subdir/missing.yaml:2\n 2 | - file3" in str(ctx.value) + + @pytest.mark.parametrize("validate_defaults", [False, True]) @patch_parsing_settings def test_sub_configs_list_path_fr_default_stdin(parser, tmp_cwd, validate_defaults, mock_stdin, subtests): @@ -1259,6 +1340,38 @@ def test_add_class_list_path(parser, tmp_cwd): assert "'[\"PATH1\",...]' | LIST_OF_PATHS_FILE | -" in help_str +class ClassOptionalListPath: + def __init__(self, files: Optional[list[Path_fr]] = None): + self.files = files # pragma: no cover + + +def test_add_class_optional_list_path(parser, tmp_cwd): + subdir = tmp_cwd / "subdir" + subdir.mkdir() + (subdir / "file1").touch() + (subdir / "files.lst").write_text("file1\n") + (subdir / "missing.lst").write_text("file1\nfile2\n") + (subdir / "null").touch() + (subdir / "null.lst").write_text("null\n") # loads as None, but is a path + + parser.add_class_arguments(ClassOptionalListPath, "cls", sub_configs=True) + + cfg = parser.parse_args(["--cls.files=subdir/files.lst"]) + assert [pathlib.Path(os.fspath(p)).resolve() for p in cfg.cls.files] == [(subdir / "file1").resolve()] + assert cfg.cls.files[0].relative == "file1" + assert parser.parse_args(["--cls.files=null"]).cls.files is None + cfg = parser.parse_args(["--cls.files=subdir/null.lst"]) + assert [pathlib.Path(os.fspath(p)).resolve() for p in cfg.cls.files] == [(subdir / "null").resolve()] + + with pytest.raises(ArgumentError) as ctx: + parser.parse_args(["--cls.files=subdir/missing.lst"]) + ctx.match("File does not exist") + assert "Source: list of paths file subdir/missing.lst:2\n 2 | file2" in str(ctx.value) + + help_str = get_parser_help(parser) + assert "'[\"PATH1\",...]' | LIST_OF_PATHS_FILE | - | null" in help_str + + class DataOptionalPath: def __init__(self, path: Optional[os.PathLike] = None): pass # pragma: no cover