Fix list of paths files not relative to their location and nested sub_configs in add_argument

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 <https://github.com/mauvilsa/jsonargparse/pull/1022>`__).
+- ``Optional[list[<path_type>]]`` with ``sub_configs=True`` also accepts a file
+  with a list of paths (`#1023
+  <https://github.com/mauvilsa/jsonargparse/pull/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
   <https://github.com/mauvilsa/jsonargparse/pull/1020>`__).
+- Plain text files listing paths, i.e. ``list[<path_type>]`` 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 <https://github.com/mauvilsa/jsonargparse/pull/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
+  <https://github.com/mauvilsa/jsonargparse/pull/1023>`__).
 
 Changed
 ^^^^^^^

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[<path_type>]``
-and ``sub_configs=True``. The special string ``'-'`` means stdin:
+plain text file or from stdin, add the argument with type ``list[<path_type>]``,
+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,

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):

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

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

jsonargparse/_typehints.py

@@ -748,6 +748,13 @@
                     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 @@
                 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_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 @@
                     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 @@
         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

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