Skip to content

Add custom_fingerprint to memoize_path, and a git-annex key fingerprint - #113

Merged
CodyCBakerPhD merged 14 commits into
masterfrom
claude/content-fingerprint
Sep 29, 2026
Merged

CodyCBakerPhD merged 14 commits into
masterfrom
claude/content-fingerprint

Conversation

@CodyCBakerPhD

@CodyCBakerPhD CodyCBakerPhD commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Upstreams the mechanism from dandi/dandi-cli#1941, which otherwise has to reimplement it outside fscacher.

memoize_path keys its cache on a stat() of the path argument. Some resources can vouch for their content better, e.g. locked git-annex'ed files through their content-hash key, which also survives the file being moved or its content dropped; and non-path objects (e.g. dandi-cli's Readables) have no stat() at all.

Change

  • memoize_path(..., custom_fingerprint=None): an alternative to the built-in stat() fingerprint. The callable is called with the top-level argument only and returns a fingerprint, or None to fall back to stat(). The rest of the logic is shared: _get_fingerprint() returns a CustomFingerprint or a PathFingerprint (the stat() one with its path, keyed exactly as before, so existing caches stay valid), then the None → direct call, modified-recently window and key injection run once for both. A custom fingerprint has no modified-recently window: it must change whenever the result may.
  • Directories: the callable may return a fingerprint of the whole tree; if it returns None, the directory is walked and stat()-ed exactly as on master.
  • fscacher.annex_key_fingerprint(path, *, pair_with_path=True): fingerprints a locked annexed file (a symlink into .../annex/objects/.../KEY/KEY, read without calling git-annex) by its key, if the backend hashes the content (SHA*, SHA3_*, SKEIN*, BLAKE2*, MD5, ± E). Unlocked files (whose key goes stale when edited, incl. adjusted branches/Windows), WORM, URL/VURL and external keys fall back to stat(). The key is paired with the path by default. A locked file's cached result is returned even after its content is dropped.
  • Docs: a short README section and docstrings; the design decisions and the contract for callables (cheap, return None rather than raise for anything unrecognized, picklable with a stable repr(), equal fingerprints share results), plus edge cases such as Windows/WSL, are in docs/design/git-annex-content.md.

Deferred to follow-ups

  • Fingerprinting annexed files inside directories (e.g. Zarrs with dropped chunks), via per-entry fingerprints in the walk or a tree hash of committed directories.
  • A way for a callable to say "do not cache" (e.g. a fscacher.NO_CACHE constant); for now an exception from the callable propagates.

Tests

test_annex.py: the helper on a fake annex layout (backends accepted/rejected, pairing, relative paths, non-annexed and non-path values, dropped content), memoize_path end to end on it (twins across paths and extensions with and without pairing, dropped content, stat() fallbacks incl. an edited unlocked file, a directory left to the stat() walk), and against real git-annex when available (locked twins, unlock + edit, drop, WORM). Generic custom-fingerprint tests in test_cache.py. Locally: 50 passed, 2 skipped (no git-annex here); black, isort, flake8 clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

Some resources can vouch for their content better than a stat() of a path
ever could, e.g. objects carrying a content digest such as a git-annex key,
which need not be paths at all (and so are never cached today: realpath()
raises and memoize_path falls through to a plain call).

memoize_path() now takes an optional content_fingerprint callable.  Called
with the value of the first argument, it returns a picklable fingerprint of
that value's content, or None.  A value with a fingerprint is cached under
("content", fingerprint) plus the cache's tokens instead of under its path
and stat(); the value itself stays excluded from joblib's key, so it need
not be picklable.  The "modified just now" window does not apply, since the
content cannot change without its fingerprint changing.  Values without a
fingerprint are handled exactly as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
@codecov

codecov Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.95%. Comparing base (d4f228b) to head (4f89cd7).

