Skip to content

Refactor: extract per-key vs multi-bag layout strategy from BagOfHoldingH5FileBackend #760

Description

@pmrv

Location

src/fleche/storage/bagofholding_file.py:229-298_path, _lock_path,
_contains, _evict, list all switch on self.prefix_length == 0.

Problem

Since PR #746 (closes #556), BagOfHoldingH5FileBackend hosts two
structurally different disk layouts under one class:

  • prefix_length == 0: per-key files (root/{full_key}), inherited
    behaviour via super() calls.
  • prefix_length >= 1: multi-bag layout (root/{prefix}.h5, sibling
    groups named by full key via bagofholding 0.1.12's file.h5/group
    path addressing).

The branch on prefix_length == 0 is re-spelled in five methods:

def _path(self, key: str) -> Path:
    if self.prefix_length == 0:
        return super()._path(key)
    return self._bag_file(key) / key

def _lock_path(self, key: str) -> Path:
    if self.prefix_length == 0:
        return super()._lock_path(key)
    return Path(f"{self._bag_file(key)}.lock")

def _contains(self, key: Digest) -> bool:
    if self.prefix_length == 0:
        return super()._contains(key)
    # h5py.File open + membership check
    ...

def _evict(self, key: Digest) -> None:
    if self.prefix_length == 0:
        return super()._evict(key)
    # single-file lock + delete-group + garbage-collect empty file
    ...

def list(self) -> Iterable[Digest]:
    if self.prefix_length == 0:
        return [... per-key files ...]
    # walk multi-bag files, open each, collect keys
    ...

The two layouts have different invariants (per-key files vs. one
.h5 per prefix), different lock granularity (per-key .lock vs.
per-bag .lock), and different list() semantics (whole-file vs.
walk-and-open). Any new backend-level method (e.g. a future size,
atime, or migration hook) will require the same fork, and the two
layout invariants have to be re-learned from scratch each time.
PR #748's refix() and consolidate() already added a third
implicit layout concern (mixed-root consistency checks), and future
storage-level features would compound the branching.

Suggested refactor

Introduce a small _Layout strategy resolved in __post_init__ from
prefix_length:

class _Layout(Protocol):
    def path(self, root: Path, key: str) -> Path: ...
    def lock_path(self, root: Path, key: str) -> Path: ...
    def contains(self, root: Path, key: str) -> bool: ...
    def evict(self, root: Path, key: str, lock_timeout: float) -> None: ...
    def list(self, root: Path) -> Iterable[Digest]: ...

class PerKeyLayout: ...       # today's super()-inherited behaviour
class MultiBagLayout:
    def __init__(self, prefix_length: int): ...

The five backend methods become straight delegators:

def _path(self, key: str) -> Path:
    return self._layout.path(self.root, key)

refix() / consolidate() construct the other layout explicitly
via dataclasses.replace(self, prefix_length=n), which already
rebuilds _layout in __post_init__.

Alternative shape: two sibling backend classes
PerKeyBagOfHoldingH5File / MultiBagOfHoldingH5File selected by a
small module-level factory. Trade-off: cleaner classes, but the
config round-trip (storage_to_config) has to preserve the choice.

Pros

  • Eliminates five parallel if self.prefix_length == 0 branches.
  • Layout invariants (which files may appear in root, which lock file
    is per-key vs. per-bag, what list() walks) become properties of one
    object rather than being scattered.
  • Makes future features — atomic reindexing between layouts, a third
    layout (nested prefixes?), migrations — extensions of the strategy
    rather than a wider elif chain.
  • Composable with feat: refix() re-shards bagofholding prefix length; validate layout at init #748's check_consistency / refix / consolidate
    API: the layout object naturally owns the "which files are mine"
    predicate, which right now leaks into list() and refix().

Cons / risks

  • One more class hierarchy to learn; strategy pattern for a two-member
    enumeration can feel over-engineered until the third case arrives.
  • Frozen-dataclass friendliness: the strategy object must be
    reconstructible from prefix_length alone, so the config round-trip
    stays a single field. Not a real constraint but worth calling out.
  • Blast radius touches every disk-op method — well covered by the
    existing storage/test_bagofholding*.py suite and the
    smoke/test_optional_deps.py bagofholding round-trip, but the
    parametrised value_storage/call_storage fixtures sweep both
    layouts and will catch regressions.
  • No new external dependencies.

Complexity / maintainability

Medium. The refactor is behaviour-preserving, but the surface is
recent (PRs #746, #748, #749) and still evolving — worth doing before
a third layout variant lands, not after.

Out of scope


Filed from a codebase-wide refactoring scan; not yet implemented.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions