diff --git a/python/siac/adapters/atmo/cams.py b/python/siac/adapters/atmo/cams.py index 282763de..3d3f95dc 100644 --- a/python/siac/adapters/atmo/cams.py +++ b/python/siac/adapters/atmo/cams.py @@ -1183,7 +1183,9 @@ def _download_cams_file(self, obs_time: datetime) -> Path | None: """Download CAMS data using the official CDS API request pattern.""" try: cdsapi = self._import_cdsapi() - except ImportError: + except ImportError as exc: + if not isinstance(exc, ModuleNotFoundError) or exc.name != "cdsapi": + raise ImportError("cdsapi is installed but failed to import.") from exc logger.warning("cdsapi is not installed; cannot auto-download CAMS data") return None diff --git a/python/siac/adapters/auth.py b/python/siac/adapters/auth.py index eba8fa9c..c4c1b850 100644 --- a/python/siac/adapters/auth.py +++ b/python/siac/adapters/auth.py @@ -95,6 +95,8 @@ def _load_optional_dependency(module_name: str, install_hint: str) -> Any: try: return import_module(module_name) except ModuleNotFoundError as exc: # pragma: no cover - env-dependent + if exc.name != module_name: + raise raise AuthenticationError(install_hint) from exc diff --git a/python/siac/adapters/data/earthaccess_source.py b/python/siac/adapters/data/earthaccess_source.py index 84e04821..72ff1f64 100644 --- a/python/siac/adapters/data/earthaccess_source.py +++ b/python/siac/adapters/data/earthaccess_source.py @@ -58,7 +58,9 @@ def _get_module(self) -> Any: if self._earthaccess is None: try: self._earthaccess = importlib.import_module("earthaccess") - except ImportError as exc: # pragma: no cover - env-dependent + except ImportError as exc: + if not isinstance(exc, ModuleNotFoundError) or exc.name != "earthaccess": + raise ImportError("earthaccess is installed but failed to import.") from exc raise ImportError( "earthaccess is required for NASA Earthdata access. " "Install with `pip install earthaccess`." diff --git a/python/siac/adapters/earthdata_common.py b/python/siac/adapters/earthdata_common.py index 5d173fb1..0a5b230b 100644 --- a/python/siac/adapters/earthdata_common.py +++ b/python/siac/adapters/earthdata_common.py @@ -16,7 +16,9 @@ try: from pyhdf.SD import SD, SDC # type: ignore[import-untyped] -except ModuleNotFoundError: # pragma: no cover - optional dependency +except ImportError as exc: + if not isinstance(exc, ModuleNotFoundError) or exc.name != "pyhdf": + raise ImportError("pyhdf is installed but failed to import.") from exc class _MissingSDC: READ = 0 diff --git a/python/siac/algorithms/cloud/providers/omnicloudmask.py b/python/siac/algorithms/cloud/providers/omnicloudmask.py index 424bbb29..005e040a 100644 --- a/python/siac/algorithms/cloud/providers/omnicloudmask.py +++ b/python/siac/algorithms/cloud/providers/omnicloudmask.py @@ -81,11 +81,8 @@ def _default_predictor( try: omnicloudmask = import_module("omnicloudmask") except ImportError as exc: - # Narrowed from ``except Exception`` (REVIEW.md ยง2.1): - # only swallow import-time failures here. Other exception - # classes (a partial install raising AttributeError during - # module init, for example) should propagate so the user can - # see the real cause. + if not isinstance(exc, ModuleNotFoundError) or exc.name != "omnicloudmask": + raise ImportError("omnicloudmask is installed but failed to import.") from exc raise ImportError( "omnicloudmask is required for SIAC cloud masking. " "Install the 'omnicloudmask' package." diff --git a/python/siac/algorithms/rt/lut/http_zip_store.py b/python/siac/algorithms/rt/lut/http_zip_store.py index 3a818670..eb5697ed 100644 --- a/python/siac/algorithms/rt/lut/http_zip_store.py +++ b/python/siac/algorithms/rt/lut/http_zip_store.py @@ -557,6 +557,8 @@ def _build_s3_filesystem(storage_options: dict[str, Any]) -> AsyncFileSystem: try: import s3fs # type: ignore[import-untyped] except ImportError as exc: + if not isinstance(exc, ModuleNotFoundError) or exc.name != "s3fs": + raise ImportError("s3fs is installed but failed to import.") from exc raise ImportError( "s3fs is required to read s3:// LUT zip paths. Install with `pip install s3fs`." ) from exc diff --git a/python/siac/app/_assembly_providers.py b/python/siac/app/_assembly_providers.py index c73815bf..16fcad5c 100644 --- a/python/siac/app/_assembly_providers.py +++ b/python/siac/app/_assembly_providers.py @@ -8,6 +8,7 @@ import numpy as np from siac.adapters.atmo import CAMSProvider +from siac.adapters.auth import CredentialManager from siac.adapters.earthdata import earthaccess_source_from_auth from siac.app.registry import ( ATMO_PROVIDER_REGISTRY, @@ -18,7 +19,6 @@ if TYPE_CHECKING: from collections.abc import Callable - from siac.adapters.auth import CredentialManager from siac.algorithms.surface.brdf_monthly_composite import MonthlyCompositeCollection from siac.domain.protocols import AtmosphericPriorProvider, MonthlyCompositeProvider from siac.runtime import ObservationBundle @@ -47,7 +47,7 @@ def _build_cams_provider( atmo_config.data_path, temporal_interp=atmo_config.temporal_interpolation == "linear", download_missing=atmo_config.download_missing, - auth=auth, + auth=auth if auth is not None else CredentialManager.from_config(config), cache_dir=atmo_config.cache_dir, ), ) diff --git a/python/siac/app/_assembly_surface.py b/python/siac/app/_assembly_surface.py index 9a74df61..eb00ea2a 100644 --- a/python/siac/app/_assembly_surface.py +++ b/python/siac/app/_assembly_surface.py @@ -10,6 +10,7 @@ import numpy as np from siac.adapters.atmo import CAMSProvider +from siac.adapters.auth import CredentialManager from siac.algorithms.surface.brdf_whittaker import BRDFWhittakerDeriver from siac.algorithms.surface.kernel_model import KernelModelDeriver from siac.app._assembly_providers import ( @@ -20,7 +21,6 @@ from siac.observability import current_execution_observer, observe_stage if TYPE_CHECKING: - from siac.adapters.auth import CredentialManager from siac.domain.sensors import SensorConfig from siac.runtime import AtmosphericState, ObservationBundle, SurfacePrior from siac.workflows.pipeline import SurfacePriorFn @@ -730,7 +730,7 @@ def _resolve_bestpixel_anchor_atmo_source( data_path, temporal_interp=False, download_missing=True, - auth=auth, + auth=auth if auth is not None else CredentialManager.from_config(config), cache_dir=cache_dir, ) return provider.get_prior diff --git a/tests/unit/test_cams_credentials.py b/tests/unit/test_cams_credentials.py new file mode 100644 index 00000000..98af664f --- /dev/null +++ b/tests/unit/test_cams_credentials.py @@ -0,0 +1,77 @@ +"""Configured CAMS credentials reach cdsapi through provider assembly.""" + +from __future__ import annotations + +import sys +from datetime import datetime +from types import SimpleNamespace +from typing import TYPE_CHECKING + +import pytest + +from siac.adapters.auth import CredentialManager +from siac.app._assembly_providers import _build_cams_provider +from siac.app._assembly_surface import _resolve_bestpixel_anchor_atmo_source +from siac.config import SIACConfig + +if TYPE_CHECKING: + from pathlib import Path + + +@pytest.mark.parametrize("factory", ["registered", "anchor"]) +@pytest.mark.parametrize("credentials", ["config", "manager", "defaults"]) +def test_cams_assembly_passes_credentials_to_cdsapi( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + factory: str, + credentials: str, +) -> None: + config = SIACConfig() + if credentials != "defaults": + config.auth.cds.api_key = "configured-test-key" + auth = None + if credentials == "manager": + auth = CredentialManager() + auth.set_credentials("cds", key="manager-test-key") + + captured: dict[str, object] = {} + + class FakeClient: + def __init__(self, **kwargs): + captured["kwargs"] = kwargs + + def retrieve(self, dataset, request): + return self + + def download(self, path): + captured["path"] = path + + monkeypatch.setitem(sys.modules, "cdsapi", SimpleNamespace(Client=FakeClient)) + if factory == "registered": + config.providers.atmo.data_path = str(tmp_path) + provider = _build_cams_provider(config, auth) + else: + anchor_config = SimpleNamespace( + auth=config.auth, + paths=SimpleNamespace(cache_root=tmp_path), + algorithms=SimpleNamespace( + surface_prior=SimpleNamespace( + bestpixel_predict_visible=True, + bestpixel_predict_visible_anchor_source="cams", + bestpixel_predict_visible_cams_data_path=str(tmp_path), + ) + ), + ) + prior = _resolve_bestpixel_anchor_atmo_source(anchor_config, auth) + provider = prior.__self__ + + output = provider._download_cams_file(datetime(2024, 1, 2)) + expected = { + "config": {"key": "configured-test-key"}, + "manager": {"key": "manager-test-key"}, + "defaults": {}, + } + assert captured["kwargs"] == expected[credentials] + assert captured["path"] == str(output) + if auth is not None: + assert provider._auth is auth diff --git a/tests/unit/test_optional_import_errors.py b/tests/unit/test_optional_import_errors.py new file mode 100644 index 00000000..c00afbf6 --- /dev/null +++ b/tests/unit/test_optional_import_errors.py @@ -0,0 +1,106 @@ +"""Optional imports distinguish missing packages from broken installations.""" + +from __future__ import annotations + +import builtins +import importlib +import runpy +from datetime import datetime +from typing import TYPE_CHECKING + +import pytest + +from siac.adapters import auth, earthdata_common +from siac.adapters.atmo.cams import CAMSProvider +from siac.adapters.data.earthaccess_source import EarthAccessSource +from siac.algorithms.cloud.providers.omnicloudmask import OmniCloudMaskProvider +from siac.algorithms.rt.lut.http_zip_store import _build_s3_filesystem +from siac.errors import AuthenticationError + +if TYPE_CHECKING: + from pathlib import Path + + +def _import_optional(package: str, tmp_path: Path): + if package == "earthaccess": + return EarthAccessSource()._get_module() + if package == "omnicloudmask": + return OmniCloudMaskProvider._default_predictor(None) + if package == "s3fs": + return _build_s3_filesystem({}) + if package == "cdsapi": + return CAMSProvider(tmp_path)._download_cams_file(datetime(2024, 1, 2)) + return runpy.run_path(earthdata_common.__file__) + + +def _fail_import(monkeypatch: pytest.MonkeyPatch, package: str, error: ImportError) -> None: + original_import = builtins.__import__ + original_import_module = importlib.import_module + + def import_package(name, *args, **kwargs): + if name.split(".")[0] == package: + raise error + return original_import(name, *args, **kwargs) + + def import_module(name, *args, **kwargs): + if name == package: + raise error + return original_import_module(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", import_package) + monkeypatch.setattr(importlib, "import_module", import_module) + monkeypatch.setattr(auth, "import_module", import_module) + monkeypatch.setattr("siac.adapters.atmo.cams.import_module", import_module) + monkeypatch.setattr( + "siac.algorithms.cloud.providers.omnicloudmask.import_module", import_module + ) + + +@pytest.mark.parametrize("package", ["earthaccess", "omnicloudmask", "s3fs", "cdsapi", "pyhdf"]) +@pytest.mark.parametrize("failure", ["transitive_missing", "incompatible"]) +def test_optional_installed_package_import_failure( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, package: str, failure: str +) -> None: + error = ( + ModuleNotFoundError("No module named dependency", name="dependency") + if failure == "transitive_missing" + else ImportError("cannot import name from dependency") + ) + _fail_import(monkeypatch, package, error) + with pytest.raises(ImportError, match=f"{package} is installed but failed to import") as raised: + _import_optional(package, tmp_path) + assert raised.value.__cause__ is error + + +@pytest.mark.parametrize("package", ["earthaccess", "omnicloudmask", "s3fs", "cdsapi", "pyhdf"]) +def test_optional_missing_package_keeps_install_hint( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, package: str +) -> None: + error = ModuleNotFoundError(f"No module named {package}", name=package) + _fail_import(monkeypatch, package, error) + if package == "cdsapi": + assert _import_optional(package, tmp_path) is None + elif package == "pyhdf": + module = _import_optional(package, tmp_path) + with pytest.raises(ModuleNotFoundError, match="pyhdf is required"): + module["SD"]("unused.hdf") + else: + with pytest.raises(ImportError, match=f"{package} is required") as raised: + _import_optional(package, tmp_path) + assert raised.value.__cause__ is error + + +def test_auth_optional_import_preserves_missing_dependency(monkeypatch: pytest.MonkeyPatch) -> None: + error = ModuleNotFoundError("No module named dependency", name="dependency") + _fail_import(monkeypatch, "cdsapi", error) + with pytest.raises(ModuleNotFoundError) as raised: + auth._load_optional_dependency("cdsapi", "cdsapi is not installed.") + assert raised.value is error + + +def test_auth_optional_import_keeps_missing_package_hint(monkeypatch: pytest.MonkeyPatch) -> None: + error = ModuleNotFoundError("No module named cdsapi", name="cdsapi") + _fail_import(monkeypatch, "cdsapi", error) + with pytest.raises(AuthenticationError, match="cdsapi is not installed") as raised: + auth._load_optional_dependency("cdsapi", "cdsapi is not installed.") + assert raised.value.__cause__ is error