diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6a047044..ff7d2e8e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -46,6 +46,19 @@ Fixed completion. A config that has settings for both a subcommand name and one of its aliases now fails, instead of one of them being silently discarded (`#978 `__). +- Loading a config file failed if its directory was removed while loading, and + loading config files concurrently in threads resolved relative paths against + the wrong directory (`#979 + `__). +- Relative paths in a config file reached through a symlinked directory got the + symlink resolved (`#979 + `__). +- A parse error was masked by a ``FileNotFoundError`` when the working directory + had been removed while parsing (`#979 + `__). +- ``from_config`` failed to resolve ``import`` statements in a jsonnet config + given as a relative path with a directory, e.g. ``sub/config.jsonnet`` (`#979 + `__). Changed ^^^^^^^ @@ -88,6 +101,9 @@ Changed - The ``comments`` flag of the print config argument is now always accepted and listed in the help, and fails with an informative error when ``ruamel.yaml`` is not installed (`#975 `__). +- Relative paths in config files are now resolved without changing the process + working directory (`#979 + `__). Removed ^^^^^^^ diff --git a/jsonargparse/_actions.py b/jsonargparse/_actions.py index e8b82c80..56831542 100644 --- a/jsonargparse/_actions.py +++ b/jsonargparse/_actions.py @@ -21,7 +21,7 @@ from ._loaders_dumpers import get_loader_exceptions, load_value from ._namespace import Namespace, ValueSource, copy_provenance, value_source_context from ._optionals import _get_config_read_mode, ruamel_support -from ._paths import change_to_path_dir +from ._paths import path_dir_context from ._type_checking import ArgumentParser from ._util import ( Path, @@ -316,7 +316,7 @@ def _load_config(self, value, parser): if not isinstance(cfg, (dict, Namespace)): raise TypeError(f'Parser key "{self.dest}": Unable to load config "{value}"') source = None if cfg_path is None else ValueSource("config file", cfg_path, parser.parser_mode) - with load_config_path_context(cfg_path), change_to_path_dir(cfg_path), value_source_context(source): + with load_config_path_context(cfg_path), path_dir_context(cfg_path), value_source_context(source): cfg = parser._apply_actions(cfg, parent_key=self.dest) return cfg except (SubclassesDisabledError, ImportDenied) as ex: diff --git a/jsonargparse/_core.py b/jsonargparse/_core.py index 6a77e781..9c663a57 100644 --- a/jsonargparse/_core.py +++ b/jsonargparse/_core.py @@ -77,7 +77,7 @@ pyyaml_available, ) from ._parameter_resolvers import UnknownDefault -from ._paths import change_to_path_dir +from ._paths import path_dir_context from ._required import ( iter_required_keys, restore_suppressed_required, @@ -688,7 +688,7 @@ def parse_path( ArgumentError: If the parsing fails and ``exit_on_error=False``. """ fpath = Path(path, mode=_get_config_read_mode()) - with load_config_path_context(fpath), change_to_path_dir(fpath): + with load_config_path_context(fpath), path_dir_context(fpath): content = fpath.read_text() parsed_cfg = self.parse_string( content=content, @@ -1038,7 +1038,7 @@ def save_paths(cfg): f.write(val.read_text()) cfg[key] = type(val)(str(val_path)) - with change_to_path_dir(path_fc), parser_context(parent_parser=self): + with path_dir_context(path_fc), parser_context(parent_parser=self): save_paths(cfg) dump_kwargs["skip_validation"] = True with open(path_fc.absolute, "w") as f: @@ -1112,7 +1112,7 @@ def get_defaults(self, skip_validation: bool = False) -> Namespace: for default_config_file in default_config_files: with ( load_config_path_context(default_config_file), - change_to_path_dir(default_config_file), + path_dir_context(default_config_file), parser_context(parent_parser=self, parsing_defaults=True), ): default_config_file_content = default_config_file.read_text() diff --git a/jsonargparse/_formatters.py b/jsonargparse/_formatters.py index e8925107..087479af 100644 --- a/jsonargparse/_formatters.py +++ b/jsonargparse/_formatters.py @@ -362,12 +362,15 @@ def _describe_origin(origin) -> str: """For config files, the path relative to the working directory if inside it, so that it can be opened.""" import pathlib - from ._paths import Path, get_initial_working_directory + from ._paths import Path if not isinstance(origin, Path) or origin.is_url or origin.is_fsspec: return str(origin) absolute = pathlib.Path(origin.absolute) - cwd = pathlib.Path(get_initial_working_directory()) + try: + cwd = pathlib.Path.cwd() + except OSError: # the working directory was removed, so the error must not be masked + return str(absolute) return str(absolute.relative_to(cwd) if absolute.is_relative_to(cwd) else absolute) diff --git a/jsonargparse/_from_config.py b/jsonargparse/_from_config.py index 74bee672..632f95eb 100644 --- a/jsonargparse/_from_config.py +++ b/jsonargparse/_from_config.py @@ -8,7 +8,7 @@ from ._core import ArgumentParser from ._loaders_dumpers import get_loader_exceptions, load_value from ._optionals import _get_config_read_mode -from ._paths import change_to_path_dir +from ._paths import path_dir_context from ._required import clear_required, iter_required_keys from ._typehints import is_subclass_spec, resolve_class_path_by_name from ._util import import_object, load_config_path_context @@ -71,7 +71,7 @@ def _parse_class_kwargs_from_config(cls: type[T], config: str | PathLike | dict, cfg_path = Path(config, mode=_get_config_read_mode()) with ( load_config_path_context(cfg_path), - change_to_path_dir(cfg_path), + path_dir_context(cfg_path), parser_context(load_value_mode=parser.parser_mode), ): cfg_str = cfg_path.read_text() @@ -94,7 +94,7 @@ def _parse_class_kwargs_from_config(cls: type[T], config: str | PathLike | dict, parser.add_class_arguments(cls) for required in iter_required_keys(parser): clear_required(parser, required) - with load_config_path_context(cfg_path), change_to_path_dir(cfg_path): + with load_config_path_context(cfg_path), path_dir_context(cfg_path): cfg = parser.parse_object(config, defaults=False) return parser.instantiate(cfg).as_dict(), cls diff --git a/jsonargparse/_loaders_dumpers.py b/jsonargparse/_loaders_dumpers.py index 261dead3..4d9b3c66 100644 --- a/jsonargparse/_loaders_dumpers.py +++ b/jsonargparse/_loaders_dumpers.py @@ -1,6 +1,7 @@ """Code related to loading and dumping.""" import inspect +import os import re from argparse import HelpFormatter from collections.abc import Callable @@ -17,6 +18,7 @@ pyyaml_available, ruamel_support, ) +from ._paths import current_local_dir from ._type_checking import ArgumentParser __all__ = [ @@ -122,6 +124,10 @@ def jsonnet_load(stream, path="", ext_vars=None): ext_vars, ext_codes = ActionJsonnet.split_ext_vars(ext_vars) _jsonnet = import_jsonnet("jsonnet_load") + path_dir = current_local_dir.get() + if path_dir and not os.path.isabs(path): + # jsonnet resolves imports relative to the given file name, which path_dir already accounts for + path = os.path.join(path_dir, os.path.basename(path) or "snippet") try: val = _jsonnet.evaluate_snippet(path, stream, ext_vars=ext_vars, ext_codes=ext_codes) except RuntimeError: diff --git a/jsonargparse/_paths.py b/jsonargparse/_paths.py index 97a38153..72a93f8a 100644 --- a/jsonargparse/_paths.py +++ b/jsonargparse/_paths.py @@ -17,8 +17,8 @@ url_support, ) +current_local_dir: ContextVar[str | None] = ContextVar("current_local_dir", default=None) _current_path_dir: ContextVar[str | None] = ContextVar("_current_path_dir", default=None) -_initial_cwd: ContextVar[str | None] = ContextVar("_initial_cwd", default=None) _remote_relative_disabled: ContextVar[bool] = ContextVar("_remote_relative_disabled", default=False) @@ -185,7 +185,7 @@ def __init__( is_fsspec = True else: if cwd is None: - cwd = os.getcwd() + cwd = current_local_dir.get() or os.getcwd() abs_path = abs_path if is_absolute else os.path.join(cwd, abs_path) url_data = None else: @@ -345,7 +345,7 @@ def open(self, mode: str = "r") -> Iterator[IO]: @contextmanager def relative_path_context(self) -> Iterator[str]: """Context manager to use this path's parent (directory or URL) for relative paths defined within.""" - with change_to_path_dir(self) as path_dir: + with path_dir_context(self) as path_dir: assert isinstance(path_dir, str) yield path_dir @@ -384,42 +384,34 @@ def disable_remote_relative_paths(disable: bool = True) -> Iterator[None]: @contextmanager -def change_to_path_dir(path: Path | str | None) -> Iterator[str | None]: - """A context manager for running code in the directory of a path.""" +def path_dir_context(path: Path | None) -> Iterator[str | None]: + """A context manager to resolve relative paths with respect to the directory of a path. + + The process working directory is not modified, so that concurrent parsing and + removal of the original directory are not a problem. + """ + local_dir = current_local_dir.get() path_dir = _current_path_dir.get() - chdir: bool | str = False + is_local = False if path is not None: - if isinstance(path, str): - path = Path(path, mode="d") if path._url_data and (path.is_url or path.is_fsspec): scheme = path._url_data.scheme path_dir = path._url_data.url_path else: scheme = "" path_dir = path.absolute - chdir = True + is_local = True if "d" not in path.mode: path_dir = os.path.dirname(path_dir) path_dir = scheme + path_dir - token = _current_path_dir.set(path_dir) - initial_cwd_token = None - if chdir and path_dir: - chdir = os.getcwd() - initial_cwd_token = _initial_cwd.set(_initial_cwd.get() or chdir) - path_dir = os.path.abspath(path_dir) - os.chdir(path_dir) + if is_local and path_dir: + path_dir = local_dir = os.path.abspath(path_dir) + token = _current_path_dir.set(path_dir) + local_token = current_local_dir.set(local_dir) try: yield path_dir finally: + current_local_dir.reset(local_token) _current_path_dir.reset(token) - if chdir: - os.chdir(chdir) - if initial_cwd_token is not None: - _initial_cwd.reset(initial_cwd_token) - - -def get_initial_working_directory() -> str: - """Returns the working directory from before changing to the directories of config files.""" - return _initial_cwd.get() or os.getcwd() diff --git a/jsonargparse/_typehints.py b/jsonargparse/_typehints.py index 3575b551..38d752e1 100644 --- a/jsonargparse/_typehints.py +++ b/jsonargparse/_typehints.py @@ -103,7 +103,7 @@ typing_extensions_import, validate_annotated, ) -from ._paths import Path, PathError, change_to_path_dir, disable_remote_relative_paths +from ._paths import Path, PathError, disable_remote_relative_paths, path_dir_context from ._required import clear_required from ._subcommands import find_action, find_parent_action, parse_kwargs from ._type_checking import ArgumentParser @@ -762,14 +762,14 @@ def _check_type(self, value, append=False, cfg=None, mode=None): "logger": self.logger, } try: - with load_config_path_context(config_path), change_to_path_dir(config_path): + with load_config_path_context(config_path), path_dir_context(config_path): val = adapt_typehints(val, self._typehint, **kwargs) except ValueError as ex: if orig_val == "-" and isinstance(getattr(ex, "parent", None), PathError): raise ex try: if isinstance(orig_val, str): - with load_config_path_context(config_path), change_to_path_dir(config_path): + with load_config_path_context(config_path), path_dir_context(config_path): val = adapt_typehints(orig_val, self._typehint, default=self.default, **kwargs) ex = None except ValueError: @@ -931,7 +931,7 @@ def adapt_subconfig_path(val, typehint, adapt_kwargs): 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): + with load_config_path_context(path), path_dir_context(path): val = adapt_typehints(subconfig, typehint, **adapt_kwargs) fill_provenance(val, ValueSource("config file", path, get_load_value_mode())) if isinstance(val, (Namespace, dict)): @@ -1601,7 +1601,7 @@ def adapt_typehints( adapt_kwargs_n = {**deepcopy(copied), **shared, "prev_val": prev_val[n]} else: adapt_kwargs_n = {**deepcopy(copied), **shared} - with change_to_path_dir(list_path): + with path_dir_context(list_path): val[n] = adapt_typehints(v, subtypehints[0], **adapt_kwargs_n) if typehint_origin is deque: val = list(val) if serialize else deque(val) diff --git a/jsonargparse/typing.py b/jsonargparse/typing.py index e12f1df0..f95f22a6 100644 --- a/jsonargparse/typing.py +++ b/jsonargparse/typing.py @@ -19,7 +19,7 @@ ) from ._namespace import Namespace from ._optionals import final, is_alias_type, pydantic_support -from ._paths import Path, change_to_path_dir +from ._paths import Path from ._util import ClassFromFunctionBase, get_import_path, import_object __all__ = [ @@ -421,8 +421,7 @@ class PathType(Path): def __init__(self, v, **k): if isinstance(v, dict) and set(v) == {"cwd", "relative"}: - with change_to_path_dir(v["cwd"]): - super().__init__(v["relative"], mode=self._mode, **k) + super().__init__(v["relative"], mode=self._mode, cwd=v["cwd"], **k) else: super().__init__(v, mode=self._mode, **k) diff --git a/jsonargparse_tests/test_jsonnet.py b/jsonargparse_tests/test_jsonnet.py index d0ceff63..c797466b 100644 --- a/jsonargparse_tests/test_jsonnet.py +++ b/jsonargparse_tests/test_jsonnet.py @@ -11,6 +11,7 @@ ActionJsonSchema, ArgumentError, ArgumentParser, + FromConfigMixin, ) from jsonargparse._optionals import jsonnet_support from jsonargparse_tests.conftest import ( @@ -130,6 +131,23 @@ def __init__(self, name: str = "Lucky", prize: int = 100): assert cfg.group.prize == 80 +def test_parser_mode_jsonnet_from_config_relative_path(tmp_cwd): + class App(FromConfigMixin): + __from_config_parser_kwargs__ = {"parser_mode": "jsonnet"} + + def __init__(self, name: str = "Lucky", prize: int = 100): + self.name = name + self.prize = prize + + Path("conf").mkdir() + Path("conf", "name.libsonnet").write_text('"Mike"') + Path("conf", "test.jsonnet").write_text('local name = import "name.libsonnet"; {"name": name, "prize": 80}') + + app = App.from_config(Path("conf", "test.jsonnet")) + assert app.name == "Mike" + assert app.prize == 80 + + # test action jsonnet diff --git a/jsonargparse_tests/test_paths.py b/jsonargparse_tests/test_paths.py index 333ea7c8..772cd771 100644 --- a/jsonargparse_tests/test_paths.py +++ b/jsonargparse_tests/test_paths.py @@ -4,7 +4,9 @@ import json import os import pathlib +import shutil import stat +import threading import zipfile from io import StringIO from typing import Any, Callable, Dict, List, Optional, Union @@ -13,6 +15,7 @@ import pytest from jsonargparse import ArgumentError, ArgumentParser, Namespace, set_parsing_settings +from jsonargparse._common import parser_context from jsonargparse._optionals import fsspec_support, url_support from jsonargparse._paths import _current_path_dir, _parse_url from jsonargparse.typing import Path, Path_drw, Path_fc, Path_fr, SecretStr, path_type @@ -583,6 +586,88 @@ def test_secret_in_union_relative_url_not_resolved_as_remote(): assert "PASSWORD" == cfg.password.get_secret_value() +# relative path context and working directory tests + + +@pytest.mark.skipif(not is_posix, reason="symlinks not supported") +def test_relative_path_context_keeps_symlinked_dir(tmp_cwd): + real_dir = tmp_cwd / "real" + real_dir.mkdir() + (real_dir / "file.txt").touch() + link_dir = tmp_cwd / "link" + link_dir.symlink_to(real_dir) + + with Path_drw(link_dir).relative_path_context(): + path = Path_fr("file.txt") + + assert path.cwd == str(link_dir) + assert path.absolute == str(link_dir / "file.txt") + + +@pytest.mark.skipif(not is_posix, reason="the working directory can't be removed in windows") +def test_relative_path_context_cwd_removed(tmp_cwd): + removed_dir = tmp_cwd / "removed" + removed_dir.mkdir() + other_dir = tmp_cwd / "other" + other_dir.mkdir() + (other_dir / "file.txt").touch() + other_path = Path_drw(other_dir) + + os.chdir(removed_dir) + try: + with other_path.relative_path_context(): + shutil.rmtree(removed_dir) + path = Path_fr("file.txt") + finally: + os.chdir(tmp_cwd) + + assert path.absolute == str(other_dir / "file.txt") + + +def test_relative_path_context_threads(tmp_cwd): + subdirs = [] + for name in ["one", "two"]: + subdir = tmp_cwd / name + subdir.mkdir() + (subdir / "file.txt").touch() + subdirs.append(subdir) + + barrier = threading.Barrier(len(subdirs)) + resolved = {} + + def resolve(subdir): + with Path_drw(subdir).relative_path_context(): + barrier.wait(timeout=10) + resolved[subdir.name] = Path_fr("file.txt").absolute + + threads = [threading.Thread(target=resolve, args=(s,)) for s in subdirs] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert resolved == {s.name: str(s / "file.txt") for s in subdirs} + + +@skip_if_fsspec_unavailable +@patch_parsing_settings +def test_relative_path_context_local_dir_kept_inside_remote(tmp_cwd): + """A path not resolved against the remote parent uses the closest local directory.""" + set_parsing_settings(config_read_mode_fsspec_enabled=True) + subdir = tmp_cwd / "sub" + subdir.mkdir() + (subdir / "file.txt").touch() + with fsspec.open("memory://outer/inner/config.yaml", "w") as f: + f.write("data") + + with Path_drw(subdir).relative_path_context(): + with Path("memory://outer/inner/config.yaml", mode="fsr").relative_path_context(): + path = Path_fr("file.txt") + + assert path.cwd == str(subdir) + assert path.absolute == str(subdir / "file.txt") + + # path types tests @@ -666,6 +751,45 @@ def test_path_dump(parser, tmp_cwd): assert json_or_yaml_load(parser.dump(cfg)) == {"path": "path"} +def test_path_dump_preserve_relative_round_trip(parser, tmp_cwd): + subdir = tmp_cwd / "sub" + subdir.mkdir() + (subdir / "file.txt").touch() + (subdir / "config.yaml").write_text(json_or_yaml_dump({"path": "file.txt"})) + + parser.add_argument("--cfg", action="config") + parser.add_argument("--path", type=Path_fr) + cfg = parser.parse_args([f"--cfg={subdir / 'config.yaml'}"]) + assert cfg.path.relative == "file.txt" + + with parser_context(path_dump_preserve_relative=True): + dump = json_or_yaml_load(parser.dump(cfg)) + assert dump["path"] == {"relative": "file.txt", "cwd": str(subdir)} + + assert parser.parse_object(dump).path.absolute == str(subdir / "file.txt") + + +def test_path_dump_preserve_relative_load_cwd_removed(tmp_cwd): + removed_dir = tmp_cwd / "removed" + serialized = {"relative": "out.txt", "cwd": str(removed_dir)} + + path = path_type("fcc")(serialized) + assert path.relative == "out.txt" + assert path.cwd == str(removed_dir) + assert path.absolute == str(removed_dir / "out.txt") + + +@skip_if_fsspec_unavailable +def test_path_dump_preserve_relative_load_fsspec_cwd(): + with fsspec.open("memory://preserve/relative/file.txt", "w") as f: + f.write("content") + + path = path_type("fsr")({"relative": "file.txt", "cwd": "memory://preserve/relative"}) + assert path.relative == "file.txt" + assert path.absolute == "memory://preserve/relative/file.txt" + assert path.read_text() == "content" + + def test_paths_dump(parser, tmp_cwd): parser.add_argument("--paths", nargs="+", type=Path_fc) cfg = parser.parse_args(["--paths", "path1", "path2"]) diff --git a/jsonargparse_tests/test_provenance.py b/jsonargparse_tests/test_provenance.py index 0084e5c3..4891c5ed 100644 --- a/jsonargparse_tests/test_provenance.py +++ b/jsonargparse_tests/test_provenance.py @@ -5,6 +5,7 @@ import json import os import pickle +import shutil from pathlib import Path from typing import Optional from unittest.mock import patch @@ -19,6 +20,7 @@ get_parse_args_stderr, get_parse_args_stdout, get_parser_help, + is_posix, skip_if_no_pyyaml, ) @@ -560,6 +562,27 @@ def test_error_config_file_path_relative_to_working_directory(parser, tmp_cwd): assert error.endswith(f"\n Source: config file {Path('cfgs', 'cfg.json')}") +@pytest.mark.skipif(not is_posix, reason="the working directory can't be removed in windows") +def test_error_config_file_path_working_directory_removed(parser, tmp_cwd): + """The parsing error must not be masked by the working directory having been removed.""" + removed_dir = tmp_cwd / "removed" + removed_dir.mkdir() + Path("cfgs").mkdir() + Path("cfgs", "cfg.json").write_text('{"val": "abc"}') + + def scratch_int(value): + shutil.rmtree(removed_dir) + return int(value) + + parser.add_argument("--val", type=scratch_int, default=0) + os.chdir(removed_dir) + try: + error = get_error(parser, ["--config=" + str(tmp_cwd / "cfgs" / "cfg.json")]) + finally: + os.chdir(tmp_cwd) + assert error.endswith(f"\n Source: config file {tmp_cwd / 'cfgs' / 'cfg.json'}") + + def test_error_subconfig_file_path_relative_to_working_directory(parser, tmp_cwd): parser.add_argument("--model", type=Model, sub_configs=True) Path("cfgs").mkdir() diff --git a/sphinx/migrate_v5.rst b/sphinx/migrate_v5.rst index 2a2b61bf..d53a60e9 100644 --- a/sphinx/migrate_v5.rst +++ b/sphinx/migrate_v5.rst @@ -57,6 +57,12 @@ changes: ``None``; they remain required. Give such parameters an explicit default if optional is intended. Warns only with ``JSONARGPARSE_DEPRECATION_WARNINGS=all``. +- **The working directory is no longer changed while loading configs.** Gives no + deprecation warning. Relative paths in a config file are still resolved with + respect to its directory, but without calling ``os.chdir``. Code that relied + on the working directory, e.g. a custom type that opens a relative path, must + instead use a path type such as :class:`.Path_fr`, which gives the resolved + absolute path. - **Configs can no longer import and instantiate anything.** See `Subclass specs and import paths`_ below.