Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion dascore/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,20 @@ class InvalidIndexVersionError(InvalidIndexError):


class MissingOptionalDependencyError(ImportError, DependencyError):
"""Raised when an optional package needed for some functionality is missing."""
"""
Raised when an optional package needed for some functionality is missing.

The install_name attribute, when set, gives the name of the package to
install (eg protobuf) which may differ from the import name
(eg google.protobuf). It defaults on the class so subclasses which don't
call this init still have it.
"""

install_name: str | None = None

def __init__(self, *args, install_name: str | None = None, **kwargs):
super().__init__(*args, **kwargs)
self.install_name = install_name


class DASVaderCompatibilityError(InvalidFiberFileError, DependencyError):
Expand Down
31 changes: 29 additions & 2 deletions dascore/io/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from __future__ import annotations

import inspect
import re
import warnings
from collections import defaultdict
from collections.abc import (
Expand Down Expand Up @@ -61,6 +62,8 @@
from dascore.utils.io import IOResourceManager, get_handle_from_resource
from dascore.utils.mapping import FrozenDict
from dascore.utils.misc import (
_get_install_message,
_get_install_name,
_iter_filesystem,
_locked,
_reinit_after_fork,
Expand Down Expand Up @@ -1298,6 +1301,23 @@ def _count_generator(generator):
return entity_count


_MISSING_MODULE_PATTERN = re.compile(r"^(\S+) is not installed")


def _get_missing_install_name(exception: MissingOptionalDependencyError) -> str:
"""Get the installable package name from a missing dependency error."""
if exception.install_name:
return exception.install_name
# Errors raised outside of optional_import (eg by a third party FiberIO)
# identify the module, if at all, with the module name or the message form
# optional_import used to use. Any other message could say anything, so
# nothing is recommended for installation.
if not (name := exception.name or ""):
match = _MISSING_MODULE_PATTERN.match(exception.msg or "")
name = match.group(1) if match else ""
return _get_install_name(name)


def _handle_missing_optionals(output_count, optional_dep_dict):
"""
Inform the user there are files that can be read but the proper
Expand All @@ -1306,10 +1326,17 @@ def _handle_missing_optionals(output_count, optional_dep_dict):
If there are other readable files that were found, raise a warning.
Otherwise, raise a MissingOptionalDependencyError.
"""
counts = ", ".join(
f"{name or 'unknown'} ({count} files)"
for name, count in sorted(optional_dep_dict.items())
)
# Unidentifiable packages can't be included in an install command.
packages = [x for x in optional_dep_dict if x]
install = f" {_get_install_message(packages)}" if packages else ""
msg = (
f"DASCore found files that can be read if additional packages are "
f"installed. The needed packages and the found number of files are: "
f"{dict(optional_dep_dict)}"
f"{counts}.{install}"
)
warn_or_raise(
msg,
Expand Down Expand Up @@ -1416,7 +1443,7 @@ def _iter_scan_results(
scan_kwargs["snap"] = snap
source = fiber_io.scan(resource, **scan_kwargs)
except MissingOptionalDependencyError as ex:
missing_optional_deps[ex.msg.split(" ")[0]] += 1
missing_optional_deps[_get_missing_install_name(ex)] += 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find directory FiberIO implementations and inspect their scan methods.
ast-grep outline dascore/io --items all --type class,function
rg -n -P -C 8 --glob '*.py' 'input_type\s*=\s*["'\'']directory["'\'']|def\s+scan\s*\(' dascore/io

# Find directory scan paths that use optional imports or raise missing-dependency errors.
rg -n -P -C 6 --glob '*.py' '\boptional_import\s*\(|\bMissingOptionalDependencyError\b' dascore/io

Repository: DASDAE/dascore

Length of output: 50370


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- scan dispatch ---'
cat -n dascore/io/core.py | sed -n '1385,1460p'

printf '%s\n' '--- directory FiberIO classes ---'
python3 - <<'PY'
import ast
from pathlib import Path

for path in Path("dascore/io").rglob("*.py"):
    try:
        tree = ast.parse(path.read_text())
    except Exception:
        continue
    for node in ast.walk(tree):
        if not isinstance(node, ast.ClassDef):
            continue
        input_type = None
        bases = []
        for base in node.bases:
            if isinstance(base, ast.Name):
                bases.append(base.id)
            elif isinstance(base, ast.Attribute):
                bases.append(base.attr)
        for stmt in node.body:
            if isinstance(stmt, ast.Assign):
                for target in stmt.targets:
                    if isinstance(target, ast.Name) and target.id == "input_type":
                        if isinstance(stmt.value, ast.Constant):
                            input_type = stmt.value.value
        if input_type == "directory":
            scan = next(
                (x for x in node.body if isinstance(x, (ast.FunctionDef, ast.AsyncFunctionDef))
                 and x.name == "scan"),
                None,
            )
            print(path, node.name, "bases=", bases,
                  "scan_lines=", (scan.lineno, scan.end_lineno) if scan else None)
PY

printf '%s\n' '--- XMLBinary imports and scan helpers ---'
rg -n -C 8 --glob '*.py' \
  'optional_import|MissingOptionalDependencyError|def (_read_xml_metadata|_paths_to_scan_patches|scan)' \
  dascore/io/xml_binary dascore/core dascore/utils.py dascore/exceptions.py 2>/dev/null || true

printf '%s\n' '--- all directory declarations and exception raises ---'
rg -n -C 4 --glob '*.py' \
  'input_type\s*=\s*["'\'']directory["'\'']|raise\s+MissingOptionalDependencyError|MissingOptionalDependencyError\s*=' \
  dascore

Repository: DASDAE/dascore

Length of output: 13842


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- scan dispatch ---'
cat -n dascore/io/core.py | sed -n '1385,1460p'

printf '%s\n' '--- directory FiberIO classes ---'
python3 - <<'PY'
import ast
from pathlib import Path

for path in Path("dascore/io").rglob("*.py"):
    tree = ast.parse(path.read_text())
    for node in ast.walk(tree):
        if not isinstance(node, ast.ClassDef):
            continue
        input_type = None
        bases = []
        for base in node.bases:
            bases.append(base.id if isinstance(base, ast.Name) else getattr(base, "attr", None))
        for stmt in node.body:
            if isinstance(stmt, ast.Assign):
                for target in stmt.targets:
                    if isinstance(target, ast.Name) and target.id == "input_type":
                        if isinstance(stmt.value, ast.Constant):
                            input_type = stmt.value.value
        if input_type == "directory":
            scan = next(
                (x for x in node.body if isinstance(x, ast.FunctionDef) and x.name == "scan"),
                None,
            )
            print(path, node.name, bases,
                  (scan.lineno, scan.end_lineno) if scan else None)
PY

printf '%s\n' '--- XMLBinary dependency paths ---'
rg -n -C 8 --glob '*.py' \
  'optional_import|MissingOptionalDependencyError|def (_read_xml_metadata|_paths_to_scan_patches|scan)' \
  dascore/io/xml_binary dascore 2>/dev/null | head -300

printf '%s\n' '--- directory declarations and dependency exceptions ---'
rg -n -C 4 --glob '*.py' \
  'input_type\s*=\s*["'\'']directory["'\'']|raise\s+MissingOptionalDependencyError|MissingOptionalDependencyError\s*=' \
  dascore

Repository: DASDAE/dascore

Length of output: 23056


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- XMLBinary modules ---'
cat -n dascore/io/xml_binary/core.py | sed -n '1,90p'
cat -n dascore/io/xml_binary/utils.py | sed -n '1,270p'

printf '%s\n' '--- XML conversion definition and imports ---'
rg -n -C 8 --glob '*.py' \
  'xml_to_dict|def\s+xml_to_dict|from .*xml|import .*xml|optional_import' \
  dascore

printf '%s\n' '--- FiberIO registry and directory resolution ---'
rg -n -C 8 --glob '*.py' \
  'register|FIBER|fiber_io|input_type|is_directory_format|_get_fiber_io_and_req_type' \
  dascore/io/__init__.py dascore/io/core.py dascore 2>/dev/null | head -500

Repository: DASDAE/dascore

Length of output: 50370


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- io package registration ---'
wc -l dascore/io/__init__.py
cat -n dascore/io/__init__.py | sed -n '1,240p'

printf '%s\n' '--- manager and FiberIO lookup definitions ---'
ast-grep outline dascore/io/core.py --items all --type class,function | sed -n '1,180p'
rg -n -C 12 \
  'class _FiberIOManager|class FiberIO|def _get_fiber_io_and_req_type|fiber_io_classes|FIBER_IO|entry_points|importlib.metadata|register' \
  dascore/io/core.py dascore/io/__init__.py

Repository: DASDAE/dascore

Length of output: 18299


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
import ast
from pathlib import Path

tree = ast.parse(Path("dascore/io/core.py").read_text())

iter_scan = next(
    node for node in ast.walk(tree)
    if isinstance(node, ast.FunctionDef) and node.name == "_iter_scan_results"
)

directory_if = next(
    node for node in ast.walk(iter_scan)
    if isinstance(node, ast.If)
    and isinstance(node.test, ast.Compare)
    and any(
        isinstance(part, ast.Constant) and part.value == "directory"
        for part in [node.test.left, *node.test.comparators]
    )
)
directory_scan = next(
    node for node in ast.walk(directory_if)
    if isinstance(node, ast.Assign)
    and isinstance(node.value, ast.Call)
    and isinstance(node.value.func, ast.Attribute)
    and node.value.func.attr == "scan"
)

dependency_handler = next(
    node for node in ast.walk(iter_scan)
    if isinstance(node, ast.ExceptHandler)
    and isinstance(node.type, ast.Name)
    and node.type.id == "MissingOptionalDependencyError"
)
non_directory_try = next(
    node for node in ast.walk(iter_scan)
    if isinstance(node, ast.Try)
    and dependency_handler in node.handlers
)

def contains(node, target):
    return any(child is target for child in ast.walk(node))

print("directory_scan_line:", directory_scan.lineno)
print("directory_scan_inside_dependency_try:", contains(non_directory_try, directory_scan))
print("plugin_entry_point:", "dascore.fiber_io" in Path("dascore/io/core.py").read_text())
print("registers_by_input_type:", any(
    isinstance(node, ast.Call)
    and isinstance(node.func, ast.Attribute)
    and node.func.attr == "setdefault"
    and any(isinstance(arg, ast.Attribute) and arg.attr == "input_type" for arg in node.args)
    for node in ast.walk(tree)
))
PY

Repository: DASDAE/dascore

Length of output: 277


Handle missing optional dependencies for directory scans.

fiber_io.scan at line 1438 runs outside the MissingOptionalDependencyError handler. A directory FiberIO plugin that requires an optional package can abort dc.scan instead of aggregating the dependency and continuing. Add equivalent handling for directory scans.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dascore/io/core.py` at line 1446, Update the directory-scan path around
fiber_io.scan so MissingOptionalDependencyError is caught and handled like the
existing optional-dependency path, incrementing missing_optional_deps via
_get_missing_install_name(ex) and continuing the scan instead of aborting
dc.scan.

continue
Comment on lines 1445 to 1447

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve internal import diagnostics during scans

When optional_import correctly identifies an ImportError from inside an installed package, it raises MissingOptionalDependencyError with both install_name and name unset. This aggregation then stores the failure under an empty key and discards its original diagnostic, so dc.scan eventually reports unknown (...) and still claims that additional packages need installing instead of showing the actionable could not be imported (<original error>) message. Preserve these unidentifiable exceptions separately or surface their original diagnostic rather than aggregating them as missing installs.

Useful? React with 👍 / 👎.

# scan() is best-effort across many resources, so surface
# dependency/compatibility problems as warnings and keep
Expand Down
3 changes: 3 additions & 0 deletions dascore/utils/jit.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import warnings
from functools import wraps

from dascore.utils.misc import _get_install_message


class _DummyNumba:
"""A simple class for acting like numba when numba is not installed."""
Expand Down Expand Up @@ -89,6 +91,7 @@ def decorated(*args, **kwargs):
msg = (
f"{func.__name__} requires python module "
f"numba but it is not installed. "
f"{_get_install_message('numba')}"
)
raise ImportError(msg)
else:
Expand Down
55 changes: 53 additions & 2 deletions dascore/utils/misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,40 @@ def iterate(obj):
return obj if isinstance(obj, Iterable) else (obj,)


# Import names whose installable package name differs from the import name.
_INSTALL_NAMES = {
"google.protobuf": "protobuf",
"yaml": "pyyaml",
}


def _get_install_name(import_name: str) -> str:
"""Get the package to install which provides the module import_name."""
parts = import_name.split(".")
# Search most to least specific so sub-modules resolve to their parent.
for stop in range(len(parts), 0, -1):
if (name := _INSTALL_NAMES.get(".".join(parts[:stop]))) is not None:
return name
return parts[0]


def _get_install_message(packages: str | Iterable[str]) -> str:
"""Get a message telling the user how to install one or more packages."""
names = " ".join(sorted(iterate(packages)))
return f"Install with `pip install {names}` or `uv pip install {names}`."


def _is_missing_module(import_name: str, error: ImportError) -> bool:
"""Determine if an ImportError means import_name itself is not installed."""
# Only the import machinery proves a module is absent, and it names the
# module it failed to find. Any other error (eg an installed package
# raising from its __init__) means the failure happened inside code which
# is installed, as does one naming a different module.
if not isinstance(error, ModuleNotFoundError) or not (failed := error.name or ""):
return False
return import_name == failed or import_name.startswith(f"{failed}.")


@overload
def optional_import(
package_name: str,
Expand Down Expand Up @@ -498,8 +532,25 @@ def optional_import(
"""
try:
mod = importlib.import_module(package_name)
except ImportError:
msg = f"{package_name} is not installed but is required for {required_for}"
except ImportError as ex:
install_name = _get_install_name(package_name)
if _is_missing_module(package_name, ex):
msg = (
f"{package_name} is not installed but is required for "
f"{required_for}. {_get_install_message(install_name)}"
)
else:
# The package is installed; something it imports is not, so
# installing it again wouldn't help.
install_name = None
msg = (
f"{package_name} could not be imported ({ex}) but is "
f"required for {required_for}."
)
# Raise here (rather than with warn_or_raise) so the install name is
# attached to the exception for callers which aggregate them.
if on_missing == "raise":
raise MissingOptionalDependencyError(msg, install_name=install_name) from ex
warn_or_raise(msg, MissingOptionalDependencyError, behavior=on_missing)
mod = None
return mod
Expand Down
1 change: 1 addition & 0 deletions docs/changelog.qmd
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ The [releases page](https://github.com/DASDAE/dascore/releases) tracks changes f

## Unreleased API Changes

- Missing optional dependency messages now name the package to install and give the command to install it. The import name and the package name are not always the same, so `dc.scan` reporting `{'google.protobuf.descriptor_pb2': 10308}` left users guessing; it now reports `protobuf (10308 files)` along with ``Install with `pip install protobuf` or `uv pip install protobuf` ``. `MissingOptionalDependencyError` also carries an `install_name` attribute, and an `ImportError` raised inside an installed package is reported as a failed import rather than a missing install.
- The fiber IO format readers now share two helpers instead of each carrying its own copy of the same scaffolding: [`make_scan_payload`](`dascore.io.make_scan_payload`) builds one `FiberIO.scan` payload (taking `dims` and `shape` from the coords unless given), and `dascore.io.utils.build_patches` performs the common `read` tail of trim, drop-if-empty, attach attrs. Two side effects for readers: an already empty source now yields no patch from the `APSensing` and `HDAS` readers rather than a zero-size one (matching every other format), and the `GDR_DAS` and `Neubrex` readers declare `time`/`distance` explicitly rather than absorbing them from `**kwargs`. The unused, never-populated `ProdMLPatchAttrs` classes are removed from `dascore.io.prodml.core` and `dascore.io.dashdf5.core`; `ProdMLRawPatchAttrs` in `dascore.io.prodml.utils` is the one the reader uses. Relatedly, evenly sampled coordinates are now built from a sample count rather than a hand-computed stop, so a source declaring zero samples yields an empty coordinate instead of raising a validation error.
- Fixed chunking a coordinate whose units are not the canonical SI unit (e.g. a distance in feet). Plan trims carry canonical SI magnitudes, which were applied to the patch coordinate as bare numbers, so each piece covered the wrong physical interval and samples were silently dropped — chunking a 300 channel patch in feet returned 61 channels. The trim now converts to the coordinate's own units at load, the same conversion `Spool.select` already performed.
- **`Spool.chunk` accepts quantities as the chunk length.** A quantity in the coordinate's own units works (`chunk(time=10 * dc.units.s)`, `chunk(distance=100 * dc.units.ft)`), where any quantity previously raised `NotImplementedError`. A quantity of information (`chunk(time=25 * dc.units.megabytes)`) chunks so each patch's *data array* is at most the requested size; the sample count is floored, and a partition mixing element types is sized against the dtype assembly upcasts to. `overlap` accepts both forms, and `Spool.chunk_plan(...).params["size"]` reports what a size resolved to. The index schema records each patch's element dtype to make this possible, so its version is bumped and existing indexes must be deleted and rebuilt. An attr named `dtype` is now reserved and stays unindexed.
Expand Down
39 changes: 38 additions & 1 deletion tests/test_io/test_io_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@
from dascore.io.core import (
FiberIO,
_FiberIOManager,
_get_missing_install_name,
_get_reloadable_source_path,
_handle_missing_optionals,
_reinit_manager_lock,
_resolve_read_spool,
_scan_result_to_summary,
Expand Down Expand Up @@ -1106,8 +1108,10 @@ def test_scan_missing_optional_dependency_raises(self, tmp_path):
path.write_text("placeholder")

msg = "found files that can be read if additional packages"
with pytest.raises(MissingOptionalDependencyError, match=msg):
with pytest.raises(MissingOptionalDependencyError, match=msg) as exc_info:
dc.scan(path)
# The message should say how to install the missing package.
assert "pip install not_optional_pkg" in str(exc_info.value)

def test_scan_missing_optional_dependency_warns_with_other_outputs(self, tmp_path):
"""Scan should warn if optional deps are missing but other files load."""
Expand Down Expand Up @@ -1611,6 +1615,39 @@ def test_get_supported_io_table(self):
assert len(result_df) > 0


class TestMissingInstallName:
"""Tests for guessing the package to install from a dependency error."""

def test_name_sources(self):
"""The name comes from the attr, the module, or the legacy message."""
errors = [
MissingOptionalDependencyError("blah", install_name="protobuf"),
MissingOptionalDependencyError("blah", name="google.protobuf"),
MissingOptionalDependencyError("protobuf is not installed but..."),
]
assert [_get_missing_install_name(x) for x in errors] == ["protobuf"] * 3

def test_unidentifiable_package(self):
"""Arbitrary messages should not be mistaken for package names."""
error = MissingOptionalDependencyError("Optional dependency foo is missing")
assert _get_missing_install_name(error) == ""
assert _get_missing_install_name(MissingOptionalDependencyError()) == ""
# Subclasses which skip the init still have an install name.
assert MissingOptionalDependencyError.install_name is None

def test_message_omits_unknown_packages(self):
"""Only identified packages belong in the install command."""
with pytest.raises(MissingOptionalDependencyError) as exc_info:
_handle_missing_optionals(0, {"": 2, "segyio": 1, "protobuf": 3})
msg = str(exc_info.value)
assert "unknown (2 files)" in msg
assert "pip install protobuf segyio" in msg
# Nothing should be recommended when no package was identified.
with pytest.raises(MissingOptionalDependencyError) as exc_info:
_handle_missing_optionals(0, {"": 2})
assert "pip install" not in str(exc_info.value)


class TestIOCoreCoverageEdges:
"""Remaining io.core resolution/robustness branches."""

Expand Down
60 changes: 60 additions & 0 deletions tests/test_utils/test_misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import dascore as dc
from dascore.exceptions import MissingOptionalDependencyError
from dascore.utils.misc import (
_get_install_name,
_iter_filesystem,
_locked,
_spool_map,
Expand Down Expand Up @@ -419,6 +420,15 @@ def test_all_null_false(self, diffs):
assert not all_diffs_close_enough(diffs)


def _raise_error(error):
"""Return a function which raises error when called."""

def _func(*args, **kwargs):
raise error

return _func


class TestOptionalImport:
"""Ensure the optional import works."""

Expand All @@ -439,6 +449,56 @@ def test_ignore(self):
out = optional_import("boblib4", on_missing="ignore")
assert out is None

def test_message_has_install_instructions(self, monkeypatch):
"""The error should say how to install the package, not the module."""
error = ModuleNotFoundError("No module named 'google'", name="google")
monkeypatch.setattr(
"importlib.import_module", _raise_error(error), raising=True
)
with pytest.raises(MissingOptionalDependencyError) as exc_info:
optional_import("google.protobuf.descriptor_pb2")
msg = str(exc_info.value)
assert "pip install protobuf" in msg
assert "uv pip install protobuf" in msg
assert exc_info.value.install_name == "protobuf"
assert exc_info.value.__cause__ is error

@pytest.mark.parametrize(
"error",
[
# An installed package which imports something missing.
ModuleNotFoundError("No module named 'bob'", name="bob"),
# An installed package raising from its own __init__, either
# unnamed or naming itself.
ImportError("dascore.core requires the C extension"),
ImportError("dascore.core is broken", name="dascore.core"),
],
)
def test_broken_install_gives_no_install_advice(self, monkeypatch, error):
"""An import failing inside an installed package isn't fixed by install."""
monkeypatch.setattr(
"importlib.import_module", _raise_error(error), raising=True
)
with pytest.raises(MissingOptionalDependencyError) as exc_info:
optional_import("dascore.core")
msg = str(exc_info.value)
assert "could not be imported" in msg
assert str(error) in msg
assert "pip install" not in msg
assert exc_info.value.install_name is None
assert exc_info.value.__cause__ is error


class TestGetInstallName:
"""Tests for mapping import names to installable package names."""

def test_install_names(self):
"""Sub-modules resolve to the package which provides them."""
assert _get_install_name("xarray") == "xarray"
assert _get_install_name("dascore.utils.misc") == "dascore"
assert _get_install_name("google.protobuf.message") == "protobuf"
assert _get_install_name("yaml") == "pyyaml"


class TestGetStencilCoefficients:
"""Tests for stencil coefficients."""
Expand Down
Loading