Files with missing lines Patch % Lines
src/fscacher/tests/test_annex.py 96.61% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
+ Coverage   92.07%   94.95%   +2.88%     
==========================================
  Files           4        6       +2     
  Lines         530      813     +283     
  Branches       40       54      +14     
==========================================
+ Hits          488      772     +284     
  Misses         25       25              
+ Partials       17       16       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@CodyCBakerPhD CodyCBakerPhD self-assigned this Sep 28, 2026
CodyCBakerPhD pushed a commit to dandi/dandi-cli that referenced this pull request Sep 28, 2026
…ze_source

The fingerprint-keyed caching is being upstreamed to fscacher
(con/fscacher#113), where it fits into memoize_path itself: the source
argument is already excluded from joblib's key there, and the cache's
tokens are at hand.  That makes most of `memoize_source` unnecessary: the
separately named `by_fingerprint` function, passing tokens explicitly
(and the tokens refactor it required), the `exclude_kwargs`/`ignore`
shim, and binding the other arguments by name to dodge joblib < 1.4.

What remains is `readable_fingerprint()`, which returns (file name,
fingerprint) for a `Readable` that knows its fingerprint and `None`
otherwise, passed as `content_fingerprint` at the three call sites.

This also restores caching for fingerprint-less `LocalReadableFile`s:
being path-like, memoize_path has always cached them by their stat(), but
`memoize_source` routed every `Readable` away from it.

fscacher is pinned to the PR branch for now; to be replaced with a
version floor once it is released.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
@CodyCBakerPhD
CodyCBakerPhD marked this pull request as ready for review September 28, 2026 15:49
@CodyCBakerPhD

Copy link
Copy Markdown
Contributor Author

@yarikoptic As you say, mostly waiting on your facelift to fix CI here

@CodyCBakerPhD CodyCBakerPhD added the minor Increment the minor version when merged label Sep 28, 2026 — with Claude

Copy link
Copy Markdown
Contributor Author

The red checks here are not from this change:

All other test jobs pass, and codecov reports all new lines covered. Leaving these for the CI facelift mentioned above rather than widening this PR.


Generated by Claude Code

@yarikoptic yarikoptic added the release Create a release when this pr is merged label Sep 28, 2026
@yarikoptic

yarikoptic commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

facelift was finished, I will rebase now "I""you" ;) merged the master to take into account

Copy link
Copy Markdown
Contributor Author

