diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d0fa9b..741056f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- `PyccelCache` imports a new build once in a subprocess before it stores it: a build the + interpreter cannot import, or one that crashes it (an extension linked to `libpython` in a + Python that has it built in, such as uv's CPython 3.12 on macOS), is a `RuntimeError` and is + not stored, instead of a segmentation fault in every process that loads it. +- The `PyccelCache` key includes the NumPy version, so environments that share an interpreter + but not NumPy no longer share builds. + ### Added - scope-profiler regions around the coarse operations (`Kernel` calls as `cunumpy.kernel.`, CUDA and Pyccel compilation, `sort_by_key`, diff --git a/docs/src/content/docs/kernels/dispatch.md b/docs/src/content/docs/kernels/dispatch.md index 3eabdf0..4770592 100644 --- a/docs/src/content/docs/kernels/dispatch.md +++ b/docs/src/content/docs/kernels/dispatch.md @@ -257,8 +257,8 @@ inside `with xp.kernels.use_host_kernel_implementation("numpy"):` (or set `epyccel` compiles again on every call, in every process. `PyccelCache(name)` compiles a module once with the `pyccel compile` command and keeps the build -on disk, keyed on the module source, the OpenMP setting, the Pyccel and Python -versions and the platform, so only the first run after an edit compiles: +on disk, keyed on the module source, the OpenMP setting, the Pyccel, NumPy and +Python versions and the platform, so only the first run after an edit compiles: - builds go to `~/.cache//kernels`, or the directory in `_KERNEL_CACHE`; if that is not writable, to `/-kernels`; @@ -266,6 +266,10 @@ versions and the platform, so only the first run after an edit compiles: modules that call the OpenMP runtime; - builds are published with an atomic rename, so MPI ranks compiling the same module on a cold cache never load a partial build; +- a new build is imported once in a subprocess before it is published; if that + import fails or crashes the interpreter (an extension linked to `libpython` in + a Python that has it built in), `compile` raises instead and nothing is + stored, so the process that uses the cache does not crash; - the key covers the module's own source only: after editing a module the kernel imports, edit the kernel or clear the cache. diff --git a/src/cunumpy/_pyccel_cache.py b/src/cunumpy/_pyccel_cache.py index 4fded85..563c2b4 100644 --- a/src/cunumpy/_pyccel_cache.py +++ b/src/cunumpy/_pyccel_cache.py @@ -2,10 +2,12 @@ :class:`PyccelCache` builds a module with the ``pyccel compile`` command line interface, in a subprocess, once. It keys a build on the module source, the -target language, the OpenMP setting, the Pyccel and Python versions and the -platform, and stores it under a per-package cache directory. Later processes, -and other MPI ranks, import the stored extension directly; only the first run -after an edit compiles. +target language, the OpenMP setting, the Pyccel, NumPy and Python versions and +the platform, and stores it under a per-package cache directory. Later +processes, and other MPI ranks, import the stored extension directly; only the +first run after an edit compiles. A new build is imported once in a subprocess +before it is stored, so a build this interpreter cannot load is an error, not a +crash of the process that uses it. cunumpy needs no Pyccel to import this module: Pyccel is looked up when a module is compiled, and a missing Pyccel is an :class:`ImportError` there, which @@ -64,20 +66,77 @@ def _writable(directory: Path) -> bool: return os.access(directory, os.W_OK | os.X_OK) -def _load_extension(directory: Path, stem: str) -> ModuleType | None: +def _extension_path(directory: Path, stem: str) -> Path | None: for suffix in importlib.machinery.EXTENSION_SUFFIXES: path = directory / f"{stem}{suffix}" - if not path.is_file(): - continue + if path.is_file(): + return path + return None + + +def _load_extension(directory: Path, stem: str) -> ModuleType | None: + path = _extension_path(directory, stem) + if path is not None: spec = importlib.util.spec_from_file_location(stem, path) - if spec is None or spec.loader is None: - continue - extension = importlib.util.module_from_spec(spec) - spec.loader.exec_module(extension) - return extension + if spec is not None and spec.loader is not None: + extension = importlib.util.module_from_spec(spec) + spec.loader.exec_module(extension) + return extension return None +#: Imports the extension at ``argv[1]`` as module ``argv[2]``. +_IMPORT_CHECK = ( + "import importlib.util, sys\n" + "spec = importlib.util.spec_from_file_location(sys.argv[2], sys.argv[1])\n" + "spec.loader.exec_module(importlib.util.module_from_spec(spec))\n" +) + + +def _check_importable(directory: Path, stem: str) -> None: + """Import a new build in a subprocess; raise if this interpreter cannot. + + A build that crashes the interpreter on import (an extension linked to + ``libpython`` loaded into a Python with ``libpython`` built in, as with some + standalone CPython builds, runs two Pythons in one process) would otherwise + crash every process that loads it from the cache. + """ + path = _extension_path(directory, stem) + if path is None: + raise ImportError(f"pyccel compile wrote no extension module for {stem}") + result = subprocess.run( + [sys.executable, "-c", _IMPORT_CHECK, str(path), stem], + capture_output=True, + text=True, + check=False, + cwd=directory, + ) + if result.returncode == 0: + return + how = ( + f"crashed with signal {-result.returncode}" + if result.returncode < 0 + else f"failed ({result.returncode})" + ) + hint = ( + "\nAn extension linked to libpython crashes a Python that has libpython " + "built in; `otool -L` (macOS) or `ldd` shows what it links." + if result.returncode < 0 + else "" + ) + raise RuntimeError( + f"importing the new build of {stem} with {sys.executable} {how}; it is " + f"not stored:\n{result.stdout}{result.stderr}{hint}" + ) + + +def _numpy_version() -> str: + try: + return importlib.metadata.version("numpy") + except importlib.metadata.PackageNotFoundError: + return "none" + + def _pyccel_version() -> str: try: return importlib.metadata.version("pyccel") @@ -183,9 +242,10 @@ def _openmp_for(self, module: ModuleType) -> bool: def build_key(self, module: ModuleType, language: str | None = None) -> str: """Return the key of `module`'s build, which changes with anything that changes it. - The module's own source, the language, the OpenMP setting, the Pyccel - and Python versions and the platform. Modules the kernel imports are not - part of it: edit the kernel (or clear the cache) after changing one. + The module's own source, the language, the OpenMP setting, the Pyccel, + NumPy and Python versions and the platform. Modules the kernel imports + are not part of it: edit the kernel (or clear the cache) after changing + one. Parameters ---------- @@ -211,6 +271,7 @@ def build_key(self, module: ModuleType, language: str | None = None) -> str: _LAYOUT, f"openmp={self._openmp_for(module)}", _pyccel_version(), + f"numpy={_numpy_version()}", sys.version, platform.platform(), ): @@ -270,8 +331,9 @@ def compile(self, module: ModuleType, *, language: str | None = None) -> ModuleT ImportError If Pyccel is not installed. RuntimeError - With the output of ``pyccel compile`` if the build fails; callers - decide on a fallback. + With the output of ``pyccel compile`` if the build fails, or of + the import if this interpreter cannot import the new build (it is + then not stored); callers decide on a fallback. PermissionError If neither the cache nor the fallback directory is writable. """ @@ -293,6 +355,7 @@ def compile(self, module: ModuleType, *, language: str | None = None) -> ModuleT ).absolute() try: build = self._build(module, language, staging) + _check_importable(build, stem) try: # Atomic, so concurrent builders never see a partial directory. os.rename(build, target) diff --git a/tests/unit/test_pyccel_cache.py b/tests/unit/test_pyccel_cache.py index ce74dfb..8719d69 100644 --- a/tests/unit/test_pyccel_cache.py +++ b/tests/unit/test_pyccel_cache.py @@ -1,5 +1,6 @@ """PyccelCache: compile once, keep the build on disk, reuse it in later processes.""" +import importlib.machinery import importlib.metadata import importlib.util import os @@ -99,6 +100,51 @@ def test_key_changes_with_the_source_and_the_openmp_setting( assert cache.build_key(edited) != key +def test_key_changes_with_the_numpy_version(make_module, cache, monkeypatch): + # a build is compiled against one NumPy's C API + monkeypatch.setattr(_pyccel_cache, "_pyccel_version", lambda: "test") + module = make_module() + monkeypatch.setattr(_pyccel_cache, "_numpy_version", lambda: "2.4.0") + key = cache.build_key(module) + monkeypatch.setattr(_pyccel_cache, "_numpy_version", lambda: "2.5.0") + assert cache.build_key(module) != key + + +def _fake_build(content): + """A ``PyccelCache._build`` that writes `content` as the extension module.""" + + def build(self, module, language, staging): + output = staging / "build" + output.mkdir() + stem = module.__name__.rsplit(".", 1)[-1] + suffix = importlib.machinery.EXTENSION_SUFFIXES[0] + (output / f"{stem}{suffix}").write_bytes(content) + return output + + return build + + +def test_a_build_that_cannot_be_imported_is_not_stored(make_module, cache, monkeypatch): + # no compiler needed: the "build" is a file no interpreter can import + monkeypatch.setattr(_pyccel_cache, "_pyccel_version", lambda: "test") + monkeypatch.setattr(xp.kernels.PyccelCache, "_build", _fake_build(b"not a library")) + with pytest.raises(RuntimeError, match="not stored"): + cache.compile(make_module()) + assert not list(cache.cache_root().iterdir()) + + +def test_a_build_that_crashes_the_interpreter_names_the_signal( + make_module, cache, monkeypatch +): + monkeypatch.setattr(_pyccel_cache, "_pyccel_version", lambda: "test") + monkeypatch.setattr(xp.kernels.PyccelCache, "_build", _fake_build(b"")) + crashed = SimpleNamespace(returncode=-11, stdout="", stderr="Segmentation fault") + monkeypatch.setattr(_pyccel_cache.subprocess, "run", lambda *a, **k: crashed) + with pytest.raises(RuntimeError, match="signal 11(.|\n)*libpython"): + cache.compile(make_module()) + assert not list(cache.cache_root().iterdir()) + + def test_a_module_calling_the_openmp_runtime_keeps_openmp( make_module, cache, monkeypatch ):