From a774672a1f50d0f196c0e370ef3034828ae7977c Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 17:30:36 +0200 Subject: [PATCH 1/5] Let a data directory carry the inventory which describes it A directory may keep its inventory under the name `.inventory` -- the authoring directory or a serialized file -- and a spool opened on one starts out attached to it, so the metadata is found where it lies rather than named again by every script which reads the archive. Hidden for the same reason `.dascore_index.sqlite3` is, and skipped by the file scanner for free. The visible `inventory.yaml` is deliberately not it: in the authoring format that name is the envelope, so a data directory holding one would be claiming to be an inventory directory itself. This is defensible only because attaching is inert. It changes which names resolve and nothing else, so auto-attach never implies auto-conform: `conform_to_inventory` changes `len`, subdivides rows, and can raise on an acquisition straddle, and a directory which silently changed its own length because a file appeared in it would not be. Three moments are kept apart. Discovery is eager and is one existence check, which is also what makes `remove_inventory` stick, since nothing fills the slot again. Reading is lazy, at the first question only an inventory can answer -- never `len`, `get_contents`, `sort`, `chunk`, extraction, or a selection about names the index already knows. Whether a query is one of those is decided without reading anything: the observing-system facts are the models' own, the same for every inventory, and a coordinate an inventory runs along the fiber is by definition a name the index does not have. Refreshing is explicit -- `attach_inventory()` with no argument means the one this directory carries, read now -- because an inventory is an input rather than a cache, and a file which changes under a running program is a new input rather than a stale one. The read is held on a holder shared by every derived spool, since spools copy-construct from their parents, so a spool sliced ten ways reads its inventory once and two views of one parent cannot disagree about what it says. `attach_inventory`, `enrich` and `conform_to_inventory` also take a path now, read on the same terms. Two properties fall out, and are the point: a malformed inventory can never stop you loading data, since discovery only asks whether something is there; and when an inventory-backed call does fail, it says which file it means and that the spool picked it up on opening, because an InvalidInventoryError surfacing from inside `select` is otherwise baffling. --- dascore/core/inventory_loader.py | 74 ++++++ dascore/core/spool.py | 293 ++++++++++++++++++--- tests/test_core/test_inventory_loader.py | 67 +++++ tests/test_proc/test_proc_inventory.py | 307 ++++++++++++++++++++++- 4 files changed, 705 insertions(+), 36 deletions(-) diff --git a/dascore/core/inventory_loader.py b/dascore/core/inventory_loader.py index d16b712e4..f5f4b2bae 100644 --- a/dascore/core/inventory_loader.py +++ b/dascore/core/inventory_loader.py @@ -75,6 +75,16 @@ _ATTRS_STEM = "attrs" _ENVELOPE_STEM = "inventory" +# The name a data directory carries its own inventory under, in either +# form: the directory `.inventory/` or a file `.inventory.yaml`. Hidden, +# like `.dascore_index.sqlite3` beside it -- a companion the directory +# keeps rather than content it holds -- which is also what keeps the file +# scanner (whose `skip_hidden` defaults to True) from reading it as data. +# The visible spelling is deliberately not accepted: `inventory.yaml` is +# the envelope of the authoring format, so a data directory holding one +# would be claiming to be an inventory directory itself. +BLESSED_NAME = ".inventory" + # Separates an entity's name from the epoch it starts. _EPOCH_MARKER = "@" @@ -1484,6 +1494,70 @@ def load_directory(path: str | os.PathLike) -> Inventory: return Inventory(**(envelope or {}), resources=resources, networks=networks).check() +def _blessed_candidates(directory: Path) -> tuple[Path, ...]: + """Every spelling of the blessed name, the directory form first.""" + tree = directory / BLESSED_NAME + return (tree, *(tree.with_suffix(suffix) for suffix in _OBJECT_SUFFIXES)) + + +def carries_inventory(directory: str | os.PathLike) -> bool: + """ + Return whether a directory holds anything under the blessed name. + + A few stats and nothing more: this is the question a spool asks when + it opens a directory, so it must not read, parse, or judge what it + finds. Whether what is there is loadable -- or even whether the + directory says which of the two forms it means -- is + ``find_inventory``'s to answer, when something actually asks. + + Parameters + ---------- + directory + The directory to look in. + """ + return any(x.exists() for x in _blessed_candidates(Path(directory))) + + +def find_inventory(directory: str | os.PathLike) -> Path | None: + """ + Return the inventory a directory carries, or None if it carries none. + + Parameters + ---------- + directory + The directory to look in. + """ + candidates = _blessed_candidates(Path(directory)) + tree = candidates[0] + if not (found := [x for x in candidates if x.exists()]): + return None + if len(found) > 1: + listed = ", ".join(sorted(x.name for x in found)) + msg = ( + f"{tree.parent} carries more than one inventory: {listed}. A " + "directory states its inventory once; keep the one it means." + ) + raise InvalidInventoryError(msg) + (only,) = found + # A near-miss: something is sitting under the blessed name in a form + # that name does not take, which is a misspelling of the convention + # rather than a file which owes it nothing. + if only == tree and not only.is_dir(): + msg = ( + f"{only} is a file. An inventory a directory carries is either " + f"the authoring directory {BLESSED_NAME}/ or a file naming its " + f"format, {BLESSED_NAME}{_OBJECT_SUFFIXES[0]}." + ) + raise InvalidInventoryError(msg) + if only != tree and only.is_dir(): + msg = ( + f"{only} is a directory. The authoring directory an inventory " + f"lives in takes no suffix: {BLESSED_NAME}/." + ) + raise InvalidInventoryError(msg) + return only + + def inventory(source: Inventory | str | os.PathLike | None = None) -> Inventory: """ Load or create a DASDAE inventory. diff --git a/dascore/core/spool.py b/dascore/core/spool.py index d583c3263..202187673 100644 --- a/dascore/core/spool.py +++ b/dascore/core/spool.py @@ -4,6 +4,7 @@ import abc import inspect +import os import warnings from collections.abc import Callable, Generator, Iterator, Mapping, Sequence from dataclasses import replace @@ -14,6 +15,7 @@ import numpy as np import pandas as pd +from pydantic import ValidationError from rich.text import Text from typing_extensions import Self @@ -34,10 +36,17 @@ path_types, timeable_types, ) -from dascore.core.inventory import Inventory +from dascore.core.inventory import _SYSTEM_FACT_NAMES, Inventory +from dascore.core.inventory_loader import ( + BLESSED_NAME, + carries_inventory, + find_inventory, +) from dascore.exceptions import ( + InvalidInventoryError, InvalidSpoolError, InvalidSpoolQueryError, + MissingOptionalDependencyError, MissingPatchError, ParameterError, PatchError, @@ -430,6 +439,97 @@ def _combine_state(values, label): return present[0] +class _InventoryRef: + """ + An inventory a spool has been pointed at but has not read. + + Attaching states where the inventory is; the read happens at the + first question only an inventory can answer, and then once. The + holder is shared rather than copied, because a spool copy-constructs + from its parent (`select`, `sort`, `chunk` each return a new one), so + a spool sliced ten ways still reads its inventory a single time and + two views of one parent can never disagree about what it says. + + An inventory is an input, not a cache, so it is never re-read behind + the caller's back: a file which changes under a running program is a + new input rather than a stale one, and `Spool.attach_inventory()` + with no argument is how the program says to read it again. A read + which failed is not a read, though, and is tried again next time -- + an unreadable inventory is a thing to go and fix, and holding the + failure would mean the fix could not be seen. + """ + + def __init__(self, path, blessed: bool = False): + # For a blessed reference this is the directory, not the file: + # which of the two forms the directory carries is decided when it + # is read, so discovery stays a stat and its complaints wait. + self.path = path + self.blessed = blessed + self._inventory: Inventory | None = None + + def resolve(self) -> Inventory: + """Return the inventory, reading it if this is the first ask.""" + if self._inventory is None: + self._inventory = self._read() + return self._inventory + + def _read(self) -> Inventory: + """Read the inventory, saying where an unreadable one came from.""" + try: + source = self.path + if self.blessed: + source = find_inventory(self.path) + if source is None: + msg = "nothing is there now, though there was on opening" + raise InvalidInventoryError(msg) + return dc.inventory(source) + except ( + InvalidInventoryError, + MissingOptionalDependencyError, + ValidationError, + OSError, + ) as error: + # Surfacing from inside select() or enrich(), the failure has + # to say which file it means and how the spool came to have + # it -- most sharply when nobody chose it by hand. + where = ( + f"the inventory {self.path} carries under the name " + f"{BLESSED_NAME!r}, attached when the spool was opened" + if self.blessed + else f"the inventory attached to this spool from {self.path}" + ) + msg = f"Could not read {where}: {error}" + raise InvalidInventoryError(msg) from error + + def __eq__(self, other) -> bool: + """ + Whether this and another attachment are the same inventory. + + Two references to one place are the same without either being + read; anything else is the inventories' own question, and + answering it is what makes a lazily attached spool comparable to + one holding the inventory itself. Something which is no kind of + inventory is not that question, and reading a file to answer it + would be a read nobody asked for. + """ + if isinstance(other, _InventoryRef): + if (self.path, self.blessed) == (other.path, other.blessed): + return True + elif not isinstance(other, Inventory): + return NotImplemented + return self.resolve() == _attached_inventory(other) + + # Defined because __eq__ is: a reference is spool state, never a key. + __hash__ = None + + +def _attached_inventory(attachment) -> Inventory: + """Return the inventory an attachment holds, reading it if needed.""" + if isinstance(attachment, _InventoryRef): + return attachment.resolve() + return attachment + + def _combine_inventories(first, second) -> tuple: """ Return the (inventory, enrich kwargs) a union of two spools carries. @@ -948,9 +1048,10 @@ class Spool(BaseSpool): ) # The catalog backing this spool; every construction path sets one. _catalog: PatchCatalog - # An attached inventory, the enrich kwargs to apply on extraction - # (None means attached without automatic enrichment), and what to do - # with a patch the inventory does not describe. + # An attached inventory -- itself, or a reference which reads it when + # something asks -- the enrich kwargs to apply on extraction (None + # means attached without automatic enrichment), and what to do with a + # patch the inventory does not describe. _inventory = None _enrich_kwargs: dict | None = None _on_unresolved: str = "warn" @@ -1083,13 +1184,9 @@ def unselect( """{doc}.""" requested = _requested_names(_attrs, _coords, kwargs) known_attrs, known_coords = self._index_names() - selectable, channels = set(), {} - if self._inventory is not None: - names = self._inventory.get_names() - channels = self._channel_query( - requested, names, known_attrs, known_coords, _coords, kwargs - ) - selectable = set(names.attrs) - known_coords + channels, selectable = self._inventory_query( + requested, known_attrs, known_coords, _coords, kwargs + ) attrs, coords = resolve_selector_namespaces( known_attrs | selectable, known_coords, @@ -1140,8 +1237,47 @@ def _index_names(self) -> tuple[set[str], set[str]]: backend = self._catalog.backend return set(backend.attr_names()), set(backend.coord_names()) + def _inventory_query( + self, requested, known_attrs, known_coords, _coords, kwargs + ) -> tuple[dict, set[str]]: + """ + Split what a query names between the inventory and the index. + + Returns the selectors the inventory answers per channel, and the + attr names it could state — which is what widens the namespace a + query may draw on. + + Whether an inventory has anything to say here is settled without + reading one: the observing-system facts are the models' own, the + same for every inventory, and a coordinate an inventory runs + along the fiber is by definition a name the index does not have. + So a query about what the index already knows leaves a lazily + attached inventory unread, and one naming anything else is + asking a question only the inventory can answer. + """ + if self._inventory is None: + return {}, set() + # A name the index already uses for a coordinate keeps its meaning; + # bare names resolve to attrs first, and an inventory must not + # quietly move one out of the namespace it has always been in. + selectable = set(_SYSTEM_FACT_NAMES) - known_coords + outside = (requested - known_attrs - known_coords - selectable) | ( + _namespace_names(_coords) - known_coords + ) + if not outside: + return {}, selectable + channels = self._channel_query( + self._resolved_inventory().get_names(), + requested, + known_attrs, + known_coords, + _coords, + kwargs, + ) + return channels, selectable + def _channel_query( - self, requested, names, known_attrs, known_coords, _coords, kwargs + self, names, requested, known_attrs, known_coords, _coords, kwargs ) -> dict: """ Return the selectors naming coordinates the inventory runs along @@ -1191,9 +1327,8 @@ def _split_inventory_query(self, _attrs, _coords, kwargs, samples, relative=Fals return {}, {}, _attrs, _coords, kwargs requested = _requested_names(_attrs, _coords, kwargs) known_attrs, known_coords = self._index_names() - names = self._inventory.get_names() - channels = self._channel_query( - requested, names, known_attrs, known_coords, _coords, kwargs + channels, selectable = self._inventory_query( + requested, known_attrs, known_coords, _coords, kwargs ) # Neither keyword has anything to mean about a value the fiber # states: it has no sample numbering of its own -- the channels it @@ -1216,10 +1351,6 @@ def _split_inventory_query(self, _attrs, _coords, kwargs, samples, relative=Fals raise InvalidSpoolQueryError(msg) kwargs = _without_keys(kwargs, channels) _coords = _without_names(_coords, channels) - # A name the index already uses for a coordinate keeps its meaning; - # bare names resolve to attrs first, and an inventory must not - # quietly move one out of the namespace it has always been in. - selectable = set(names.attrs) - known_coords # samples=True selections are coordinate-only, so an attr among # them is an error the index states better than this can. if samples or not requested & selectable: @@ -1277,7 +1408,11 @@ def _select_channels(self, query: dict, *, complement=False, applies_to=None): judged = np.isin(working["_patch_id"].to_numpy(), np.asarray(applies_to)) contexts[~judged] = None name, pieces, reasons = resolve_channel_pieces( - self._inventory, contexts, working, query, complement=complement + self._resolved_inventory(), + contexts, + working, + query, + complement=complement, ) _refuse_rows(source_rows, reasons, _UNPLACEABLE) if name is None: @@ -1333,7 +1468,9 @@ def _select_from_inventory(self, query: dict) -> Self: if contexts is None: contexts = self._resolve_rows(ids) matched[~stated] = _match_resolved( - get_attr_values(self._inventory, contexts[~stated], name), + get_attr_values( + self._resolved_inventory(), contexts[~stated], name + ), name, selector, backend.attr_units(name), @@ -1361,7 +1498,7 @@ def _resolve_rows(self, ids) -> np.ndarray: resolved = dict( zip( df["_patch_id"].to_numpy(), - resolve_contexts(self._inventory, *columns), + resolve_contexts(self._resolved_inventory(), *columns), strict=True, ) ) @@ -1384,7 +1521,7 @@ def _index_matches(self, name: str, selector, known: set[str]) -> np.ndarray: dtype=np.int64, ) - def attach_inventory(self, inventory) -> Self: + def attach_inventory(self, inventory=None) -> Self: """ Attach a DASDAE inventory to this spool. @@ -1401,7 +1538,10 @@ def attach_inventory(self, inventory) -> Self: Parameters ---------- inventory - The inventory to carry. + The inventory to carry: an `Inventory`, or the path of one + (an authoring directory or a serialized file), which is read + at the first question rather than now. None means the one + the spool's own directory carries, read again. Examples -------- @@ -1422,15 +1562,63 @@ def attach_inventory(self, inventory) -> Self: inventory defines along the fiber become selectable, and [`split_by`](`dascore.core.spool.Spool.split_by`) can expand the spool by the values of one. + + A spool opened on a directory which carries an inventory under + the name `.inventory` starts out attached to it, so this is + needed there only to attach a different one — or, with no + argument, to read that one again after editing it. An inventory + is read once and held, since it is an input rather than a cache; + re-reading it is a thing the program says, not something which + happens behind it. """ - if not isinstance(inventory, Inventory): - msg = f"attach_inventory needs an Inventory, got {type(inventory)}." + if inventory is None: + inventory = self._blessed_inventory(demanded=True) + elif isinstance(inventory, str | os.PathLike): + path = Path(inventory) + # Eager, though the read is not: a path which is not there is + # the caller's own mistake, and saying so later would blame + # whichever call first happened to ask a question. + if not path.exists(): + msg = f"No inventory at {path}." + raise InvalidInventoryError(msg) + inventory = _InventoryRef(path) + elif not isinstance(inventory, Inventory): + msg = ( + "attach_inventory needs an Inventory or the path of one, " + f"got {type(inventory)}." + ) raise ParameterError(msg) new = self.__class__(self) new._inventory = inventory new._enrich_kwargs = None return new + def _blessed_inventory(self, demanded: bool = False): + """ + A reference to the inventory this spool's directory carries. + + Whether one is there is settled now, by a stat; which form it + takes and whether it can be read wait until something asks. When + `demanded`, having none is an error rather than an answer, since + the caller asked for that one in particular. + """ + path = self.spool_path + on_directory = path is not None and path.is_dir() + if on_directory and carries_inventory(path): + return _InventoryRef(path, blessed=True) + if not demanded: + return None + where = ( + f"{path} holds nothing named {BLESSED_NAME}" + if on_directory + else "this spool was not opened on a directory which could hold one" + ) + msg = ( + f"This spool carries no inventory of its own: {where}. Pass the " + "inventory to attach, or the path of one." + ) + raise InvalidInventoryError(msg) + def remove_inventory(self) -> Self: """ Return a spool carrying no inventory. @@ -1440,6 +1628,10 @@ def remove_inventory(self) -> Self: with no inventory is returned unchanged in substance; as everywhere else, the original spool is left alone. + Removal sticks, including on a spool which found its inventory in + its own directory: the slot is filled when the spool is opened + and nothing fills it again. + Examples -------- >>> import dascore as dc @@ -1489,8 +1681,9 @@ def enrich( Parameters ---------- inventory - The inventory to enrich from. Defaults to the spool's attached - inventory; given one, it is attached as well. + The inventory to enrich from, or the path of one. Defaults to + the spool's attached inventory; given one, it is attached as + well. on_unresolved What to do with a patch the inventory does not describe — one naming no entry, or naming one the inventory does not resolve @@ -1612,7 +1805,7 @@ def split_by( # into, so it would quietly give an empty spool. Selection refuses # a name it does not know, and a misspelling is no more meaningful # here than it is there. - if name not in set(self._inventory.get_names().coords): + if name not in set(self._resolved_inventory().get_names().coords): msg = ( f"{name!r} is not a coordinate the attached inventory defines " "along the fiber, so there is nothing to split on. " @@ -1624,7 +1817,11 @@ def split_by( _check_stampable(name, working) contexts = self._plan_contexts(working) dim, rows, reasons = resolve_split_pieces( - self._inventory, contexts, working, name, _glob_filter(include, exclude) + self._resolved_inventory(), + contexts, + working, + name, + _glob_filter(include, exclude), ) _refuse_rows(source_rows, reasons, _UNPLACEABLE) if dim is None: # nothing to split: no row has a fiber to split on @@ -1642,7 +1839,7 @@ def _plan_contexts(self, working) -> np.ndarray: columns = _resolution_columns(working) if columns is None: return np.full(len(working), None, dtype=object) - return resolve_contexts(self._inventory, *columns) + return resolve_contexts(self._resolved_inventory(), *columns) def conform_to_inventory( self, @@ -1669,7 +1866,8 @@ def conform_to_inventory( Parameters ---------- inventory - The inventory to conform to. Defaults to the spool's attached + The inventory to conform to, or the path of one. Defaults to + the spool's attached inventory; given one, it is attached as well — and attaching clears enrichment set up from the old one, as it does everywhere. Conforming to the spool's own inventory leaves @@ -1731,7 +1929,7 @@ def conform_to_inventory( epochs = ( [_NO_EPOCHS] * len(working) if columns is None - else resolve_row_epochs(new._inventory, *columns) + else resolve_row_epochs(new._resolved_inventory(), *columns) ) _refuse_rows( source_rows, @@ -1829,6 +2027,22 @@ def _restrict_to_rows(self, patch_ids, keep: bool = True) -> Self: return self return self._new_from_catalog(self._catalog.restrict(mask, ids=ids)) + def _resolved_inventory(self) -> Inventory: + """ + The attached inventory itself, read now if it has not been. + + Every use of an inventory goes through here, and only uses: the + cheap `self._inventory is None` says whether one is attached at + all, which is what lets a spool be opened, counted, ordered, + chunked, and read without a lazily attached inventory ever being + touched. Data access is never hostage to a metadata file. + """ + # Every caller is already behind that cheap check, one way or + # another: asking an inventory question of a spool carrying none + # is refused where the question is asked, in its own words. + assert self._inventory is not None + return _attached_inventory(self._inventory) + def _enrichment(self): """Return how this spool enriches, or None if it does not.""" if self._inventory is None or self._enrich_kwargs is None: @@ -1841,7 +2055,7 @@ def _maybe_enrich(self, patch): return patch kwargs, on_unresolved = enrichment try: - return patch.enrich(self._inventory, **kwargs) + return patch.enrich(self._resolved_inventory(), **kwargs) except UnresolvedPatchError: # The inventory does not describe this patch. Dropping it is # conform_to_inventory's job, so it comes out as it went in. @@ -2165,6 +2379,12 @@ def from_directory(cls, path, index_path=None) -> Self: The directory's index (created/updated via ``update()``) backs the catalog; ``path`` may also be an existing directory indexer. + + A directory which carries an inventory under the name + ``.inventory`` — the authoring directory ``.inventory/`` or a + serialized ``.inventory.yaml`` — hands it to the spool, which + reads it at the first question only an inventory can answer. See + [`attach_inventory`](`dascore.core.spool.Spool.attach_inventory`). """ from dascore.io.index.catalog import FileResolver, PatchCatalog # noqa: PLC0415 from dascore.io.index.indexer import DBDirectoryIndexer # noqa: PLC0415 @@ -2181,6 +2401,11 @@ def from_directory(cls, path, index_path=None) -> Self: ) else: out._catalog = PatchCatalog.from_directory(path, index_path=index_path) + # A directory which carries an inventory hands it to the spool + # over it, once, here: filling the slot at the moment the spool is + # opened is what makes `remove_inventory` stick, since nothing + # refills it afterwards. + out._inventory = out._blessed_inventory() return out @classmethod diff --git a/tests/test_core/test_inventory_loader.py b/tests/test_core/test_inventory_loader.py index fc8e66a12..4a760a2b2 100644 --- a/tests/test_core/test_inventory_loader.py +++ b/tests/test_core/test_inventory_loader.py @@ -2026,3 +2026,70 @@ def test_a_large_table_is_not_held_twice(self, make_inventory): ), } assert len(one_path(make_inventory(files)).geometry[0].distance) == 2000 + + +class TestFindInventory: + """The name a data directory carries its own inventory under.""" + + def test_nothing_there(self, tmp_path): + """A directory carrying no inventory says so both ways.""" + assert loader.find_inventory(tmp_path) is None + assert not loader.carries_inventory(tmp_path) + + def test_the_file_form(self, tmp_path): + """A serialized inventory beside the data it describes.""" + found = tmp_path / f"{loader.BLESSED_NAME}.yaml" + found.write_text(dc.inventory().to_yaml()) + assert loader.carries_inventory(tmp_path) + assert loader.find_inventory(tmp_path) == found + assert isinstance(dc.inventory(loader.find_inventory(tmp_path)), inv.Inventory) + + def test_the_directory_form(self, tmp_path): + """An authoring directory beside the data it describes.""" + found = write_inventory(tmp_path / loader.BLESSED_NAME, MINIMAL) + assert loader.carries_inventory(tmp_path) + assert loader.find_inventory(tmp_path) == found + assert len(dc.inventory(loader.find_inventory(tmp_path)).networks) == 1 + + def test_the_visible_name_is_not_it(self, tmp_path): + """`inventory.yaml` is the envelope of the authoring format.""" + (tmp_path / "inventory.yaml").write_text(dc.inventory().to_yaml()) + assert not loader.carries_inventory(tmp_path) + assert loader.find_inventory(tmp_path) is None + + @pytest.mark.parametrize("suffix", loader._OBJECT_SUFFIXES) + def test_every_object_suffix(self, tmp_path, suffix): + """One data model stands behind each spelling here too.""" + found = tmp_path / f"{loader.BLESSED_NAME}{suffix}" + found.write_text("{}") + assert loader.find_inventory(tmp_path) == found + + def test_two_spellings_of_one_fact(self, tmp_path): + """A directory states its inventory once.""" + (tmp_path / f"{loader.BLESSED_NAME}.yaml").write_text("{}") + (tmp_path / loader.BLESSED_NAME).mkdir() + with pytest.raises(InvalidInventoryError, match="more than one inventory"): + loader.find_inventory(tmp_path) + + def test_two_files_of_one_fact(self, tmp_path): + """Two suffixes are two spellings, as they are inside the format.""" + (tmp_path / f"{loader.BLESSED_NAME}.yaml").write_text("{}") + (tmp_path / f"{loader.BLESSED_NAME}.json").write_text("{}") + with pytest.raises(InvalidInventoryError, match="more than one inventory"): + loader.find_inventory(tmp_path) + + def test_a_suffixless_file(self, tmp_path): + """The bare name is the directory form; a file under it names none.""" + (tmp_path / loader.BLESSED_NAME).write_text("{}") + with pytest.raises(InvalidInventoryError, match="is a file"): + loader.find_inventory(tmp_path) + + def test_a_suffixed_directory(self, tmp_path): + """The authoring directory takes no suffix.""" + (tmp_path / f"{loader.BLESSED_NAME}.yaml").mkdir() + with pytest.raises(InvalidInventoryError, match="is a directory"): + loader.find_inventory(tmp_path) + + def test_the_name_is_hidden(self): + """Which is what keeps the file scanner from reading it as data.""" + assert loader.BLESSED_NAME.startswith(".") diff --git a/tests/test_proc/test_proc_inventory.py b/tests/test_proc/test_proc_inventory.py index fb58b7879..9878404c3 100644 --- a/tests/test_proc/test_proc_inventory.py +++ b/tests/test_proc/test_proc_inventory.py @@ -35,6 +35,7 @@ OpticalPath, OpticalPathAnnotation, ) +from dascore.core.inventory_loader import BLESSED_NAME from dascore.examples import get_example_patch, inventory_patch_pair from dascore.exceptions import ( CoordMergeError, @@ -573,9 +574,9 @@ def test_equality_includes_inventory(self, patch, inventory): assert attached == plain.attach_inventory(inventory) def test_requires_an_inventory(self, patch): - """Anything else names no metadata to attach.""" + """Anything which is neither an inventory nor a path names none.""" with pytest.raises(ParameterError, match="needs an Inventory"): - dc.spool(patch).attach_inventory("inventory.yaml") + dc.spool(patch).attach_inventory(42) class TestRemoveInventory: @@ -605,6 +606,308 @@ def test_keeps_the_patches(self, patch, inventory): assert spool.remove_inventory()[0].equals(patch) +def write_inventory(root, files): + """Write a mapping of relative path to text as an authoring directory.""" + for name, text in files.items(): + path = root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + return root + + +@pytest.fixture(scope="class") +def data_directory(tmp_path_factory, patch, inventory): + """A directory of data which carries the inventory describing it.""" + path = tmp_path_factory.mktemp("blessed") + dc.write(patch, path / "patch.h5", "dasdae") + inventory.to_yaml(path / f"{BLESSED_NAME}.yaml") + return path + + +class TestBlessedInventory: + """A data directory hands its own inventory to the spool over it.""" + + def test_a_directory_spool_comes_attached(self, data_directory): + """Which is the whole point: the metadata is found where it lies.""" + spool = dc.spool(data_directory).update() + assert spool.enrich()[0].attrs.gauge_length == 10.0 + + def test_the_directory_form(self, tmp_path, patch): + """The authoring directory is the other spelling of the name.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + write_inventory( + tmp_path / BLESSED_NAME, + { + "acquisitions/DAS.R2D1..RAW.yaml": ( + "object_type: Acquisition\ndata_category: DAS\ngauge_length: 3.0\n" + ), + }, + ) + spool = dc.spool(tmp_path).update() + assert spool.enrich(coords=False)[0].attrs.gauge_length == 3.0 + + def test_the_scanner_ignores_it(self, tmp_path, patch): + """A hidden name is one the file scanner already skips.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + write_inventory(tmp_path / BLESSED_NAME, {"inventory.yaml": "version: 0.0.1\n"}) + spool = dc.spool(tmp_path).update() + assert len(spool) == 1 + + def test_a_visible_inventory_is_not_attached(self, tmp_path, patch, inventory): + """`inventory.yaml` names the envelope, not a spool's inventory.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + inventory.to_yaml(tmp_path / "inventory.yaml") + assert dc.spool(tmp_path).update()._inventory is None + + def test_in_memory_spools_carry_none(self, patch): + """There is no directory to have carried one.""" + assert dc.spool(patch)._inventory is None + + +class TestLazyInventory: + """Attaching states where an inventory is; reading it waits.""" + + def test_nothing_is_read_by_opening(self, data_directory): + """Discovery is a stat, so an archive opens as fast as ever.""" + spool = dc.spool(data_directory).update() + assert spool._inventory._inventory is None + + def test_nothing_is_read_by_using_the_data(self, data_directory): + """Data access is never hostage to a metadata file.""" + spool = dc.spool(data_directory).update() + len(spool) + spool.get_contents() + spool.select(time=...).sort("time").chunk(time=None) + assert "gauge_length" not in dict(spool[0].attrs) + assert spool._inventory._inventory is None + + def test_nothing_is_read_by_unselecting_on_the_index(self, data_directory): + """Which names it can widen a query with is known without reading.""" + spool = dc.spool(data_directory).update() + assert len(spool.unselect(tag="random")) == 0 + assert spool._inventory._inventory is None + + def test_the_attr_names_are_the_models_own(self, inventory): + """What `_inventory_query` decides without reading rests on this.""" + assert inventory.get_names().attrs == dc.inventory().get_names().attrs + + def test_a_fiber_coordinate_reads_it(self, data_directory): + """A name the index does not have is a question for the inventory.""" + spool = dc.spool(data_directory).update() + assert len(spool.select(zone="north")) == 1 + assert spool._inventory._inventory is not None + + def test_a_system_fact_reads_it(self, data_directory): + """The name is the models', but only the inventory holds the value.""" + spool = dc.spool(data_directory).update() + assert len(spool.select(gauge_length=10.0)) == 1 + assert spool._inventory._inventory is not None + + def test_the_first_question_reads_it(self, data_directory): + """And answers it from what was read.""" + spool = dc.spool(data_directory).update() + assert spool.enrich()[0].attrs.gauge_length == 10.0 + assert spool._inventory._inventory is not None + + def test_derived_spools_read_once(self, data_directory): + """The holder is shared, so a sliced spool cannot read twice.""" + spool = dc.spool(data_directory).update() + derived = spool.select(time=...).sort("time") + assert derived._inventory is spool._inventory + derived.enrich()[0] + assert spool._inventory._inventory is not None + + def test_a_malformed_inventory_still_loads_data(self, tmp_path, patch): + """Only the calls which need an inventory fail.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + (tmp_path / f"{BLESSED_NAME}.yaml").write_text("networks: [{code: [1, 2]}]\n") + spool = dc.spool(tmp_path).update() + assert len(spool) == 1 + assert spool[0].equals(patch) + with pytest.raises(InvalidInventoryError, match=re.escape(BLESSED_NAME)): + spool.enrich()[0] + + def test_the_failure_says_where_it_came_from(self, tmp_path, patch): + """A spool nobody attached to must say how it came to be attached.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + (tmp_path / f"{BLESSED_NAME}.yaml").write_text("[]\n") + spool = dc.spool(tmp_path).update() + with pytest.raises(InvalidInventoryError, match="when the spool was opened"): + spool.split_by("zone") + + def test_an_ambiguous_name_waits_for_a_question(self, tmp_path, patch): + """Two spellings of the name are still no reason to refuse data.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + (tmp_path / f"{BLESSED_NAME}.yaml").write_text("{}\n") + (tmp_path / BLESSED_NAME).mkdir() + spool = dc.spool(tmp_path).update() + assert len(spool) == 1 + with pytest.raises(InvalidInventoryError, match="more than one inventory"): + spool.enrich()[0] + + def test_an_inventory_which_leaves_says_so(self, tmp_path, patch): + """Removed between opening and asking, which is not "none here".""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + (tmp_path / f"{BLESSED_NAME}.yaml").write_text("{}\n") + spool = dc.spool(tmp_path).update() + (tmp_path / f"{BLESSED_NAME}.yaml").unlink() + with pytest.raises(InvalidInventoryError, match="nothing is there now"): + spool.enrich()[0] + + def test_an_edit_is_not_seen_until_it_is_asked_for( + self, tmp_path, patch, inventory + ): + """An inventory is an input, not a cache: it is read once.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + inventory.to_yaml(tmp_path / f"{BLESSED_NAME}.yaml") + spool = dc.spool(tmp_path).update() + assert spool.enrich()[0].attrs.gauge_length == 10.0 + _replace_acquisition(inventory, gauge_length=2.0).to_yaml( + tmp_path / f"{BLESSED_NAME}.yaml" + ) + assert spool.enrich()[0].attrs.gauge_length == 10.0 + assert spool.attach_inventory().enrich()[0].attrs.gauge_length == 2.0 + + def test_a_failed_read_is_not_a_read(self, tmp_path, patch, inventory): + """An unreadable inventory is a thing to fix, so the fix is seen.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + (tmp_path / f"{BLESSED_NAME}.yaml").write_text("[]\n") + spool = dc.spool(tmp_path).update() + with pytest.raises(InvalidInventoryError): + spool.enrich()[0] + inventory.to_yaml(tmp_path / f"{BLESSED_NAME}.yaml") + assert spool.enrich()[0].attrs.gauge_length == 10.0 + + def test_a_lazily_attached_spool_pickles(self, data_directory): + """Which is how a spool reaches a process pool at all.""" + spool = dc.spool(data_directory).update() + restored = pickle.loads(pickle.dumps(spool)) + assert restored.enrich()[0].attrs.gauge_length == 10.0 + + def test_removing_it_sticks(self, data_directory): + """Nothing refills the slot after the spool is opened.""" + spool = dc.spool(data_directory).update().remove_inventory() + assert spool._inventory is None + assert spool.select(time=...)._inventory is None + + def test_update_keeps_it(self, data_directory): + """Re-indexing the files says nothing about the inventory.""" + assert dc.spool(data_directory).update().update()._inventory is not None + + +class TestAttachInventoryPath: + """An inventory can be attached by name as well as by value.""" + + def test_a_file_path(self, tmp_path, patch, inventory): + """The serialized artifact shipped beside an archive.""" + path = tmp_path / "somewhere.yaml" + inventory.to_yaml(path) + spool = dc.spool(patch).attach_inventory(path) + assert spool.enrich()[0].attrs.gauge_length == 10.0 + + def test_a_directory_path(self, tmp_path, patch): + """The authoring directory itself.""" + root = write_inventory( + tmp_path / "authored", + { + "acquisitions/DAS.R2D1..RAW.yaml": ( + "object_type: Acquisition\ndata_category: DAS\ngauge_length: 4.0\n" + ), + }, + ) + spool = dc.spool(patch).attach_inventory(root) + assert spool.enrich(coords=False)[0].attrs.gauge_length == 4.0 + + def test_a_path_is_read_lazily(self, tmp_path, patch, inventory): + """As the blessed name is, and for the same reason.""" + path = tmp_path / "somewhere.yaml" + inventory.to_yaml(path) + spool = dc.spool(patch).attach_inventory(path) + assert spool._inventory._inventory is None + + def test_a_path_which_is_not_there(self, patch): + """Eager, though the read is not: the caller named this one.""" + with pytest.raises(InvalidInventoryError, match="No inventory at"): + dc.spool(patch).attach_inventory("nowhere.yaml") + + def test_a_named_failure_says_which_file(self, tmp_path, patch): + """Named by hand, so the name is what the message needs.""" + path = tmp_path / "broken.yaml" + path.write_text("[]\n") + spool = dc.spool(patch).attach_inventory(path) + with pytest.raises(InvalidInventoryError, match=r"broken\.yaml"): + spool.enrich()[0] + + def test_enrich_takes_a_path(self, tmp_path, patch, inventory): + """Both inventory verbs attach what they are given.""" + path = tmp_path / "somewhere.yaml" + inventory.to_yaml(path) + assert dc.spool(patch).enrich(path)[0].attrs.gauge_length == 10.0 + + def test_conform_takes_a_path(self, tmp_path, patch, inventory): + """The same, for the verb which makes the index truthful.""" + path = tmp_path / "somewhere.yaml" + inventory.to_yaml(path) + assert len(dc.spool(patch).conform_to_inventory(path)) == 1 + + def test_no_argument_needs_a_directory(self, patch): + """There is nowhere an in-memory spool could have carried one.""" + with pytest.raises(InvalidInventoryError, match="not opened on a directory"): + dc.spool(patch).attach_inventory() + + def test_no_argument_needs_a_blessed_name(self, tmp_path, patch): + """The directory is there; what it was asked for is not.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + spool = dc.spool(tmp_path).update() + with pytest.raises(InvalidInventoryError, match="holds nothing named"): + spool.attach_inventory() + + def test_re_attaching_clears_enrichment(self, data_directory): + """As attaching an inventory by value does.""" + spool = dc.spool(data_directory).update().enrich(coords=False) + assert spool.attach_inventory()._enrich_kwargs is None + + +class TestLazyInventoryEquality: + """Two spools agree about an inventory whether or not they read it.""" + + def test_two_references_to_one_place(self, data_directory): + """Which needs no read at all.""" + first, second = dc.spool(data_directory), dc.spool(data_directory) + assert first.update() == second.update() + assert first._inventory._inventory is None + + def test_a_reference_and_the_inventory_itself(self, data_directory, inventory): + """One side is resolved to answer, since a name is not a value.""" + spool = dc.spool(data_directory).update() + assert spool == spool.attach_inventory(inventory) + + def test_a_reference_and_a_different_inventory(self, data_directory, inventory): + """The read decides it, and decides against.""" + spool = dc.spool(data_directory).update() + other = _replace_acquisition(inventory, gauge_length=7.0) + assert spool != spool.attach_inventory(other) + + def test_something_which_is_no_kind_of_inventory(self, data_directory): + """Not a question worth reading a file to answer.""" + spool = dc.spool(data_directory).update() + assert spool._inventory != 42 + assert spool._inventory._inventory is None + + def test_a_union_carries_it_over(self, data_directory, inventory): + """Both halves are resolved, so agreeing is a thing they can do.""" + spool = dc.spool(data_directory).update() + combined = spool + spool.attach_inventory(inventory) + assert combined.enrich()[0].attrs.gauge_length == 10.0 + + def test_a_union_of_two_different_ones_raises(self, data_directory, inventory): + """Two answers to one question have no combined meaning.""" + spool = dc.spool(data_directory).update() + other = _replace_acquisition(inventory, gauge_length=7.0) + with pytest.raises(InvalidSpoolError, match="different inventories"): + spool + spool.attach_inventory(other) + + class TestMultiValuedTrackFields: """A coordinate holds one value per channel, so containers cannot go in.""" From 043a2b3065221659eef4d7d239868fcd2cf3c279 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 18:00:13 +0200 Subject: [PATCH 2/5] Close what six reviewers found in the attachment Comparing two attachments must not read either of them. It did, and three of the consequences were the review's strongest finding: `==` could raise a parser error out of an unreadable file, so a spool could not be put in a list or rendered by pytest; `+` parsed and validated both operands' inventories to decide whether they agreed, so a union could fail on a metadata file before any data was touched; and the answer depended on whether something had happened to read one first, which is the thing spool equality documents itself as not doing. An attachment is now compared as the thing it is -- a place, or a value. The same place equals the same place, an equal inventory equals an equal inventory, and a place is no value until someone asks. The cost, taken deliberately: combining two archives which each carry their own inventory raises, since the union has two and neither describes the whole, and the message now says how to mean one of them. A document which does not parse is an invalid inventory. `.inventory` files are read the way the format reads its own object files, so the suffix picks the parser -- a JSON inventory loads where PyYAML is not installed, which the blessed name accepted and could not honour -- and a parse failure, an undecodable byte, or a field which is not named arrives as an InvalidInventoryError rather than as whatever the parser happened to raise. Three legs found that gap independently; every "malformed" fixture in the first pass used YAML which parsed. Also: a path is anchored when it is attached, since the read comes later and possibly from another directory or another process; the read is locked, because threads mapping over one spool all reach it at once and reading a large authoring directory once per worker is the cost the holder exists to avoid; and one spelling of what an inventory could state, rather than the gate deciding to read on one rule and the reader then reading under another. Ten mutations the tests used to survive now fail them, including the provenance wrapper deleted entirely, the blessed flag ignored when two attachments are compared, `OSError` dropped from the read, and `skip_hidden` turned off -- which the scanner test could not see, because the file it planted was no format the scanner would have indexed either way. Declined: comparing two references by resolving them when their paths differ, which is where this started; and sharing the seven-line `write_inventory` test helper across two test trees, which would couple them to import it. --- dascore/core/inventory_loader.py | 45 +++++- dascore/core/spool.py | 122 +++++++++------- tests/test_core/test_inventory_loader.py | 46 ++++++ tests/test_proc/test_proc_inventory.py | 169 ++++++++++++++++++++--- 4 files changed, 304 insertions(+), 78 deletions(-) diff --git a/dascore/core/inventory_loader.py b/dascore/core/inventory_loader.py index f5f4b2bae..ece55febe 100644 --- a/dascore/core/inventory_loader.py +++ b/dascore/core/inventory_loader.py @@ -76,7 +76,8 @@ _ENVELOPE_STEM = "inventory" # The name a data directory carries its own inventory under, in either -# form: the directory `.inventory/` or a file `.inventory.yaml`. Hidden, +# form: the directory `.inventory/` or a file naming its format -- +# `.inventory.yaml`, `.inventory.yml`, or `.inventory.json`. Hidden, # like `.dascore_index.sqlite3` beside it -- a companion the directory # keeps rather than content it holds -- which is also what keeps the file # scanner (whose `skip_hidden` defaults to True) from reading it as data. @@ -1494,6 +1495,24 @@ def load_directory(path: str | os.PathLike) -> Inventory: return Inventory(**(envelope or {}), resources=resources, networks=networks).check() +def _load_file(path: Path) -> Inventory: + """ + Load a whole inventory from one serialized document. + + The envelope of an authoring directory read on its own terms: same + parsers, same errors, so the single-file artifact ``to_yaml`` writes + and the directory it came from fail the same way when they fail. + """ + data = _read_object(path) + # `Inventory(**data)` would raise TypeError on a key which is not a + # string -- `1: 2` is legal YAML -- naming Python's calling + # convention rather than the document which broke the rule. + if named := sorted(f"{x!r}" for x in data if not isinstance(x, str)): + msg = f"{_quote(path)} holds fields which are not named: {', '.join(named)}." + raise InvalidInventoryError(msg) + return Inventory(**data).check() + + def _blessed_candidates(directory: Path) -> tuple[Path, ...]: """Every spelling of the blessed name, the directory form first.""" tree = directory / BLESSED_NAME @@ -1506,9 +1525,10 @@ def carries_inventory(directory: str | os.PathLike) -> bool: A few stats and nothing more: this is the question a spool asks when it opens a directory, so it must not read, parse, or judge what it - finds. Whether what is there is loadable -- or even whether the - directory says which of the two forms it means -- is - ``find_inventory``'s to answer, when something actually asks. + finds. Which of the two forms the directory means, or whether it + names a form at all, is ``find_inventory``'s to answer, when + something actually asks; whether what it names loads is the loader's, + later still, and nobody's until then. Parameters ---------- @@ -1520,7 +1540,15 @@ def carries_inventory(directory: str | os.PathLike) -> bool: def find_inventory(directory: str | os.PathLike) -> Path | None: """ - Return the inventory a directory carries, or None if it carries none. + Return the path of the inventory a directory carries, or None. + + Only the name is judged, never the contents: a path comes back + because something sits under the blessed name in a form that name + takes, not because it loads. Raises instead of answering when the + directory says two things at once -- both forms present -- or when + what sits there is the wrong kind of thing for the name it has, which + is a misspelling of the convention rather than a file which owes it + nothing. Parameters ---------- @@ -1582,6 +1610,13 @@ def inventory(source: Inventory | str | os.PathLike | None = None) -> Inventory: if isinstance(source, str | os.PathLike): if os.path.isdir(source): return load_directory(source) + # A serialized document is read the way the format reads its own + # object files: the suffix picks the parser, so a JSON inventory + # loads where PyYAML is not installed, and a file which does not + # parse says so as an invalid inventory rather than as whatever + # the parser happened to raise. + if os.path.isfile(source) and _object_suffix(Path(source)) is not None: + return _load_file(Path(source)) return Inventory.from_yaml(source) msg = f"Could not get an inventory from {source!r}." raise InvalidInventoryError(msg) diff --git a/dascore/core/spool.py b/dascore/core/spool.py index 202187673..08d1a47c3 100644 --- a/dascore/core/spool.py +++ b/dascore/core/spool.py @@ -5,6 +5,7 @@ import abc import inspect import os +import threading import warnings from collections.abc import Callable, Generator, Iterator, Mapping, Sequence from dataclasses import replace @@ -433,7 +434,10 @@ def _combine_state(values, label): if len(present) == 2 and present[0] != present[1]: msg = ( f"The spools carry different {label}, which have no combined " - "meaning. Attach one inventory to the combined spool instead." + "meaning. Attach one inventory to the combined spool instead, " + "or drop an operand's with Spool.remove_inventory -- which is " + "also the answer when neither was attached by hand and each " + "directory simply carries its own." ) raise InvalidSpoolError(msg) return present[0] @@ -452,26 +456,45 @@ class _InventoryRef: An inventory is an input, not a cache, so it is never re-read behind the caller's back: a file which changes under a running program is a - new input rather than a stale one, and `Spool.attach_inventory()` - with no argument is how the program says to read it again. A read - which failed is not a read, though, and is tried again next time -- - an unreadable inventory is a thing to go and fix, and holding the - failure would mean the fix could not be seen. + new input rather than a stale one, and re-attaching is how the + program says to read it again -- `Spool.attach_inventory()` with no + argument for the one a directory carries, the same path again for + any other. A read which failed is not a read, though, and is tried + again next time: an unreadable inventory is a thing to go and fix, + and holding the failure would mean the fix could not be seen. """ def __init__(self, path, blessed: bool = False): + # Anchored now, while the working directory is still the one the + # caller named it from: the read happens later, and a relative + # path would then be resolved against wherever the program had + # got to -- another directory, or another process entirely. # For a blessed reference this is the directory, not the file: # which of the two forms the directory carries is decided when it # is read, so discovery stays a stat and its complaints wait. - self.path = path + self.path = path.absolute() self.blessed = blessed self._inventory: Inventory | None = None + self._lock = threading.Lock() + + def __getstate__(self): + """A lock cannot be pickled, and a fresh one is what a copy wants.""" + return {k: v for k, v in self.__dict__.items() if k != "_lock"} + + def __setstate__(self, state): + self.__dict__.update(state) + self._lock = threading.Lock() def resolve(self) -> Inventory: """Return the inventory, reading it if this is the first ask.""" - if self._inventory is None: - self._inventory = self._read() - return self._inventory + # Held across the read, not merely around the assignment: threads + # mapping over one spool all reach this at once, and reading a + # large authoring directory once per worker is the cost this + # whole class exists to avoid. + with self._lock: + if self._inventory is None: + self._inventory = self._read() + return self._inventory def _read(self) -> Inventory: """Read the inventory, saying where an unreadable one came from.""" @@ -503,33 +526,26 @@ def _read(self) -> Inventory: def __eq__(self, other) -> bool: """ - Whether this and another attachment are the same inventory. + Whether this and another attachment are the same one. - Two references to one place are the same without either being - read; anything else is the inventories' own question, and - answering it is what makes a lazily attached spool comparable to - one holding the inventory itself. Something which is no kind of - inventory is not that question, and reading a file to answer it - would be a read nobody asked for. + An attachment is compared as the thing it is -- a place, or a + value -- rather than by what reading it would produce. Comparing + never reads, which is what keeps `==` and `+` from doing file + I/O, from raising out of an unreadable inventory, and from + answering differently depending on whether something happened to + read it first. So a place equals the same place, a value equals + an equal value, and a place is no value until someone asks. """ if isinstance(other, _InventoryRef): - if (self.path, self.blessed) == (other.path, other.blessed): - return True - elif not isinstance(other, Inventory): - return NotImplemented - return self.resolve() == _attached_inventory(other) + return (self.path, self.blessed) == (other.path, other.blessed) + if isinstance(other, Inventory): + return False + return NotImplemented # Defined because __eq__ is: a reference is spool state, never a key. __hash__ = None -def _attached_inventory(attachment) -> Inventory: - """Return the inventory an attachment holds, reading it if needed.""" - if isinstance(attachment, _InventoryRef): - return attachment.resolve() - return attachment - - def _combine_inventories(first, second) -> tuple: """ Return the (inventory, enrich kwargs) a union of two spools carries. @@ -1249,11 +1265,11 @@ def _inventory_query( Whether an inventory has anything to say here is settled without reading one: the observing-system facts are the models' own, the - same for every inventory, and a coordinate an inventory runs - along the fiber is by definition a name the index does not have. - So a query about what the index already knows leaves a lazily - attached inventory unread, and one naming anything else is - asking a question only the inventory can answer. + same for every inventory, and a name the index already carries + keeps the index's meaning even where an inventory could also + place it on the fiber. So a query about what the index already + knows leaves a lazily attached inventory unread, and one naming + anything else is asking a question only the inventory can answer. """ if self._inventory is None: return {}, set() @@ -1266,10 +1282,14 @@ def _inventory_query( ) if not outside: return {}, selectable + # `selectable` rather than the inventory's own attr names, which + # are the same set: one spelling of what an inventory could state + # keeps this from deciding to read on one rule and then reading + # under another. channels = self._channel_query( - self._resolved_inventory().get_names(), + self._resolved_inventory().get_names().coords, requested, - known_attrs, + known_attrs | known_coords | selectable, known_coords, _coords, kwargs, @@ -1277,7 +1297,7 @@ def _inventory_query( return channels, selectable def _channel_query( - self, names, requested, known_attrs, known_coords, _coords, kwargs + self, coord_names, requested, known, known_coords, _coords, kwargs ) -> dict: """ Return the selectors naming coordinates the inventory runs along @@ -1292,8 +1312,7 @@ def _channel_query( could also place it on the fiber, and an inventory must not quietly move a name out of the namespace it has always been in. """ - coords = set(names.coords) - known_coords - known = known_attrs | known_coords | set(names.attrs) + coords = set(coord_names) - known_coords candidates = (requested - known) | (_namespace_names(_coords) & coords) wanted = candidates & coords if not wanted: @@ -2031,17 +2050,20 @@ def _resolved_inventory(self) -> Inventory: """ The attached inventory itself, read now if it has not been. - Every use of an inventory goes through here, and only uses: the - cheap `self._inventory is None` says whether one is attached at - all, which is what lets a spool be opened, counted, ordered, + Every question answered *from* an inventory goes through here, + and nothing else reads one -- comparing two attachments, which is + the other thing a spool does with them, deliberately does not. + The cheap `self._inventory is None` says whether one is attached + at all, which is what lets a spool be opened, counted, ordered, chunked, and read without a lazily attached inventory ever being touched. Data access is never hostage to a metadata file. """ # Every caller is already behind that cheap check, one way or # another: asking an inventory question of a spool carrying none # is refused where the question is asked, in its own words. - assert self._inventory is not None - return _attached_inventory(self._inventory) + attached = self._inventory + assert attached is not None + return attached.resolve() if isinstance(attached, _InventoryRef) else attached def _enrichment(self): """Return how this spool enriches, or None if it does not.""" @@ -2382,8 +2404,9 @@ def from_directory(cls, path, index_path=None) -> Self: A directory which carries an inventory under the name ``.inventory`` — the authoring directory ``.inventory/`` or a - serialized ``.inventory.yaml`` — hands it to the spool, which - reads it at the first question only an inventory can answer. See + serialized ``.inventory.yaml``, ``.inventory.yml``, or + ``.inventory.json`` — hands it to the spool, which reads it at + the first question only an inventory can answer. See [`attach_inventory`](`dascore.core.spool.Spool.attach_inventory`). """ from dascore.io.index.catalog import FileResolver, PatchCatalog # noqa: PLC0415 @@ -2401,10 +2424,9 @@ def from_directory(cls, path, index_path=None) -> Self: ) else: out._catalog = PatchCatalog.from_directory(path, index_path=index_path) - # A directory which carries an inventory hands it to the spool - # over it, once, here: filling the slot at the moment the spool is - # opened is what makes `remove_inventory` stick, since nothing - # refills it afterwards. + # Filling the slot at the moment the spool is opened is what makes + # `remove_inventory` stick: nothing refills it afterwards, so no + # sentinel is needed to tell unset from deliberately emptied. out._inventory = out._blessed_inventory() return out diff --git a/tests/test_core/test_inventory_loader.py b/tests/test_core/test_inventory_loader.py index 4a760a2b2..73d56178d 100644 --- a/tests/test_core/test_inventory_loader.py +++ b/tests/test_core/test_inventory_loader.py @@ -2093,3 +2093,49 @@ def test_a_suffixed_directory(self, tmp_path): def test_the_name_is_hidden(self): """Which is what keeps the file scanner from reading it as data.""" assert loader.BLESSED_NAME.startswith(".") + + +class TestLoadSerializedFile: + """A whole inventory read from one document, not a directory.""" + + def test_json_needs_no_yaml(self, tmp_path, monkeypatch): + """The suffix picks the parser, so JSON is not YAML's to read.""" + path = tmp_path / "whole.json" + path.write_text('{"description": "a JSON inventory"}') + asked = [] + monkeypatch.setattr(loader, "optional_import", lambda x, **kw: asked.append(x)) + assert dc.inventory(path).description == "a JSON inventory" + assert not asked + + def test_a_document_which_does_not_parse(self, tmp_path): + """Which is an invalid inventory, not a parser's business.""" + path = tmp_path / "whole.yaml" + path.write_text("this: [is not: yaml\n") + with pytest.raises(InvalidInventoryError, match="Could not parse YAML"): + dc.inventory(path) + + def test_fields_which_are_not_named(self, tmp_path): + """`1: 2` is legal YAML, and names no field of anything.""" + path = tmp_path / "whole.yaml" + path.write_text("1: 2\n") + with pytest.raises(InvalidInventoryError, match="not named"): + dc.inventory(path) + + def test_a_document_which_is_not_a_mapping(self, tmp_path): + """A list of things defines no inventory.""" + path = tmp_path / "whole.yaml" + path.write_text("- 1\n- 2\n") + with pytest.raises(InvalidInventoryError, match="holds no mapping"): + dc.inventory(path) + + def test_a_missing_file_is_still_named(self, tmp_path): + """The message a caller who mistyped a path needs.""" + with pytest.raises(InvalidInventoryError, match="No such inventory file"): + dc.inventory(tmp_path / "absent.yaml") + + def test_a_round_trip(self, tmp_path): + """What `to_yaml` writes is what this reads.""" + original = dc.inventory(write_inventory(tmp_path / "authored", MINIMAL)) + path = tmp_path / "whole.yaml" + original.to_yaml(path) + assert dc.inventory(path) == original diff --git a/tests/test_proc/test_proc_inventory.py b/tests/test_proc/test_proc_inventory.py index 9878404c3..249bbc92f 100644 --- a/tests/test_proc/test_proc_inventory.py +++ b/tests/test_proc/test_proc_inventory.py @@ -4,10 +4,13 @@ import inspect import itertools +import os import pickle import re +import tempfile import warnings from collections.abc import Mapping +from pathlib import Path import numpy as np import pandas as pd @@ -22,6 +25,7 @@ enrich_on_missing_description, ) from dascore.core.inventory import ( + _SYSTEM_FACT_NAMES, Acquisition, CoordinateReferenceSystem, CouplingCondition, @@ -36,6 +40,7 @@ OpticalPathAnnotation, ) from dascore.core.inventory_loader import BLESSED_NAME +from dascore.core.spool import _InventoryRef from dascore.examples import get_example_patch, inventory_patch_pair from dascore.exceptions import ( CoordMergeError, @@ -606,6 +611,20 @@ def test_keeps_the_patches(self, patch, inventory): assert spool.remove_inventory()[0].equals(patch) +def _enforces_permissions() -> bool: + """Return True if a file with no mode bits is actually unreadable.""" + with tempfile.TemporaryDirectory() as name: + path = Path(name) / "probe" + path.write_text("") + path.chmod(0o000) + return not os.access(path, os.R_OK) + + +# Asked once, at collection: root ignores modes and Windows has none, so +# there the file a permission test means to make unreadable simply is not. +ENFORCES_PERMISSIONS = _enforces_permissions() + + def write_inventory(root, files): """Write a mapping of relative path to text as an authoring directory.""" for name, text in files.items(): @@ -649,9 +668,18 @@ def test_the_directory_form(self, tmp_path, patch): def test_the_scanner_ignores_it(self, tmp_path, patch): """A hidden name is one the file scanner already skips.""" dc.write(patch, tmp_path / "patch.h5", "dasdae") - write_inventory(tmp_path / BLESSED_NAME, {"inventory.yaml": "version: 0.0.1\n"}) - spool = dc.spool(tmp_path).update() - assert len(spool) == 1 + blessed = tmp_path / BLESSED_NAME + blessed.mkdir() + # A readable DAS file, which is the only thing that tells being + # skipped from being unreadable: a stray .yaml is no format the + # scanner would have indexed anyway. + dc.write(patch, blessed / "patch.h5", "dasdae") + (blessed / "inventory.yaml").write_text("schema_version: 1\n") + assert len(dc.spool(tmp_path).update()) == 1 + visible = tmp_path / "visible" + visible.mkdir() + dc.write(patch, visible / "patch.h5", "dasdae") + assert len(dc.spool(tmp_path).update()) == 2 def test_a_visible_inventory_is_not_attached(self, tmp_path, patch, inventory): """`inventory.yaml` names the envelope, not a spool's inventory.""" @@ -663,6 +691,12 @@ def test_in_memory_spools_carry_none(self, patch): """There is no directory to have carried one.""" assert dc.spool(patch)._inventory is None + def test_a_file_spool_carries_none(self, tmp_path, patch, inventory): + """A file is not a directory, whatever lies beside it.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + inventory.to_yaml(tmp_path / f"{BLESSED_NAME}.yaml") + assert dc.spool(tmp_path / "patch.h5")._inventory is None + class TestLazyInventory: """Attaching states where an inventory is; reading it waits.""" @@ -688,8 +722,14 @@ def test_nothing_is_read_by_unselecting_on_the_index(self, data_directory): assert spool._inventory._inventory is None def test_the_attr_names_are_the_models_own(self, inventory): - """What `_inventory_query` decides without reading rests on this.""" - assert inventory.get_names().attrs == dc.inventory().get_names().attrs + """What `_inventory_query` decides without reading rests on this. + + It gates on a constant rather than on the attached inventory, so + an inventory whose attrs depended on its contents would make the + gate skip reads it needs. + """ + assert set(inventory.get_names().attrs) == set(_SYSTEM_FACT_NAMES) + assert set(dc.inventory().get_names().attrs) == set(_SYSTEM_FACT_NAMES) def test_a_fiber_coordinate_reads_it(self, data_directory): """A name the index does not have is a question for the inventory.""" @@ -768,6 +808,35 @@ def test_an_edit_is_not_seen_until_it_is_asked_for( assert spool.enrich()[0].attrs.gauge_length == 10.0 assert spool.attach_inventory().enrich()[0].attrs.gauge_length == 2.0 + def test_a_syntax_error_says_where_it_came_from(self, tmp_path, patch): + """A file which does not parse is an inventory which does not load.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + (tmp_path / f"{BLESSED_NAME}.yaml").write_text("this: [is not: yaml\n") + spool = dc.spool(tmp_path).update() + with pytest.raises(InvalidInventoryError, match="when the spool was opened"): + spool.enrich()[0] + + def test_fields_which_are_not_named(self, tmp_path, patch): + """`1: 2` is legal YAML and no inventory; the document is blamed.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + (tmp_path / f"{BLESSED_NAME}.yaml").write_text("1: 2\n") + spool = dc.spool(tmp_path).update() + with pytest.raises(InvalidInventoryError, match="not named"): + spool.enrich()[0] + + @pytest.mark.skipif( + not ENFORCES_PERMISSIONS, reason="an unreadable file is readable here" + ) + def test_an_unreadable_file_says_where_it_came_from(self, tmp_path, patch): + """A permission error is no more the caller's to decipher.""" + dc.write(patch, tmp_path / "patch.h5", "dasdae") + path = tmp_path / f"{BLESSED_NAME}.yaml" + path.write_text("schema_version: 1\n") + spool = dc.spool(tmp_path).update() + path.chmod(0o000) + with pytest.raises(InvalidInventoryError, match="when the spool was opened"): + spool.enrich()[0] + def test_a_failed_read_is_not_a_read(self, tmp_path, patch, inventory): """An unreadable inventory is a thing to fix, so the fix is seen.""" dc.write(patch, tmp_path / "patch.h5", "dasdae") @@ -792,7 +861,13 @@ def test_removing_it_sticks(self, data_directory): def test_update_keeps_it(self, data_directory): """Re-indexing the files says nothing about the inventory.""" - assert dc.spool(data_directory).update().update()._inventory is not None + spool = dc.spool(data_directory) + assert spool.update()._inventory is spool._inventory + + def test_update_does_not_bring_it_back(self, data_directory): + """Removal outlives a re-index, since nothing refills the slot.""" + spool = dc.spool(data_directory).update().remove_inventory() + assert spool.update()._inventory is None class TestAttachInventoryPath: @@ -831,13 +906,24 @@ def test_a_path_which_is_not_there(self, patch): dc.spool(patch).attach_inventory("nowhere.yaml") def test_a_named_failure_says_which_file(self, tmp_path, patch): - """Named by hand, so the name is what the message needs.""" + """Named by hand, and still said to have been attached here.""" path = tmp_path / "broken.yaml" path.write_text("[]\n") spool = dc.spool(patch).attach_inventory(path) - with pytest.raises(InvalidInventoryError, match=r"broken\.yaml"): + with pytest.raises(InvalidInventoryError, match="attached to this spool from"): spool.enrich()[0] + def test_a_path_is_anchored_when_it_is_attached(self, tmp_path, patch, inventory): + """The read comes later, possibly from another directory entirely.""" + inventory.to_yaml(tmp_path / "somewhere.yaml") + here = os.getcwd() + os.chdir(tmp_path) + try: + spool = dc.spool(patch).attach_inventory("somewhere.yaml") + finally: + os.chdir(here) + assert spool.enrich()[0].attrs.gauge_length == 10.0 + def test_enrich_takes_a_path(self, tmp_path, patch, inventory): """Both inventory verbs attach what they are given.""" path = tmp_path / "somewhere.yaml" @@ -869,43 +955,80 @@ def test_re_attaching_clears_enrichment(self, data_directory): class TestLazyInventoryEquality: - """Two spools agree about an inventory whether or not they read it.""" + """Comparing two attachments never reads either of them.""" def test_two_references_to_one_place(self, data_directory): - """Which needs no read at all.""" + """The same place is the same attachment, unread.""" first, second = dc.spool(data_directory), dc.spool(data_directory) assert first.update() == second.update() assert first._inventory._inventory is None - def test_a_reference_and_the_inventory_itself(self, data_directory, inventory): - """One side is resolved to answer, since a name is not a value.""" + def test_two_references_to_different_places(self, tmp_path, patch, inventory): + """Two places are two attachments, whatever they hold.""" + spools = [] + for name in ("first", "second"): + root = tmp_path / name + root.mkdir() + dc.write(patch, root / "patch.h5", "dasdae") + inventory.to_yaml(root / f"{BLESSED_NAME}.yaml") + spools.append(dc.spool(root).update()) + assert spools[0] != spools[1] + assert all(x._inventory._inventory is None for x in spools) + + def test_a_place_and_a_value(self, data_directory, inventory): + """A place is no value until someone asks, and this does not ask.""" spool = dc.spool(data_directory).update() - assert spool == spool.attach_inventory(inventory) + assert spool != spool.attach_inventory(inventory) + assert spool._inventory._inventory is None - def test_a_reference_and_a_different_inventory(self, data_directory, inventory): - """The read decides it, and decides against.""" + def test_a_directory_and_the_inventory_it_carries(self, data_directory): + """One path, two things: the data directory and an inventory.""" spool = dc.spool(data_directory).update() - other = _replace_acquisition(inventory, gauge_length=7.0) - assert spool != spool.attach_inventory(other) + named = spool.attach_inventory(data_directory / f"{BLESSED_NAME}.yaml") + assert spool._inventory != named._inventory + assert spool._inventory != _InventoryRef(data_directory) def test_something_which_is_no_kind_of_inventory(self, data_directory): - """Not a question worth reading a file to answer.""" + """Which the comparison declines rather than answers.""" spool = dc.spool(data_directory).update() + assert spool._inventory.__eq__(42) is NotImplemented assert spool._inventory != 42 assert spool._inventory._inventory is None - def test_a_union_carries_it_over(self, data_directory, inventory): - """Both halves are resolved, so agreeing is a thing they can do.""" + def test_a_union_carries_one_over(self, data_directory): + """Both operands name the same place, so the union has an answer.""" spool = dc.spool(data_directory).update() - combined = spool + spool.attach_inventory(inventory) + combined = spool + spool.select(time=...) assert combined.enrich()[0].attrs.gauge_length == 10.0 def test_a_union_of_two_different_ones_raises(self, data_directory, inventory): """Two answers to one question have no combined meaning.""" spool = dc.spool(data_directory).update() - other = _replace_acquisition(inventory, gauge_length=7.0) with pytest.raises(InvalidSpoolError, match="different inventories"): - spool + spool.attach_inventory(other) + spool + spool.attach_inventory(inventory) + + def test_a_union_never_reads_an_inventory(self, tmp_path, patch): + """Data access is never hostage to a metadata file, `+` included.""" + spools = [] + for name in ("first", "second"): + root = tmp_path / name + root.mkdir() + dc.write(patch, root / "patch.h5", "dasdae") + (root / f"{BLESSED_NAME}.yaml").write_text("this: [is not: yaml\n") + spools.append(dc.spool(root).update()) + with pytest.raises(InvalidSpoolError, match="different inventories"): + spools[0] + spools[1] + assert len(spools[0] + spools[0].select(time=...)) == 1 + + def test_equality_never_raises(self, tmp_path, patch, inventory): + """`==` is asked in places which cannot take an exception.""" + broken, whole = tmp_path / "broken", tmp_path / "whole" + for root in (broken, whole): + root.mkdir() + dc.write(patch, root / "patch.h5", "dasdae") + (broken / f"{BLESSED_NAME}.yaml").write_text("this: [is not: yaml\n") + inventory.to_yaml(whole / f"{BLESSED_NAME}.yaml") + assert dc.spool(broken).update() not in [dc.spool(whole).update()] class TestMultiValuedTrackFields: From 51142bd908cc03281e57e457e4b637b9c44724b1 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 18:26:10 +0200 Subject: [PATCH 3/5] Refuse a document which is no inventory in one voice The verification pass found the fix half-applied: routing serialized documents through the authoring format's own reader covered the three suffixes that format knows, and `attach_inventory` takes a path of any name. So `attach_inventory("inventory.txt")` still let a parser error, an undecodable byte, or a field which is not named escape as whatever the parser raised, with nothing saying which file the spool meant. The rule belongs where every route already passes: `from_yaml` wraps its own read and parse, and the mapping a document parsed to is checked in one place both routes call. A caller who asked for an inventory should not have to know which parser was reaching for the file. --- dascore/core/inventory.py | 38 ++++++++++++++++++++---- dascore/core/inventory_loader.py | 9 +----- tests/test_core/test_inventory_loader.py | 33 ++++++++++++++++++++ tests/test_proc/test_proc_inventory.py | 15 ++++++++++ 4 files changed, 82 insertions(+), 13 deletions(-) diff --git a/dascore/core/inventory.py b/dascore/core/inventory.py index ef1de8c7a..79387924e 100644 --- a/dascore/core/inventory.py +++ b/dascore/core/inventory.py @@ -2301,11 +2301,39 @@ def from_yaml(cls, source) -> Self: if not os.path.exists(source): msg = f"No such inventory file: {source!r}." raise InvalidInventoryError(msg) - with open(source) as fh: - text = fh.read() + try: + with open(source) as fh: + text = fh.read() + except (OSError, UnicodeDecodeError) as error: + msg = f"Could not read {source!r}: {error}." + raise InvalidInventoryError(msg) from error yaml = optional_import("yaml", required_for="YAML inventory serialization") - data = yaml.safe_load(text) - if not isinstance(data, dict): - msg = f"Could not parse an inventory mapping from {source!r}." + try: + data = yaml.safe_load(text) + except yaml.YAMLError as error: + # A document which does not parse is an invalid inventory, and + # says so as one: a caller who asked for an inventory should + # not have to know which parser was reaching for the file. + msg = f"Could not parse YAML from {source!r}: {error}." + raise InvalidInventoryError(msg) from error + return cls._from_mapping(data, f"{source!r}") + + @classmethod + def _from_mapping(cls, data, source: str) -> Self: + """ + Build a checked inventory from one parsed document. + + Shared by every route into a whole inventory, so that a document + which is not one is refused in the same words whichever parser + read it. + """ + if not isinstance(data, Mapping): + msg = f"Could not parse an inventory mapping from {source}." + raise InvalidInventoryError(msg) + # `cls(**data)` would raise TypeError on a key which is not a + # string -- `1: 2` is legal YAML -- naming Python's calling + # convention rather than the document which broke the rule. + if named := sorted(f"{x!r}" for x in data if not isinstance(x, str)): + msg = f"{source} holds fields which are not named: {', '.join(named)}." raise InvalidInventoryError(msg) return cls(**data).check() diff --git a/dascore/core/inventory_loader.py b/dascore/core/inventory_loader.py index ece55febe..e94166e8c 100644 --- a/dascore/core/inventory_loader.py +++ b/dascore/core/inventory_loader.py @@ -1503,14 +1503,7 @@ def _load_file(path: Path) -> Inventory: parsers, same errors, so the single-file artifact ``to_yaml`` writes and the directory it came from fail the same way when they fail. """ - data = _read_object(path) - # `Inventory(**data)` would raise TypeError on a key which is not a - # string -- `1: 2` is legal YAML -- naming Python's calling - # convention rather than the document which broke the rule. - if named := sorted(f"{x!r}" for x in data if not isinstance(x, str)): - msg = f"{_quote(path)} holds fields which are not named: {', '.join(named)}." - raise InvalidInventoryError(msg) - return Inventory(**data).check() + return Inventory._from_mapping(_read_object(path), _quote(path)) def _blessed_candidates(directory: Path) -> tuple[Path, ...]: diff --git a/tests/test_core/test_inventory_loader.py b/tests/test_core/test_inventory_loader.py index 73d56178d..98d519b86 100644 --- a/tests/test_core/test_inventory_loader.py +++ b/tests/test_core/test_inventory_loader.py @@ -2128,6 +2128,39 @@ def test_a_document_which_is_not_a_mapping(self, tmp_path): with pytest.raises(InvalidInventoryError, match="holds no mapping"): dc.inventory(path) + # 0x81 is undefined in cp1252 as well as invalid UTF-8, so the file is + # undecodable wherever this runs rather than only where UTF-8 is the + # default -- a Windows checkout would otherwise decode it and parse on. + UNDECODABLE = b"description: \x81\n" + + def test_an_undecodable_file(self, tmp_path): + """Bytes which are no text are no inventory either.""" + path = tmp_path / "whole.yaml" + path.write_bytes(self.UNDECODABLE) + with pytest.raises(InvalidInventoryError, match="Could not read"): + dc.inventory(path) + + def test_an_undecodable_document_of_any_suffix(self, tmp_path): + """The route which reads YAML by default decodes alike.""" + path = tmp_path / "whole.txt" + path.write_bytes(self.UNDECODABLE) + with pytest.raises(InvalidInventoryError, match="Could not read"): + dc.inventory(path) + + def test_a_suffixless_document(self, tmp_path): + """Read as YAML, and refused in the same words when it is not.""" + path = tmp_path / "whole.txt" + path.write_text("this: [is not: yaml\n") + with pytest.raises(InvalidInventoryError, match="Could not parse YAML"): + dc.inventory(path) + + def test_a_suffixless_document_with_unnamed_fields(self, tmp_path): + """The one route into an inventory says this the same way.""" + path = tmp_path / "whole.txt" + path.write_text("1: 2\n") + with pytest.raises(InvalidInventoryError, match="not named"): + dc.inventory(path) + def test_a_missing_file_is_still_named(self, tmp_path): """The message a caller who mistyped a path needs.""" with pytest.raises(InvalidInventoryError, match="No such inventory file"): diff --git a/tests/test_proc/test_proc_inventory.py b/tests/test_proc/test_proc_inventory.py index 249bbc92f..6ccbd263b 100644 --- a/tests/test_proc/test_proc_inventory.py +++ b/tests/test_proc/test_proc_inventory.py @@ -913,6 +913,21 @@ def test_a_named_failure_says_which_file(self, tmp_path, patch): with pytest.raises(InvalidInventoryError, match="attached to this spool from"): spool.enrich()[0] + def test_a_named_file_of_any_suffix(self, tmp_path, patch, inventory): + """A path is a path; only the blessed name insists on a spelling.""" + path = tmp_path / "inventory.txt" + path.write_text(inventory.to_yaml()) + spool = dc.spool(patch).attach_inventory(path) + assert spool.enrich()[0].attrs.gauge_length == 10.0 + + def test_a_named_file_which_does_not_parse(self, tmp_path, patch): + """Whatever its suffix, and whichever parser reached for it.""" + path = tmp_path / "inventory.txt" + path.write_text("this: [is not: yaml\n") + spool = dc.spool(patch).attach_inventory(path) + with pytest.raises(InvalidInventoryError, match="attached to this spool from"): + spool.enrich()[0] + def test_a_path_is_anchored_when_it_is_attached(self, tmp_path, patch, inventory): """The read comes later, possibly from another directory entirely.""" inventory.to_yaml(tmp_path / "somewhere.yaml") From a73930ecf7780fbda0cca40c5f6d3b0f455ba5b1 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 18:42:44 +0200 Subject: [PATCH 4/5] Skip the tests which need a YAML writer where there is none The free-threaded and WebAssembly jobs run without PyYAML, which dascore supports: the rest of this module builds its inventories in memory and never noticed. These put one on disk. Verified by hiding PyYAML in the local environment rather than by reading the workflow: 9759 passed, 238 skipped, nothing failed. --- tests/test_proc/test_proc_inventory.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/test_proc/test_proc_inventory.py b/tests/test_proc/test_proc_inventory.py index 6ccbd263b..34bfdd956 100644 --- a/tests/test_proc/test_proc_inventory.py +++ b/tests/test_proc/test_proc_inventory.py @@ -10,6 +10,7 @@ import tempfile import warnings from collections.abc import Mapping +from importlib.util import find_spec from pathlib import Path import numpy as np @@ -611,6 +612,14 @@ def test_keeps_the_patches(self, patch, inventory): assert spool.remove_inventory()[0].equals(patch) +# These tests put an inventory on disk, which needs a YAML writer. The +# rest of this module builds them in memory, and dascore runs without +# PyYAML installed -- as the free-threaded and WebAssembly jobs do. +needs_yaml = pytest.mark.skipif( + find_spec("yaml") is None, reason="PyYAML is not installed" +) + + def _enforces_permissions() -> bool: """Return True if a file with no mode bits is actually unreadable.""" with tempfile.TemporaryDirectory() as name: @@ -643,6 +652,7 @@ def data_directory(tmp_path_factory, patch, inventory): return path +@needs_yaml class TestBlessedInventory: """A data directory hands its own inventory to the spool over it.""" @@ -698,6 +708,7 @@ def test_a_file_spool_carries_none(self, tmp_path, patch, inventory): assert dc.spool(tmp_path / "patch.h5")._inventory is None +@needs_yaml class TestLazyInventory: """Attaching states where an inventory is; reading it waits.""" @@ -870,6 +881,7 @@ def test_update_does_not_bring_it_back(self, data_directory): assert spool.update()._inventory is None +@needs_yaml class TestAttachInventoryPath: """An inventory can be attached by name as well as by value.""" @@ -969,6 +981,7 @@ def test_re_attaching_clears_enrichment(self, data_directory): assert spool.attach_inventory()._enrich_kwargs is None +@needs_yaml class TestLazyInventoryEquality: """Comparing two attachments never reads either of them.""" From 2930fb28ae20cbeaec635531c461c222407a48ee Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 18:49:49 +0200 Subject: [PATCH 5/5] Make the JSON test able to fail JSON is legal YAML, so a fallback to the YAML route would have parsed the fixture and passed a test which only watched the loader's own import. Both bindings now refuse, which the suffix routing has to not need: disabling that routing fails this test, where before it did not. Found by CodeRabbit on the PR. --- tests/test_core/test_inventory_loader.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/test_core/test_inventory_loader.py b/tests/test_core/test_inventory_loader.py index 98d519b86..9970d42ed 100644 --- a/tests/test_core/test_inventory_loader.py +++ b/tests/test_core/test_inventory_loader.py @@ -2102,10 +2102,16 @@ def test_json_needs_no_yaml(self, tmp_path, monkeypatch): """The suffix picks the parser, so JSON is not YAML's to read.""" path = tmp_path / "whole.json" path.write_text('{"description": "a JSON inventory"}') - asked = [] - monkeypatch.setattr(loader, "optional_import", lambda x, **kw: asked.append(x)) + + def _refuse(name, **kwargs): + raise MissingOptionalDependencyError(name) + + # Both bindings, because JSON is legal YAML: a fallback to the + # YAML route would parse this file and pass a test which only + # watched the loader's own import. + monkeypatch.setattr(loader, "optional_import", _refuse) + monkeypatch.setattr(inv, "optional_import", _refuse) assert dc.inventory(path).description == "a JSON inventory" - assert not asked def test_a_document_which_does_not_parse(self, tmp_path): """Which is an invalid inventory, not a parser's business."""