Heads-up: I had just merged master (with #106) into the branch as a8ed903 (clean, tests pass locally). Feel free to rebase over it instead if you prefer linear history; I won't push to the branch in the meantime.


Generated by Claude Code

@yarikoptic yarikoptic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

THANK YOU! Overall - looks pretty good! And the main aspect from direct review -- the need to centralize the logic, as here I see the custom fingerprinting to be just an alternative to built in stat based one -- so ideally it should just swap one function with the provided one, and be done, the rest of the code should stay the same.

Also I think it might be worth to immediately develop here the target helper of fingerprinting based on annex keys, since that would "ground" this custom implementation and potentially drive some of its decisions. So, while thinking about that, I recalled that some annex backends should not even be considered; or likely we need to pair with the path (the argument) since otherwise if actual function is file path dependent (not just content; e.g. like smth loading from BIDS dataset from both data file and sidecar .json it deduces) -- we would return wrong cached result.

Here is more on that and related from me+claude:

The only consumer so far is a synthetic Blob, though. I'd rather develop a real fingerprint function together with the hook in this PR. A git-annex key-based one would be the obvious candidate, and it could be tested as part of fscacher and reused by dandi-cli and others. It could go in a follow-up PR, but doing it here is the best way to check that the hook is complete. Thinking it through already raises the points below.

Which keys can vouch for content

  • Only content-hash backends (SHA*, SHA3_*, SKEIN*, BLAKE2*, MD5, and their *E variants) pin the content. WORM keys are size + mtime + filename. URL/VURL keys identify a URL whose content may change. External X* backends are unknown. Those should not be trusted, at least not by default.
  • Only locked files qualify. For an unlocked file, the key stays the same after the file is edited (git annex lookupkey still reports the old one), so the key would serve stale results. The same goes for adjusted branches and Windows. All of these must fall back to stat().

Contract for the callable
It now runs on every call, including plain str/Path arguments that used to go straight to stat(), so it must be cheap and must return None, not raise, for anything it doesn't recognize. Today lambda x: x.digest makes any call with a plain path fail. The docstring should state this contract.

How it differs from the stat() mode

  1. Sharing scope. stat() mode already keys on the dereferenced path. For locked annex files that is the object file, so twins within a repo already share an entry. That holds even across extensions for non-E keys (c.dat and d.nwb stored under the same SHA256 key). Content keys widen the sharing to any location or clone. That's the point, but it's unsafe for functions whose result depends on the path or extension (suffix-dispatched readers, file names in metadata). Whether results are paired with the path should be an explicit choice, with pairing as the default.
  2. Missing content. After git annex drop, stat() mode can't fingerprint the broken symlink and calls the function directly. Content mode returns the cached result without the content being present. That's useful, but it is a behavior change and should be documented and tested.
  3. The modified-recently window is skipped. That's justified only for locked, hash-based keys.
  4. Directories. The hook sees only the top-level argument. For a directory with annexed files (e.g. a .zarr in a dandiset), a custom callable would have to fingerprint the whole tree itself. Otherwise it falls back to _get_dir_fingerprint, which follows symlinks: a single dropped file then disables caching for the whole directory, and annexed files' mtimes still feed the modified-recently window. Per-file content fingerprints inside the directory walk would address this. Even if that comes later, it should inform the shape of the hook now.

Tests

  • Run a real function end to end: the listed backends accepted or rejected, locked vs. unlocked (including an edited unlocked file), content present vs. dropped, twins across paths and extensions, and a directory with some files dropped.
  • Use a fake annex layout, so the tests run without git-annex, plus real git-annex tests where it's available.

Comment thread README.rst Outdated
Comment thread src/fscacher/cache.py Outdated
…annex key one

Per review:

- Rename `content_fingerprint` to `custom_fingerprint`: what a fingerprint
  vouches for is up to the callable.
- Make it a drop-in alternative to the stat()-based fingerprint rather than
  a separate code path: `_get_fingerprint()` returns either a
  `CustomFingerprint` or a `PathFingerprint` (the stat()-based one with its
  path, keyed exactly as before), and the rest of `memoize_path` (no
  fingerprint -> direct call, modified-recently window, key injection) is
  shared.  A custom fingerprint is never "modified recently": it is up to
  the callable to change it when the result may change.
- Also consult the callable for each entry met while fingerprinting a
  directory, so that e.g. a tree with some annexed files dropped can still
  be fingerprinted (such entries do not feed the modified-recently window).
- Document the callable's contract: it runs on every call, so it must be
  cheap and return None (fall back to stat()) rather than raise for what it
  does not recognize; results are shared by equal fingerprints, so include
  the path unless the result does not depend on it.
- Add `fscacher.annex_key_fingerprint`, a real such callable: it reads the
  link of a *locked* annexed file (no git-annex call) and returns its key if
  the backend hashes the content (SHA*, SHA3_*, SKEIN*, BLAKE2*, MD5, with
  or without E); unlocked files (whose key goes stale when edited, as on
  adjusted branches/Windows), WORM, URL/VURL and external keys fall back to
  stat().  It pairs the key with the path by default (`pair_with_path`), as
  results may depend on the path or extension.  Results for a locked file
  survive its content being dropped, which is documented and tested.
- Tests: the helper on a fake annex layout (accepted and rejected backends,
  pairing, non-annexed and non-path values, dropped content), memoize_path
  end to end on it (twins across paths and extensions with and without
  pairing, dropped content, stat() fallbacks, a directory with dropped
  files), and against real git-annex when available (locked twins, unlocked
  and edited, dropped, WORM).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

Copy link
Copy Markdown
Contributor Author

Addressed the review in c43232e:

  • Single flow: custom_fingerprint is now just an alternative to the stat() fingerprint (see the inline reply).
  • fscacher.annex_key_fingerprint(path, *, pair_with_path=True), developed here as the real consumer:
    • only reads the symlink (no git-annex call), so it is cheap and returns None for anything else;
    • accepts only locked files (a link into .../annex/objects/.../KEY/KEY) with content-hash backends (SHA*, SHA3_*, SKEIN*, BLAKE2*, MD5, ± E); unlocked files (incl. adjusted branches/Windows), WORM, URL/VURL and X* keys fall back to stat();
    • pairs the key with the path (absolute, as given) by default; pair_with_path=False shares results across locations/clones.
  • Contract documented in the docstring and README: called on every call (and per directory entry), must be cheap, must return None rather than raise; no modified-recently window for custom fingerprints; equal fingerprints share results, so include the path unless the result doesn't depend on it.
  • Missing content: a locked file's cached result is returned after git annex drop; documented and tested. A file whose content was never present is still called (and fails) every time.
  • Directories: the directory walk now also consults the callable per entry, so a tree with dropped annexed files is fingerprinted by their keys (and those entries don't feed the modified-recently window) instead of failing on the broken symlink.
  • Tests (test_annex.py): fake annex layout (backends accepted/rejected, pairing, dropped, non-annexed/non-path values; twins across paths and extensions with and without pairing; stat() fallbacks incl. an edited unlocked file; a directory with some files dropped, and changes to it) plus a real git-annex test (locked twins, unlock + edit, drop, WORM), skipped when git-annex is missing. Locally, with git-annex installed: 52 passed, 3 runs.

Generated by Claude Code

@CodyCBakerPhD CodyCBakerPhD changed the title Add content_fingerprint hook to memoize_path Add custom_fingerprint to memoize_path, and a git-annex key fingerprint Sep 28, 2026
CodyCBakerPhD pushed a commit to dandi/dandi-cli that referenced this pull request Sep 28, 2026
con/fscacher#113 renamed the `memoize_path` argument per review, and now
also consults it for each entry of a directory it fingerprints;
`readable_fingerprint` returns None for those plain paths, so nothing
changes for dandi.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
On Windows CI the temporary directory is on C: while the working
directory is on D:, so os.path.relpath() raised.  Test relative paths by
changing into the temporary directory instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Add project-level .claude/settings.json denying the GitHub MCP tools
that post, edit, or resolve comments and reviews on pull requests and
issues.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Comment thread src/fscacher/cache.py Outdated
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Comment thread src/fscacher/cache.py Outdated
@CodyCBakerPhD

Copy link
Copy Markdown
Contributor Author

facelift was finished, I will rebase now "I""you"

me Clauded-me with auto-watching enabled (Claude settings now forbid such noise)

Only content-hash backends (SHA*, SHA3_, SKEIN, BLAKE2*, MD5, and their *E variants) pin the content.

Yeah that was a baked in assumption that somehow was not made clear through all this

The same goes for adjusted branches and Windows.

Poor, poor Windows

Missing content. After git annex drop, stat() mode can't fingerprint the broken symlink and calls the function directly. Content mode returns the cached result without the content being present. That's useful, but it is a behavior change and should be documented and tested.

Oh, that's kind of nice, didn't think about that! TBH since my main use for this is 100% streaming I wasn't considering how it would work for dynamically fetching/dropping content

The modified-recently window is skipped. That's justified only for locked, hash-based keys.

Yup, as intended

Directories. The hook sees only the top-level argument. For a directory with annexed files (e.g. a .zarr in a dandiset), a custom callable would have to fingerprint the whole tree itself. Otherwise it falls back to _get_dir_fingerprint, which follows symlinks: a single dropped file then disables caching for the whole directory, and annexed files' mtimes still feed the modified-recently window. Per-file content fingerprints inside the directory walk would address this. Even if that comes later, it should inform the shape of the hook now.

Yeah... As per usual, the Zarr side of this will need a good deal more thought

For now early-exiting on detection of that case, can decide more in follow-ups

…EADME

Passing the DirEntry lets annex_key_fingerprint skip regular files
using the file type from the directory listing, without a readlink()
system call, so walking a large tree of mostly regular files costs no
more than before.  Also trim the README section on custom_fingerprint.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Record the design decisions trimmed from the docstrings and README:
the custom_fingerprint contract, which annex files are fingerprinted by
key and why, pair_with_path, dropped content, and directory walks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

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.

This file is for more of those lower-level design concepts for the AI to refer to (a human can perhaps glance and clarify major details)

It also contains thoughts/discussions of edge cases like WSL for reference

(hiding away in commit messages makes it a tad harder to introspect / search IMO)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in general -- I do not mind those. But that is where I feel adhering to some framework like spec-kit is benefitical since keeping such files in sync with the code is part of the process. Otherwise they can get "out of sync"

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.

what part of spec-kit keeps already-specified docs up-to-date?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

the /speckit-analyze

Sure but that doesn't work automatically 'on its own' and I bet it would work on design docs like these just fine; all one would need is a 'routine' (weekly? monthly? when releases are cut?) to regularly scan the repo with that command

Comment thread .claude/settings.json Outdated
Keep that configuration personal for now, as asked in review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
CodyCBakerPhD pushed a commit to dandi/dandi-cli that referenced this pull request Sep 29, 2026
Keep that configuration personal instead, as in con/fscacher#113.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Comment thread src/fscacher/cache.py Outdated
`fscacher.annex.annex_key_fingerprint`. It is called with the value
of the first argument, and with the `os.DirEntry` of each entry met
while fingerprinting a directory, and returns either a fingerprint or
`None` to fall back to ``stat()``.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While walking the dog yesterday I came up with ingenious idea for which I did not see an immediate use case here but who knows: there is also Ellipsis, so I thought we could make it explicit

  • None -- stops chaining, use original function: this would allow for callbacks to enforce full processing whenever user knows that such paths need to be processed fully (cute idea but I failed to come up immediately with immediate use case, FIFO pipes?)
  • Ellipsis -- fall back to stat (kinda -- follow three dots to the next one)

but oh well, if we decide later we need it, we could add that Ellipsis.... but also I guess raising an exception should do the same desired effect of not proceeding to stat?

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.

Don't think we really need it yet (a FIFO or device file is a problem for plain stat() caching too, not something specific to custom fingerprints in this PR)

On the ellipsis, I prefer explicit over implicit: if ever needed in a follow-up, I'd prefer a named constant, e.g. fscacher.NO_CACHE

Raising doesn't give that effect - an exception from custom_fingerprint propagates, so the decorated call fails.

I'd keep it that way rather than catch it, because such an exception is more likely a bug in the callable, which should fail loudly rather than silently disable caching (IMO)

Then we return None for anything not recognized

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment thread src/fscacher/cache.py Outdated
custom_fingerprint(e)
if custom_fingerprint is not None
else None
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unclear to me yet why we need to do this here as presumably this is the part of the original 'mstat' code path, so we wouldn't even get here if custom_fingerprint handles it... might be just a matter of documentation on how custom_fingerprint to interact with folder paths???

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.

Sure, the fingerprint function is now only called on the top-level argument, not on each entry inside a directory

Per-entry composition or tree-hash fingerprint for committed directories is now fully deferred to a follow-up

The directory walk is the stat() code path, which is only taken once
custom_fingerprint returned None for the directory, so it no longer
consults the callable per entry: a directory is fingerprinted as
before, unless the callable fingerprints the whole tree itself.
Per-entry fingerprints (e.g. for Zarrs with dropped annexed chunks)
are left as an open question in the design notes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

@yarikoptic yarikoptic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

overall I think it is good, just thta docs/design/git-annex-content.md might have diverged. make agent go through diff and it to ensure that it reflects the situation (or may be even remove by now? ;-) )

Comment thread src/fscacher/annex.py
key = parts[-1]
if not CONTENT_HASH_BACKEND_RE.fullmatch(key.split("-", 1)[0]):
return None
return (op.abspath(path), key) if pair_with_path else key

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

note to paranoid me (smth negging in the back of my mind) -- abspath does not resolve links!

❯ python -c 'import os.path; print(os.path.abspath("sub-qa_ses-20200102_acq-MPRAGE_T1w.nii.gz"))'
/home/yoh/datalad/dbic/QA/sub-qa/ses-20200102/anat/sub-qa_ses-20200102_acq-MPRAGE_T1w.nii.gz
❯ dg sub-qa_ses-20200102_acq-MPRAGE_T1w.nii.gz
get(ok): sub-qa/ses-20200102/anat/sub-qa_ses-20200102_acq-MPRAGE_T1w.nii.gz (file) [from origin...]                                                            
❯ python -c 'import os.path; print(os.path.abspath("sub-qa_ses-20200102_acq-MPRAGE_T1w.nii.gz"))'
/home/yoh/datalad/dbic/QA/sub-qa/ses-20200102/anat/sub-qa_ses-20200102_acq-MPRAGE_T1w.nii.gz
❯ python -c 'import os.path; print(os.path.abspath("./sub-qa_ses-20200102_acq-MPRAGE_T1w.nii.gz"))'
/home/yoh/datalad/dbic/QA/sub-qa/ses-20200102/anat/sub-qa_ses-20200102_acq-MPRAGE_T1w.nii.gz

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.

Yeh, many reasons why I personally prefer pathlib, which then has .resolve() as an action to be explicit

Comment thread src/fscacher/annex.py
all files with the same key, e.g., across clones of a dataset.
"""
try:
path = os.fsdecode(os.fspath(path))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

need to remember about this fsdecode... not sure I ever used it ...

Comment thread docs/design/git-annex-content.md Outdated
- **It runs on every call** of the decorated function, so it must be cheap. `annex_key_fingerprint` only
reads the symlink; it never runs git-annex.
- **Equal fingerprints share results, wherever they are.** The value of the
path argument is excluded from the cache key, so a fingerprint must include

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is wrong -- we do include path by default, don't we?

@CodyCBakerPhD CodyCBakerPhD Sep 29, 2026 •

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.

the doc was correct, but it didn't say who includes the path...

Now it says fscacher adds the path to the key only for plain stat() fingerprints

Clarify that a custom fingerprint must include the path itself (as
annex_key_fingerprint does by default), that exceptions from the
callable propagate, and that the paired path is not dereferenced.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
@CodyCBakerPhD
CodyCBakerPhD merged commit 4f13561 into master Sep 29, 2026
48 checks passed
@CodyCBakerPhD
CodyCBakerPhD deleted the claude/content-fingerprint branch September 29, 2026 18:29
CodyCBakerPhD pushed a commit to dandi/dandi-cli that referenced this pull request Sep 29, 2026
con/fscacher#113 was merged and released as 0.5.0 (tagged), and its
branch deleted, but the upload to PyPI failed; install from the tag
until 0.5.0 is on PyPI.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
yarikoptic pushed a commit to dandi/dandi-cli that referenced this pull request Sep 30, 2026
* Accept broken symlinks as path arguments of `dandi validate`

`dandi validate` already handles annexed files whose content has not been
fetched (the broken symlinks of a DataLad dataset) according to
`--missing-file-content`: `error`, `skip`, or `only-non-data`.  Those policies
were applied only when such a file was reached through its directory,
though: given directly on the command line, the file never got past argument
parsing, because `click.Path(exists=True)` follows the link and rejected it
with "does not exist".  Check the path with `lexists()` instead, so that

    dandi validate --missing-file-content=skip sub-01/sub-01.nwb

works the same as validating `sub-01/`, while paths that truly do not exist
are still rejected with the same error.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA

* Harden and move the broken-symlink-accepting path type

Address review on the `click.Path` subclass accepting broken symlinks:

- Rename `ExistingPath` to `PathOrBrokenSymlink`, which says what sets it
  apart, and move it to `dandi/cli/base.py` with the other reusable
  `ParamType`s.
- Force `exists=False` and refuse `exists=True` or `resolve_path=True`:
  the former would bring back click's own check, which rejects broken
  links, and the latter would replace an annexed link with its
  `.git/annex/objects/...` target.
- Match `click.Path.convert` more closely: skip the check for `-` when
  `allow_dash` applies, and build the message with gettext and
  `click.utils.format_filename()` as click does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

* Replace PathOrBrokenSymlink with a click.Path accepting a lexists flag

Per review: rather than a special-purpose type whose name raises "what
about a symlink that is not broken?", provide `dandi.cli.base.Path`, a
`click.Path` taking a `lexists` flag next to `exists`.  With
`lexists=True` a path must satisfy `os.path.lexists()`, so it may be a
symlink whose target is missing, which `exists=True` (following the link)
rejects.  `dandi validate` needs that so its `--missing-file-content`
policies (error, skip, only-non-data) also apply to an annexed file whose
content was not fetched when that file is given directly on the command
line, not only when reached via its directory.

`lexists=True` refuses `resolve_path=True`, which would replace such a
link with its `.git/annex/objects/...` target; `exists=True` keeps its
click meaning.  The docstring now describes only the behavior, leaving
the rationale to this message.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

* Deny Claude posting comments/reviews on GitHub PRs and issues

Add project-level .claude/settings.json denying the GitHub MCP tools
that post, edit, or resolve comments and reviews on pull requests and
issues.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

* Simplify docstring of cli.base.Path

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

* Rename cli.base.Path to LinkAwarePath and explain its purpose

Path clashed with pathlib.Path; the docstring now explains why a
symlink-aware path type is needed rather than how it works.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

* Focus LinkAwarePath docstring on its purpose; document lexists on __init__

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

* Use a plain f-string for the LinkAwarePath error message

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

* Remove project-level Claude settings

Keep that configuration personal instead, as in con/fscacher#113.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr

---------

Co-authored-by: Claude <noreply@anthropic.com>
yarikoptic pushed a commit to dandi/dandi-cli that referenced this pull request Sep 30, 2026
…ze_source

The fingerprint-keyed caching is being upstreamed to fscacher
(con/fscacher#113), where it fits into memoize_path itself: the source
argument is already excluded from joblib's key there, and the cache's
tokens are at hand.  That makes most of `memoize_source` unnecessary: the
separately named `by_fingerprint` function, passing tokens explicitly
(and the tokens refactor it required), the `exclude_kwargs`/`ignore`
shim, and binding the other arguments by name to dodge joblib < 1.4.

What remains is `readable_fingerprint()`, which returns (file name,
fingerprint) for a `Readable` that knows its fingerprint and `None`
otherwise, passed as `content_fingerprint` at the three call sites.

This also restores caching for fingerprint-less `LocalReadableFile`s:
being path-like, memoize_path has always cached them by their stat(), but
`memoize_source` routed every `Readable` away from it.

fscacher is pinned to the PR branch for now; to be replaced with a
version floor once it is released.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
yarikoptic pushed a commit to dandi/dandi-cli that referenced this pull request Sep 30, 2026
con/fscacher#113 renamed the `memoize_path` argument per review, and now
also consults it for each entry of a directory it fingerprints;
`readable_fingerprint` returns None for those plain paths, so nothing
changes for dandi.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
yarikoptic pushed a commit to dandi/dandi-cli that referenced this pull request Sep 30, 2026
con/fscacher#113 was merged and released as 0.5.0 (tagged), and its
branch deleted, but the upload to PyPI failed; install from the tag
until 0.5.0 is on PyPI.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
CodyCBakerPhD pushed a commit to dandi/dandi-cli that referenced this pull request Oct 6, 2026
…ze_source

The fingerprint-keyed caching is being upstreamed to fscacher
(con/fscacher#113), where it fits into memoize_path itself: the source
argument is already excluded from joblib's key there, and the cache's
tokens are at hand.  That makes most of `memoize_source` unnecessary: the
separately named `by_fingerprint` function, passing tokens explicitly
(and the tokens refactor it required), the `exclude_kwargs`/`ignore`
shim, and binding the other arguments by name to dodge joblib < 1.4.

What remains is `readable_fingerprint()`, which returns (file name,
fingerprint) for a `Readable` that knows its fingerprint and `None`
otherwise, passed as `content_fingerprint` at the three call sites.

This also restores caching for fingerprint-less `LocalReadableFile`s:
being path-like, memoize_path has always cached them by their stat(), but
`memoize_source` routed every `Readable` away from it.

fscacher is pinned to the PR branch for now; to be replaced with a
version floor once it is released.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
CodyCBakerPhD pushed a commit to dandi/dandi-cli that referenced this pull request Oct 6, 2026
con/fscacher#113 renamed the `memoize_path` argument per review, and now
also consults it for each entry of a directory it fingerprints;
`readable_fingerprint` returns None for those plain paths, so nothing
changes for dandi.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
CodyCBakerPhD pushed a commit to dandi/dandi-cli that referenced this pull request Oct 6, 2026
con/fscacher#113 was merged and released as 0.5.0 (tagged), and its
branch deleted, but the upload to PyPI failed; install from the tag
until 0.5.0 is on PyPI.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
CodyCBakerPhD pushed a commit to dandi/dandi-cli that referenced this pull request Oct 6, 2026
…ze_source

The fingerprint-keyed caching is being upstreamed to fscacher
(con/fscacher#113), where it fits into memoize_path itself: the source
argument is already excluded from joblib's key there, and the cache's
tokens are at hand.  That makes most of `memoize_source` unnecessary: the
separately named `by_fingerprint` function, passing tokens explicitly
(and the tokens refactor it required), the `exclude_kwargs`/`ignore`
shim, and binding the other arguments by name to dodge joblib < 1.4.

What remains is `readable_fingerprint()`, which returns (file name,
fingerprint) for a `Readable` that knows its fingerprint and `None`
otherwise, passed as `content_fingerprint` at the three call sites.

This also restores caching for fingerprint-less `LocalReadableFile`s:
being path-like, memoize_path has always cached them by their stat(), but
`memoize_source` routed every `Readable` away from it.

fscacher is pinned to the PR branch for now; to be replaced with a
version floor once it is released.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
CodyCBakerPhD pushed a commit to dandi/dandi-cli that referenced this pull request Oct 6, 2026
con/fscacher#113 renamed the `memoize_path` argument per review, and now
also consults it for each entry of a directory it fingerprints;
`readable_fingerprint` returns None for those plain paths, so nothing
changes for dandi.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
CodyCBakerPhD pushed a commit to dandi/dandi-cli that referenced this pull request Oct 6, 2026
con/fscacher#113 was merged and released as 0.5.0 (tagged), and its
branch deleted, but the upload to PyPI failed; install from the tag
until 0.5.0 is on PyPI.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor Increment the minor version when merged release Create a release when this pr is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants