Skip to content

feat(hashing): add populate_hash_cache() and orcapod warm-cache CLI (ITL-489)#208

Merged
eywalker merged 25 commits into
mainfrom
eywalker/itl-489-cache-file-hashes-for-large-ephys-files-500mb-in-spike-sort
Jul 3, 2026
Merged

feat(hashing): add populate_hash_cache() and orcapod warm-cache CLI (ITL-489)#208
eywalker merged 25 commits into
mainfrom
eywalker/itl-489-cache-file-hashes-for-large-ephys-files-500mb-in-spike-sort

Conversation

@kurodo3

@kurodo3 kurodo3 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds populate_hash_cache() utility in src/orcapod/hashing/cache_population.py that recursively walks a directory, skips files below a configurable size threshold (default 500 MB), checks the SQLite hash cache first, and hashes+caches qualifying files using the existing FileHasher and SqliteHashCacher from ITL-472.
  • Adds CachePopulationStats frozen dataclass with full throughput accounting: hashed, already_cached, skipped_small, errors, total_bytes_hashed, total_duration, avg_hashing_speed.
  • Adds orcapod warm-cache PATH [--min-size FLOAT] [--db-path PATH] [--algorithm TEXT] [--buffer-size INT] [--workers INT] CLI subcommand via typer>=0.12.
  • Registers orcapod as an installable entry point in [project.scripts].
  • Exports CachePopulationStats and populate_hash_cache from orcapod.hashing.
  • Concurrent hashing via ThreadPoolExecutormax_workers: int = 4 (default) dispatches per-file stat/cache-lookup/hash/write to worker threads; main thread owns all DFS traversal and DB-file exclusion. SqliteHashCacher is safe to share across threads (uses threading.local() connections internally); FileHasher is stateless. Pass max_workers=1 to restore serial behaviour.
  • 19 unit tests for the core function (including 3 concurrency tests), 6 CLI tests (including --workers smoke test).

Test plan

  • uv run pytest tests/test_hashing/test_cache_population.py -v — all 19 tests pass
  • uv run pytest tests/test_cli/test_warm_cache.py -v — all 6 tests pass
  • uv run pytest tests/ -x -q — full suite passes
  • uv run orcapod warm-cache --help — shows PATH + all options including --workers INT with default 4
  • uv run orcapod warm-cache /tmp --min-size 0 --db-path /tmp/test.db --workers 4 — runs cleanly, prints summary

Fixes ITL-489

🤖 Generated with Claude Code

kurodo3 Bot and others added 13 commits July 3, 2026 07:26
Spec for populate_hash_cache() utility and orcapod warm-cache CLI
subcommand that pre-populates the SQLite file-hash cache for large
ephys recordings (>500 MB threshold by default).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
6-task TDD plan covering CachePopulationStats, populate_hash_cache(),
orcapod.hashing exports, typer dependency, warm-cache CLI subcommand,
and end-to-end smoke test.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d stats (ITL-489)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…L-489)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hash_cache

- Wrap SqliteHashCacher in a `with` block so the thread-local SQLite
  connection is explicitly closed when populate_hash_cache() returns.
- Remove redundant UPath() wrapper around entry.resolve() (returns UPath
  for local paths already).
- Add CachePopulationStats and populate_hash_cache to hashing/__init__.py
  module docstring for consistency.
- Tighten CLI test assertion to "500 MB" instead of bare "500".

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.78014% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/orcapod/hashing/cache_population.py 87.73% 13 Missing ⚠️

📢 Thoughts on this report? Let us know!

kurodo3 Bot and others added 6 commits July 3, 2026 16:55
…ate_hash_cache

Implements max_workers parameter to enable concurrent per-file hashing using ThreadPoolExecutor.
Directory traversal runs on the main thread while per-file work (stat, cache lookup, hashing,
cache write) is dispatched to a configurable number of worker threads. Defaults to 4 workers,
well-suited for NAS/HDD storage where seek contention occurs beyond 4 parallel reads. Also
excludes SQLite database files (.db, .db-wal, .db-shm) from hashing to prevent hashing
cache database files themselves.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rial

