Skip to content

feat(hashing): add force, dry_run, progress_callback to populate_hash_cache (ITL-500)#210

Merged
eywalker merged 18 commits into
mainfrom
eywalker/itl-500-improve-populate_hash_cache-ergonomics-progress-callback-dry
Jul 5, 2026
Merged

feat(hashing): add force, dry_run, progress_callback to populate_hash_cache (ITL-500)#210
eywalker merged 18 commits into
mainfrom
eywalker/itl-500-improve-populate_hash_cache-ergonomics-progress-callback-dry

Conversation

@kurodo3

@kurodo3 kurodo3 Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds force=True, dry_run=True, and progress_callback parameters to populate_hash_cache(), and total_bytes_cached to CachePopulationStats
  • Refactors the internal implementation into a clean visitor/accumulator split (_HashVisitor, _DryRunVisitor, _Stats, _Accumulator) replacing loose local counter variables
  • Adds --dry-run and --force flags to orcapod warm-cache CLI with appropriate output formatting; adds cached GB to summary output
  • Exports FileOutcome and ProgressCallback from orcapod.hashing public API

New populate_hash_cache signature

def populate_hash_cache(
    path: PathLike | UPath,
    *,
    min_size_bytes: int = ...,
    db_path: UPath | PathLike | None = None,
    algorithm: str = "sha256",
    buffer_size: int = 65536,
    max_workers: int = 4,
    dry_run: bool = False,       # new
    force: bool = False,         # new
    progress_callback: ProgressCallback | None = None,  # new
) -> CachePopulationStats:

All three new parameters default to safe no-op values — fully backward compatible.

Test plan

  • 47 tests in tests/test_hashing/test_cache_population.py covering all new behaviours: TestForce, TestDryRun, TestVisitors, TestProgressCallback, TestCLI, TestCachedBytes
  • Full suite: 4140 passed, 56 skipped, 6 xfailed — no regressions
  • uv run python -c "from orcapod.hashing import FileOutcome, ProgressCallback; print('OK')" → OK

Fixes ITL-500

kurodo3 Bot and others added 16 commits July 5, 2026 00:38
…L-500)

Spec covers progress callback, dry-run mode, force re-hash, internal
_Stats accumulator, visitor/accumulator split, and CLI --dry-run/--force
flags.

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

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

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

The Returns block incorrectly stated that bytes_hashed is 0 for cached files,
but the implementation returns file_stat.st_size for both "hashed" and "cached"
outcomes. Only "error" returns 0.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… thread safety (ITL-500)

- Remove unused Callable import from typing (fixes ruff F401)
- Change _Accumulator.__init__ callback parameter from undefined "ProgressCallback | None" to None (fixes ruff F821)
- Document _Accumulator thread-safety constraint in class docstring
- Clarify _Stats.snapshot() docstring to explain fresh time.monotonic() reading

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add force parameter to populate_hash_cache to allow re-hashing files even if they're already in the cache. The parameter is passed through to _HashVisitor which already had the capability to skip cache lookups.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ant string quote (ITL-500)

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

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Expose the dry_run and force parameters of populate_hash_cache via the
warm-cache CLI. Update output to include cached GB in all runs and add
dry-run-specific messaging. Add 3 CLI integration tests via CliRunner.
@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.92806% with 14 lines in your changes missing coverage. Please review.

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

📢 Thoughts on this report? Let us know!

@eywalker eywalker requested a review from Copilot July 5, 2026 02:17

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

This PR extends OrcaPod’s hashing cache population workflow by adding ergonomic controls (force re-hash, dry-run scanning, and progress callbacks), tracking cached-byte totals, and wiring the new functionality through the orcapod warm-cache CLI and public hashing API.

Changes:

  • Added dry_run, force, and progress_callback to populate_hash_cache(), plus total_bytes_cached to CachePopulationStats.
  • Refactored cache population internals into visitor/accumulator components and added tests for the new behaviors.
  • Updated orcapod warm-cache CLI to expose --dry-run and --force, and to include cached GB in output; exported new types from orcapod.hashing.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/test_hashing/test_cache_population.py Adds extensive coverage for dry-run/force/progress callback behavior, CLI wiring, and cached-byte stats.
src/orcapod/hashing/cache_population.py Implements new API parameters and refactors logic into _Stats, _Accumulator, _HashVisitor, and _DryRunVisitor.
src/orcapod/hashing/init.py Exports FileOutcome and ProgressCallback from the public hashing package API.
src/orcapod/cli/warm_cache.py Adds --dry-run/--force flags and updates summary output to include cached GB.
superpowers/specs/2026-07-05-itl-500-populate-hash-cache-ergonomics-design.md Adds design documentation for the feature and internal architecture.
superpowers/plans/2026-07-05-itl-500-populate-hash-cache-ergonomics.md Adds a detailed implementation plan for ITL-500.

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

Comment thread src/orcapod/hashing/cache_population.py Outdated
Comment thread src/orcapod/hashing/cache_population.py Outdated
Comment on lines +273 to +280
progress_callback: Optional callable invoked once per qualifying file
(i.e. every file that passes the size filter, including those that
encounter stat or hashing errors). Receives the resolved ``Path``,
a ``FileOutcome`` string (``"hashed"``, ``"cached"``,
``"would_hash"``, or ``"error"``), and a frozen
``CachePopulationStats`` snapshot of running totals at that moment.
Not called for files below ``min_size_bytes`` or for directory
access errors. Defaults to ``None``.

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. The docstring now distinguishes two cases:

  • Most outcomes ("hashed", "cached", "would_hash", and hashing errors): the callback receives the resolved absolute UPath — symlinks followed, .. eliminated — the path the OS kernel uses to open the file.
  • Stat or resolve failures: the callback receives the original iterdir() entry. At the point the OSError is raised, resolved may not have been fully computed, so entry is passed instead. This path identifies where in the tree the failure occurred but may still contain unresolved symlink components from parent directories.

