From be2d9e5d423cced4d4fc391b7086647379ed05e7 Mon Sep 17 00:00:00 2001 From: Mauricio Villegas <5780272+mauvilsa@users.noreply.github.com> Date: Thu, 17 Sep 2026 06:20:19 +0200 Subject: [PATCH 1/2] Relative paths resolved without changing the working directory --- CHANGELOG.rst | 12 ++ jsonargparse/_actions.py | 4 +- jsonargparse/_core.py | 8 +- jsonargparse/_formatters.py | 4 +- jsonargparse/_from_config.py | 6 +- jsonargparse/_loaders_dumpers.py | 6 + jsonargparse/_paths.py | 50 ++++---- jsonargparse/_typehints.py | 10 +- jsonargparse/typing.py | 5 +- jsonargparse_tests/test_paths.py | 160 ++++++++++++++++++++++++++ jsonargparse_tests/test_subclasses.py | 30 +++++ sphinx/migrate_v5.rst | 7 ++ 12 files changed, 258 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6a047044..5c434638 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -46,6 +46,13 @@ 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 + `__). Changed ^^^^^^^ @@ -88,6 +95,11 @@ 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. The directory of a config file being loaded is added to + ``sys.path``, so that modules next to it can be imported, e.g. to resolve a + ``class_path`` (`#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..cc20c7df 100644 --- a/jsonargparse/_formatters.py +++ b/jsonargparse/_formatters.py @@ -362,12 +362,12 @@ 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()) + cwd = pathlib.Path.cwd() 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..fbe25b7c 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 + path = os.path.join(path_dir, 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..89bc2465 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,42 @@ 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. For local directories, + the directory is prepended to ``sys.path``, such that modules next to a config + file can be imported, e.g. to resolve a ``class_path``. + """ + 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) + sys_path_dir = None + if is_local and path_dir: + path_dir = local_dir = os.path.abspath(path_dir) + if path_dir not in sys.path: + sys.path.insert(0, path_dir) + sys_path_dir = 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() + if sys_path_dir: + sys.path.remove(sys_path_dir) 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_paths.py b/jsonargparse_tests/test_paths.py index 333ea7c8..8ece8121 100644 --- a/jsonargparse_tests/test_paths.py +++ b/jsonargparse_tests/test_paths.py @@ -1,10 +1,14 @@ from __future__ import annotations import dataclasses +import importlib import json import os import pathlib +import shutil import stat +import sys +import threading import zipfile from io import StringIO from typing import Any, Callable, Dict, List, Optional, Union @@ -13,6 +17,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 +588,122 @@ 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") + + +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} + + +def test_relative_path_context_sys_path(tmp_cwd): + subdir = tmp_cwd / "sub" + subdir.mkdir() + (subdir / "sidecar_module.py").write_text("value = 3\n") + + assert str(subdir) not in sys.path + try: + with Path_drw(subdir).relative_path_context(): + assert sys.path[0] == str(subdir) + assert importlib.import_module("sidecar_module").value == 3 + finally: + sys.modules.pop("sidecar_module", None) + assert str(subdir) not in sys.path + + +def test_relative_path_context_sys_path_already_present(tmp_cwd): + subdir = tmp_cwd / "sub" + subdir.mkdir() + + sys.path.insert(0, str(subdir)) + try: + with Path_drw(subdir).relative_path_context(): + assert sys.path.count(str(subdir)) == 1 + assert sys.path.count(str(subdir)) == 1 + finally: + sys.path.remove(str(subdir)) + + +@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") + + +@skip_if_requests_unavailable +def test_relative_path_context_url_not_in_sys_path(): + num_sys_path = len(sys.path) + with Path("http://example.com/nested/path/file.txt", mode="u").relative_path_context(): + assert len(sys.path) == num_sys_path + + # path types tests @@ -666,6 +787,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_subclasses.py b/jsonargparse_tests/test_subclasses.py index 2c820208..c8a23e47 100644 --- a/jsonargparse_tests/test_subclasses.py +++ b/jsonargparse_tests/test_subclasses.py @@ -2,6 +2,7 @@ import json import os +import sys import textwrap import warnings from abc import ABC, abstractmethod @@ -2587,6 +2588,35 @@ def test_subclass_multifile_save(parser, tmp_cwd): assert obj == {"class_path": f"{__name__}.BaseC", "init_args": {"p": 0}} +def test_subclass_class_path_module_next_to_config(parser, tmp_cwd): + parser.add_subclass_arguments(Calendar, "cal") + + subdir = Path("sub") + subdir.mkdir() + (subdir / "sidecar_calendar.py").write_text( + textwrap.dedent( + """ + from calendar import Calendar + + class SidecarCalendar(Calendar): + def __init__(self, firstweekday: int = 3): + super().__init__(firstweekday) + """ + ) + ) + config_path = subdir / "config.yaml" + config_path.write_text(json_or_yaml_dump({"class_path": "sidecar_calendar.SidecarCalendar"})) + + try: + cfg = parser.parse_args([f"--cal={config_path}"]) + assert cfg.cal.class_path == "sidecar_calendar.SidecarCalendar" + assert cfg.cal.init_args == Namespace(firstweekday=3) + init = parser.instantiate(cfg) + assert init.cal.firstweekday == 3 + finally: + sys.modules.pop("sidecar_calendar", None) + + # failure cases tests diff --git a/sphinx/migrate_v5.rst b/sphinx/migrate_v5.rst index 2a2b61bf..18e5e515 100644 --- a/sphinx/migrate_v5.rst +++ b/sphinx/migrate_v5.rst @@ -57,6 +57,13 @@ 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. A ``class_path`` naming a module next to the config file keeps + working, since that directory is added to ``sys.path``. - **Configs can no longer import and instantiate anything.** See `Subclass specs and import paths`_ below. From d9594b4764c7c3c316d04d0f4a9599da9ed52f4a Mon Sep 17 00:00:00 2001 From: Mauricio Villegas <5780272+mauvilsa@users.noreply.github.com> Date: Thu, 17 Sep 2026 07:36:43 +0200 Subject: [PATCH 2/2] Address review comments - Remove the sys.path modification from path_dir_context. It races through process-global state, and a class_path naming a module next to the config file was never a documented feature. - Don't mask a parse error with a FileNotFoundError when the working directory has been removed, by falling back to the absolute path in _describe_origin. - Anchor the jsonnet snippet name on the base name, so that from_config resolves imports for a config given as a relative path with a directory. - Skip test_relative_path_context_cwd_removed in windows, where the working directory can't be removed. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.rst | 10 ++++--- jsonargparse/_formatters.py | 5 +++- jsonargparse/_loaders_dumpers.py | 4 +-- jsonargparse/_paths.py | 10 +------ jsonargparse_tests/test_jsonnet.py | 18 +++++++++++++ jsonargparse_tests/test_paths.py | 38 +-------------------------- jsonargparse_tests/test_provenance.py | 23 ++++++++++++++++ jsonargparse_tests/test_subclasses.py | 30 --------------------- sphinx/migrate_v5.rst | 3 +-- 9 files changed, 57 insertions(+), 84 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 5c434638..ff7d2e8e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -53,6 +53,12 @@ Fixed - 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 ^^^^^^^ @@ -96,9 +102,7 @@ Changed 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. The directory of a config file being loaded is added to - ``sys.path``, so that modules next to it can be imported, e.g. to resolve a - ``class_path`` (`#979 + working directory (`#979 `__). Removed diff --git a/jsonargparse/_formatters.py b/jsonargparse/_formatters.py index cc20c7df..087479af 100644 --- a/jsonargparse/_formatters.py +++ b/jsonargparse/_formatters.py @@ -367,7 +367,10 @@ def _describe_origin(origin) -> str: if not isinstance(origin, Path) or origin.is_url or origin.is_fsspec: return str(origin) absolute = pathlib.Path(origin.absolute) - cwd = pathlib.Path.cwd() + 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/_loaders_dumpers.py b/jsonargparse/_loaders_dumpers.py index fbe25b7c..4d9b3c66 100644 --- a/jsonargparse/_loaders_dumpers.py +++ b/jsonargparse/_loaders_dumpers.py @@ -126,8 +126,8 @@ def jsonnet_load(stream, path="", ext_vars=None): _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 - path = os.path.join(path_dir, path or "snippet") + # 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 89bc2465..72a93f8a 100644 --- a/jsonargparse/_paths.py +++ b/jsonargparse/_paths.py @@ -388,9 +388,7 @@ 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. For local directories, - the directory is prepended to ``sys.path``, such that modules next to a config - file can be imported, e.g. to resolve a ``class_path``. + removal of the original directory are not a problem. """ local_dir = current_local_dir.get() path_dir = _current_path_dir.get() @@ -407,12 +405,8 @@ def path_dir_context(path: Path | None) -> Iterator[str | None]: path_dir = os.path.dirname(path_dir) path_dir = scheme + path_dir - sys_path_dir = None if is_local and path_dir: path_dir = local_dir = os.path.abspath(path_dir) - if path_dir not in sys.path: - sys.path.insert(0, path_dir) - sys_path_dir = path_dir token = _current_path_dir.set(path_dir) local_token = current_local_dir.set(local_dir) @@ -421,5 +415,3 @@ def path_dir_context(path: Path | None) -> Iterator[str | None]: finally: current_local_dir.reset(local_token) _current_path_dir.reset(token) - if sys_path_dir: - sys.path.remove(sys_path_dir) 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 8ece8121..772cd771 100644 --- a/jsonargparse_tests/test_paths.py +++ b/jsonargparse_tests/test_paths.py @@ -1,13 +1,11 @@ from __future__ import annotations import dataclasses -import importlib import json import os import pathlib import shutil import stat -import sys import threading import zipfile from io import StringIO @@ -606,6 +604,7 @@ def test_relative_path_context_keeps_symlinked_dir(tmp_cwd): 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() @@ -650,34 +649,6 @@ def resolve(subdir): assert resolved == {s.name: str(s / "file.txt") for s in subdirs} -def test_relative_path_context_sys_path(tmp_cwd): - subdir = tmp_cwd / "sub" - subdir.mkdir() - (subdir / "sidecar_module.py").write_text("value = 3\n") - - assert str(subdir) not in sys.path - try: - with Path_drw(subdir).relative_path_context(): - assert sys.path[0] == str(subdir) - assert importlib.import_module("sidecar_module").value == 3 - finally: - sys.modules.pop("sidecar_module", None) - assert str(subdir) not in sys.path - - -def test_relative_path_context_sys_path_already_present(tmp_cwd): - subdir = tmp_cwd / "sub" - subdir.mkdir() - - sys.path.insert(0, str(subdir)) - try: - with Path_drw(subdir).relative_path_context(): - assert sys.path.count(str(subdir)) == 1 - assert sys.path.count(str(subdir)) == 1 - finally: - sys.path.remove(str(subdir)) - - @skip_if_fsspec_unavailable @patch_parsing_settings def test_relative_path_context_local_dir_kept_inside_remote(tmp_cwd): @@ -697,13 +668,6 @@ def test_relative_path_context_local_dir_kept_inside_remote(tmp_cwd): assert path.absolute == str(subdir / "file.txt") -@skip_if_requests_unavailable -def test_relative_path_context_url_not_in_sys_path(): - num_sys_path = len(sys.path) - with Path("http://example.com/nested/path/file.txt", mode="u").relative_path_context(): - assert len(sys.path) == num_sys_path - - # path types tests 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/jsonargparse_tests/test_subclasses.py b/jsonargparse_tests/test_subclasses.py index c8a23e47..2c820208 100644 --- a/jsonargparse_tests/test_subclasses.py +++ b/jsonargparse_tests/test_subclasses.py @@ -2,7 +2,6 @@ import json import os -import sys import textwrap import warnings from abc import ABC, abstractmethod @@ -2588,35 +2587,6 @@ def test_subclass_multifile_save(parser, tmp_cwd): assert obj == {"class_path": f"{__name__}.BaseC", "init_args": {"p": 0}} -def test_subclass_class_path_module_next_to_config(parser, tmp_cwd): - parser.add_subclass_arguments(Calendar, "cal") - - subdir = Path("sub") - subdir.mkdir() - (subdir / "sidecar_calendar.py").write_text( - textwrap.dedent( - """ - from calendar import Calendar - - class SidecarCalendar(Calendar): - def __init__(self, firstweekday: int = 3): - super().__init__(firstweekday) - """ - ) - ) - config_path = subdir / "config.yaml" - config_path.write_text(json_or_yaml_dump({"class_path": "sidecar_calendar.SidecarCalendar"})) - - try: - cfg = parser.parse_args([f"--cal={config_path}"]) - assert cfg.cal.class_path == "sidecar_calendar.SidecarCalendar" - assert cfg.cal.init_args == Namespace(firstweekday=3) - init = parser.instantiate(cfg) - assert init.cal.firstweekday == 3 - finally: - sys.modules.pop("sidecar_calendar", None) - - # failure cases tests diff --git a/sphinx/migrate_v5.rst b/sphinx/migrate_v5.rst index 18e5e515..d53a60e9 100644 --- a/sphinx/migrate_v5.rst +++ b/sphinx/migrate_v5.rst @@ -62,8 +62,7 @@ changes: 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. A ``class_path`` naming a module next to the config file keeps - working, since that directory is added to ``sys.path``. + absolute path. - **Configs can no longer import and instantiate anything.** See `Subclass specs and import paths`_ below.