After the serial run, cache1.db and its WAL/SHM siblings exist inside
tmp_path. The concurrent run's _excluded set only covers cache2.db, so
it picked up cache1.db as a data file, causing an assert 2 == 5 failure.

Fix: scan data_dir (tmp_path/data) and store both DBs in a sibling
db_dir (tmp_path/dbs) that is never inside the scan root.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t preconditions

- Use Literal["hashed","cached","skipped_small","error"] as the first
  element of _hash_one's return type so static analysis can verify the
  as_completed dispatch block is exhaustive.
- Add self-verifying precondition assertions to
  test_max_workers_1_matches_serial so a silent mis-exclusion bug
  (both runs hashing 0 files) cannot make the test pass vacuously.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Exposes the max_workers parameter of populate_hash_cache() so operators
can tune hashing concurrency from the command line.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rsal edge cases

- Add test for passing a file (not a directory) as PATH — covers lines 67-68
- Add tests asserting speed line shown/hidden based on hashed count
- Add test verifying summary output contains all four count fields
- Add test asserting symlinked directories are not followed (only file symlinks were tested before)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI left a comment

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.

Pull request overview

Adds a new hash-cache “warming” utility and CLI so large files can be pre-hashed and stored in the existing SQLite cache (with optional concurrent hashing to improve throughput on NAS/HDD workloads).

Changes:

  • Introduces populate_hash_cache() + CachePopulationStats for recursive directory hashing/caching with throughput stats and max_workers concurrency.
  • Adds orcapod warm-cache Typer CLI (entry point + --workers) wrapping the cache-population utility.
  • Adds unit/CLI test coverage plus design/plan documentation and updates dependencies/lockfile.

Reviewed changes

Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
uv.lock Locks the newly added typer dependency.
pyproject.toml Adds typer>=0.12 and registers the orcapod CLI entry point.
src/orcapod/hashing/cache_population.py New cache population implementation with concurrent hashing and stats accounting.
src/orcapod/hashing/init.py Exports CachePopulationStats and populate_hash_cache from orcapod.hashing.
src/orcapod/cli/init.py Adds the Typer app root and registers the warm-cache command.
src/orcapod/cli/warm_cache.py Implements orcapod warm-cache including --workers and summary output.
tests/test_hashing/test_cache_population.py Adds comprehensive unit tests, including concurrency behavior and export checks.
tests/test_cli/init.py Marks CLI tests as a package.
tests/test_cli/test_warm_cache.py Adds CLI tests for help text, basic run, cache hits, defaults, and --workers.
superpowers/specs/2026-07-03-itl-concurrent-warm-cache-design.md Design doc for concurrent hashing changes (max_workers / --workers).
superpowers/specs/2026-07-03-itl-489-warm-cache-design.md Design doc for warm-cache utility + CLI.
superpowers/plans/2026-07-03-itl-concurrent-warm-cache.md Implementation plan/checklist for concurrency changes.
superpowers/plans/2026-07-03-itl-489-warm-cache.md Implementation plan/checklist for warm-cache feature set.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +142 to +149
_db_resolved = cacher.db_path.resolve()
_excluded: frozenset[Path] = frozenset(
[
_db_resolved,
_db_resolved.with_suffix(_db_resolved.suffix + "-wal"),
_db_resolved.with_suffix(_db_resolved.suffix + "-shm"),
]
)
Comment on lines +134 to +138
root = UPath(path)
_db_path: Path | None = Path(db_path) if db_path is not None else None
hasher = FileHasher(algorithm=algorithm, buffer_size=buffer_size)

with ThreadPoolExecutor(max_workers=max_workers) as executor:
…clusion

Two bugs reported in PR review:

1. ThreadPoolExecutor raises an unhelpful ValueError when max_workers < 1.
   Now validated eagerly in populate_hash_cache() with a clear message, and
   the warm-cache CLI prints "Error: --workers must be at least 1" and exits 1.

2. Sidecar paths were built with Path.with_suffix() which raises ValueError
   when the DB path has no file extension (e.g. --db-path ./cache). Also,
   the rollback-journal sidecar ("<db>-journal") was missing from the
   exclusion set. Both fixed by using string concatenation instead of
   with_suffix(), covering .db-wal, .db-shm, and .db-journal.