Note that hashing errors (where the visitor itself raises) still receive the resolved path, because resolved is stored in pending before the worker thread starts — the error only surfaces on drain.

Comment thread src/orcapod/hashing/cache_population.py Outdated
Comment on lines +268 to +270
dry_run: If ``True``, perform the full walk, stat, size filter, and cache
check but skip hashing and cache writes. ``stats.hashed`` reports how
many files *would* be hashed. Defaults to ``False``.

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. Added to the dry_run Args entry: "The walk runs serially on the calling thread regardless of max_workers; max_workers is ignored when dry_run=True.". The serial path was an intentional design choice — cache lookups are fast SQLite reads, and parallelising them would add complexity for negligible benefit. The docstring now makes that explicit so callers aren't surprised when max_workers=8 has no effect in dry-run mode.

@kurodo3

kurodo3 Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Review response (Copilot — round 1)

Three documentation/type issues addressed in commit a1f4718. No logic changes.

1. ProgressCallback path type corrected: PathUPath

ProgressCallback = Callable[[UPath, FileOutcome, CachePopulationStats], None]

The runtime always passes UPath instances. Changing the type annotation makes the public API contract accurate and avoids misleading type errors for callers who want to use UPath-specific features in their callbacks.

2. progress_callback docstring: distinguished resolved vs. unresolved path

The docstring now explicitly documents when callers receive a resolved path vs. the raw iterdir() entry:

  • Resolved UPath (symlinks followed, .. eliminated): all "hashed", "cached", "would_hash" outcomes, and hashing errors (where resolved is stored in pending before the worker starts).
  • Original iterdir() entry: stat and resolve failures — resolved may not have been computed yet when the OSError is raised, so entry is passed instead.

3. dry_run docstring: max_workers is ignored

Added: "The walk runs serially on the calling thread regardless of max_workers; max_workers is ignored when dry_run=True". The serial path is intentional — SQLite cache lookups are fast enough that a thread pool would add complexity for no benefit.


with ThreadPoolExecutor(max_workers=max_workers) as executor:
with SqliteHashCacher(_db_path) as cacher:
# Collect the DB file and its SQLite sidecar files so they are never

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.

These comments must be preserved

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.

Acknowledged. The thread-safety note in _HashVisitor's docstring ("Thread-safe: SqliteHashCacher uses threading.local() connections and FileHasher is stateless.") is intentional documentation of the concurrency contract and will not be removed.

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 6 out of 6 changed files in this pull request and generated 3 comments.

Comment thread src/orcapod/hashing/cache_population.py Outdated
self._stats = _Stats()
self._callback = callback

def record(self, path: Path, outcome: FileOutcome, nbytes: int) -> None:

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. Changed _Accumulator.record(path: Path, ...) to path: UPath. The type now matches ProgressCallback's contract end-to-end, so static checkers will no longer flag the forwarding call at self._callback(path, ...). The Path import is still needed for the _db_path local variable.

Comment thread src/orcapod/hashing/cache_population.py Outdated
Comment on lines +275 to +277
progress_callback: Optional callable invoked once per qualifying file
(i.e. every file that passes the size filter, including those that
encounter stat or hashing errors). Receives three arguments:

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. The opening sentence now reads: "Optional callable invoked once for each file that either meets the size threshold or fails at stat()/resolve() time (stat and resolve failures are reported before any size check)." The trailing note "Not called for files below min_size_bytes or for directory access errors" is unchanged and still accurate.

Comment on lines 304 to 306
_db_path: Path | None = Path(db_path) if db_path is not None else None
hasher = FileHasher(algorithm=algorithm, buffer_size=buffer_size)

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. FileHasher is now constructed inside the else: branch, just before _HashVisitor is instantiated — so a dry-run call never touches the hasher or triggers any algorithm validation.

- Move FileHasher construction into the non-dry-run branch; dry_run=True
  never uses the hasher so it should not be constructed unconditionally.
- Fix _Accumulator.record() path annotation: Path → UPath to match the
  ProgressCallback type contract.
- Correct progress_callback docstring: stat/resolve failures fire the
  callback before any size check, not only for size-filter-passing files.

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

kurodo3 Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Review round 3 — changes in 6dd8f35

Three issues addressed (all in src/orcapod/hashing/cache_population.py):

1. FileHasher constructed unconditionally even in dry-run mode
Moved hasher = FileHasher(algorithm=algorithm, buffer_size=buffer_size) inside the else: branch, just before _HashVisitor is instantiated. dry_run=True uses _DryRunVisitor which never touches the hasher, so there is no reason to construct it (or validate the algorithm) on dry-run calls.

2. _Accumulator.record() path: Path annotation should be path: UPath
Changed the parameter annotation from Path to UPath to match the ProgressCallback = Callable[[UPath, ...], None] contract end-to-end. Static checkers no longer flag the self._callback(path, ...) call. The Path import is still needed for the _db_path local variable.

3. progress_callback docstring opening sentence was inaccurate
The old wording said "every file that passes the size filter, including those that encounter stat errors" — but accumulator.record(entry, "error", 0) is called before the size check in the implementation. Reworded to: "Optional callable invoked once for each file that either meets the size threshold or fails at stat()/resolve() time (stat and resolve failures are reported before any size check)."

No other changes. 47 tests passing.

@eywalker eywalker merged commit 53c13af into main Jul 5, 2026
11 checks passed
@eywalker eywalker deleted the eywalker/itl-500-improve-populate_hash_cache-ergonomics-progress-callback-dry branch July 5, 2026 03:07
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