Tests added for both fixes (33 total, up from 30).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kurodo3

kurodo3 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Addressing review comment on sidecar path construction (#208 (comment)):

Fixed in 66e192e. Replaced all three with_suffix(suffix + "...") calls with plain string concatenation:

_db_str = str(_db_resolved)
_excluded: frozenset[Path] = frozenset([
    _db_resolved,
    Path(_db_str + "-wal"),
    Path(_db_str + "-shm"),
    Path(_db_str + "-journal"),
])

This handles DB paths with no file extension (where with_suffix("-wal") would raise ValueError) and also adds the previously missing rollback-journal sidecar (-journal). A new test test_db_path_without_extension in TestTraversal confirms a no-extension DB path works end-to-end.


Addressing review comment on max_workers validation (#208 (comment)):

Fixed in 66e192e. populate_hash_cache() now raises ValueError eagerly before creating the executor:

if max_workers < 1:
    raise ValueError(f"max_workers must be >= 1, got {max_workers!r}")

The CLI (warm_cache()) also validates before proceeding, printing Error: --workers must be at least 1 to stderr and exiting with code 1. Two tests were added: test_max_workers_zero_raises (API) and test_workers_zero_exits_nonzero (CLI).


Summary of all changes in this review round:

All fixes are in a single commit: 66e192ec

Issue Fix Tests added
with_suffix() ValueError on extension-less DB paths String concatenation for sidecar paths test_db_path_without_extension
Missing -journal sidecar in exclusion set Added Path(str(db) + "-journal") to _excluded (covered by above)
max_workers < 1 raises unhelpful error from executor Early ValueError in populate_hash_cache() + CLI guard test_max_workers_zero_raises, test_workers_zero_exits_nonzero

Total tests: 33 (up from 30), both source modules still at 100% line coverage.


def warm_cache(
path: str = typer.Argument(..., help="Root directory to scan recursively."),
min_size: float = typer.Option(

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.

Add an issue to follow up and allow for other units to be specified e.g. ("1 GB", "20 B", "30MiB")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Created Linear issue PLT-1913 to track this: https://linear.app/enigma-metamorphic/issue/PLT-1913/warm-cache-support-human-readable-size-strings-for-min-size-option

The issue covers supporting strings like "1 GB", "500 MB", "30 MiB" alongside bare floats (which will keep their MB interpretation for backward compatibility).

Comment thread src/orcapod/cli/warm_cache.py Outdated
typer.echo("Error: --workers must be at least 1", err=True)
raise typer.Exit(code=1)

root = Path(path)

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.

Shouldn't this be UPath? The root can be specified as UPath

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 64a7ffc. Changed root = Path(path) to root = UPath(path) and added from upath import UPath to the imports. The CLI now uses UPath consistently for path handling.

Comment thread src/orcapod/hashing/cache_population.py Outdated


def populate_hash_cache(
path: PathLike,

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.

this should be explicitly path: PathLIke | UPath to accept workingon UPath and internally we should consistently use UPath handling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 64a7ffc. Changed path: PathLike to path: PathLike | UPath. While UPath is technically already covered by os.PathLike (which UPath implements), making it explicit matches the existing pattern on the db_path parameter and makes the intent clear at a glance. Docstring updated accordingly.

if max_workers < 1:
raise ValueError(f"max_workers must be >= 1, got {max_workers!r}")

root = UPath(path)

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.

As we are accepting UPath here, shouldn't the type hint clearly mention that? I don't think os.PathLIke maps to UPath, does it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed together with the comment on line 93 (64a7ffc). The path parameter is now PathLike | UPath — explicit about UPath support, matching the db_path: UPath | PathLike | None pattern already used elsewhere in the same signature.

…ints

Address PR review from eywalker:

- populate_hash_cache(): path parameter type hint changed from PathLike
  to PathLike | UPath, making UPath acceptance explicit (matches the
  existing db_path: UPath | PathLike | None pattern). Docstring updated.
- warm_cache CLI: root = Path(path) → UPath(path) for consistency with
  the rest of the codebase. UPath import added.

Human-readable size strings for --min-size (e.g. "1 GB", "30 MiB")
tracked as PLT-1913 for a follow-up.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kurodo3

kurodo3 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Review round summary (eywalker, 64a7ffc)

Comment Action
Allow human-readable size strings ("1 GB", "30MiB") for --min-size Created PLT-1913 to track this as a follow-up. No code change in this PR.
root = Path(path) in warm_cache.py should use UPath Changed to UPath(path). Added from upath import UPath import.
path: PathLike in populate_hash_cache() should include UPath explicitly Changed to path: PathLike | UPath. Docstring updated.
Same UPath type-hint question re: db_path context Resolved by the same PathLike | UPath fix above.

All 33 tests continue to pass.

@eywalker eywalker requested a review from Copilot July 3, 2026 21:10

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.

Comment thread src/orcapod/hashing/cache_population.py Outdated
Comment on lines +172 to +176
futures = []
stack: list[UPath] = [root]
while stack:
current = stack.pop()
try:
Comment thread src/orcapod/hashing/cache_population.py Outdated
Comment on lines +77 to +81
resolved = entry.resolve()
stat = resolved.stat()
if stat.st_size < min_size_bytes:
return ("skipped_small", 0)
key = FileHashKey(resolved, stat.st_mtime_ns, stat.st_size)
kurodo3 Bot and others added 2 commits July 3, 2026 21:19
…_string, union types

- TestCombineHashes: covers truncation, prefix, order-independence (lines 82-89)
- TestHashFile: covers str path, missing file, invalid algorithm, hash_path algo
- TestIsInString: covers string-literal position detection (lines 177-184)
- test_clean_sparse/dense_union_type: covers both union branches in schema_cleaner
- test_get_default_arrow_hasher_returns_protocol_instance: covers defaults.py lines 62-63

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…und pending futures

Move stat() and size filtering onto the traversal thread so that only
qualifying files are ever submitted to the executor — avoids creating
a Future for every tiny file in a large tree.

Also cap the number of in-flight futures to max_workers * 4 using
wait(FIRST_COMPLETED), giving backpressure when hashing is slower than
traversal and keeping peak memory O(max_workers) rather than O(num_files).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kurodo3

kurodo3 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Response to Copilot review (round 2) — commit 112422e

Comment 1: Unbounded futures list → O(num_files) memory

Fixed. Replaced the unbounded futures list with a bounded pending set driven by wait(FIRST_COMPLETED). When len(pending) >= max_workers * 4, the traversal loop blocks until at least one future completes before submitting the next file. This keeps peak memory O(max_workers) rather than O(num_files) and provides natural backpressure when hashing is slower than traversal. The remaining futures are drained with as_completed once the DFS stack is exhausted.

Comment 2: Size filtering inside worker → futures created for every tiny file

Fixed. Moved resolve(), stat(), and the size check onto the traversal thread. Files below min_size_bytes now increment skipped_small directly and are never submitted to the executor — no Future object is created for them. As a bonus, qualifying files no longer pay a second stat() inside _hash_one: the os.stat_result obtained on the traversal thread is passed directly to the worker, which uses it to build the cache key (mtime_ns + size) without any duplicate syscall. The _hash_one signature changed accordingly (takes resolved: Path, file_stat: os.stat_result instead of entry: UPath, min_size_bytes: int) and its return type no longer includes "skipped_small".

All 33 targeted tests pass (22 unit + 11 CLI).

_hash_one now takes resolved: UPath instead of Path. The _excluded
frozenset is also UPath so entry.resolve() can be compared directly
without a Path() cast, keeping UPath throughout.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kurodo3

kurodo3 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in af2005e. _hash_one now takes resolved: UPath instead of Path. The _excluded frozenset is also typed as frozenset[UPath] (wrapping each member in UPath(...)), so entry.resolve() — which natively returns a UPath — can be compared directly without the Path() cast that was previously required to reconcile types. UPath is now consistent throughout: stack, entries, resolved paths, exclusion set, and the worker signature.

@eywalker eywalker merged commit a8e1551 into main Jul 3, 2026
11 checks passed
@eywalker eywalker deleted the eywalker/itl-489-cache-file-hashes-for-large-ephys-files-500mb-in-spike-sort branch July 3, 2026 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants