Repository navigation
Memoize readable fingerprints - #1941
CodyCBakerPhD wants to merge 10 commits into
Conversation
CodyCBakerPhD
changed the title
CodyCBakerPhD
added
internal
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1941 +/- ##
==========================================
+ Coverage 78.41% 78.63% +0.22%
==========================================
Files 92 94 +2
Lines 14130 14590 +460
==========================================
+ Hits 11080 11473 +393
- Misses 3050 3117 +67
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
relying on git-annex'ed key is a great idea, but I feel like it should be implemented within fscacher directly not bolted on dandi-cli. WDYT? |
|
@yarikoptic It's entirely up to you - looks like that should be perfectly fine |
a0b7aff to
9b6930e
Compare
then please submit it as PR against fscacher -- we should be able to manage a quick cycle/release there (I will also do some facelift there now -- seems to be CI having issues) |
Raised on con/fscacher#113 and adjusted 'here' to use dev state 'there' |
|
Generated by Claude Code |
8efbf33 to
da3d4d3
Compare
da3d4d3 to
aa299da
Compare
|
TODO: peel out the Annex fingerprinting and associated tests from 1933 to give this more of a use case |
yarikoptic
left a comment
yarikoptic
left a comment
There was a problem hiding this comment.
overall looks ok as a prerequuisite, but pay attention to BIDS side-car potential relevance, and likely should not appear in master independely of where/how it is actually used. May be adding git-annex support here would make it right away useful and pragmatically testable
fscacher's memoize_path derives its cache key from a stat of the file at the given path, so it never caches a call made on a `Readable` (there is nothing to stat). Some readables can nevertheless vouch for their content better than mtime/inode ever could, e.g. with a content digest such as a git-annex key: results computed from one such resource are valid for any other with the same fingerprint. - `Readable.get_fingerprint()`: new optional hook returning a content fingerprint string, `None` (the default) meaning "unknown, do not cache". - `pynwb_utils.memoize_source(cache, tokens)`: a decorator that behaves exactly like `cache.memoize_path` for path arguments and, for a `Readable` with a fingerprint, caches under (file name, fingerprint, tokens, other arguments) via `cache.memoize` instead. The tokens are passed explicitly since plain `memoize` does not add them by itself. - Applied to `get_metadata`, `get_neurodata_types`, and `nwb_has_external_links`, the memoized functions that already accept a `Readable`. No behavior change for paths or for readables without a fingerprint (still none of them at this point). Since the decorator preserves the signature, mypy now sees through `get_metadata`, whose return annotation claimed `dict | None` although it never returns `None`; fixed to `dict[str, Any]`. isort also reordered a pair of local imports in `RemoteReadableAsset.open()` while at it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA
`PersistentCache.memoize(exclude_kwargs=...)` only exists from fscacher 0.4 on; the declared floor is 0.3.0, where the argument is called `ignore`, so the lowest-deps job failed at import time. Pick the name from the signature. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA
joblib 1.3 (the declared floor) mistakes positional arguments beyond the
fingerprint ones for the keyword-only `_source` parameter of the memoized
function ("Keyword-only parameter '_source' was passed as positional
parameter"), which broke `validate(path, readable=...)` under lowest-deps.
Bind the decorated function's other arguments by name instead (defaults
included), which also makes the cache key independent of how they were
passed; the decorated function must not take *args as a consequence.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA
…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
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
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
fscacher 0.5.0 (with memoize_path(custom_fingerprint=...)) is now on PyPI, so drop the temporary git pin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
aa299da to
6b5aaaf
Compare
…ally Moved over from #1933 without its integration into `dandi validate`: dandi.support.annex reads the key of an annexed file from its (possibly broken) symlink and the URLs registered for it from the git-annex branch, using only git, and AnnexReadableFile streams the content from those URLs with fsspec. Its get_fingerprint() is the key, so results of the functions memoized with readable_fingerprint (metadata, ...) are cached under it and shared by files with the same key and name. Tests cover key and URL-log parsing, the git-annex branch lookup, streaming from file:// and HTTP(S) URLs, and caching by key, using fixtures that fake a DataLad Dandiset with plain git. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Per review: a fingerprint covers the bytes of one file only, not its location nor other files such as BIDS sidecars, so only results computed from the file alone (and its name, which readable_fingerprint adds) may be keyed by it. Also give the repeated-call assertion a message stating what it checks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
| file name); results that depend on other files must not be keyed by | ||
| it alone. | ||
| """ | ||
| return None |
There was a problem hiding this comment.
FYI this returns None in the base class because that is how all current non-annex Readable paths behave (only the annexed paths then override with a fingerprint in their class)
This is now more apparent since the annex class has been moved to this PR
|
@yarikoptic PR should make more sense now the annex support is added to it; still only affecting internal classes, no real outward expose or use until #1933 |
get_annex_key() re-implemented the parsing of the symlink into the git-annex object store, and accepted keys of any backend. A WORM or URL key does not pin the content, so AnnexReadableFile.get_fingerprint() could have served cached results for modified content. Use fscacher's annex_key_fingerprint(), which accepts content-hash backends only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
yarikoptic
left a comment
yarikoptic
left a comment
There was a problem hiding this comment.
we really do not want to get that deep into git-annex guts. E.g. to get URLs there is git annex whereis and that is what datalad-fuse uses I believe. Let's try to datalad-fuse's Python interfaces instead: if we run into performance issues, let's see how to address those but again IMHO without looking manually into git-annex branch ATM.
3 participants
Split out of #1933 as a generally useful pre-PR enhancing memoization of things like git-annex links
Requires fscacher >= 0.5.0 for
memoize_path(custom_fingerprint=...)(con/fscacher#113).Caching by content fingerprint
fscacher's
memoize_pathkeys its cache on astat()of the path argument, so it never caches a call made on a non-path-likeReadable(there is nothing to stat). Some readables can vouch for their content better than mtime and inode ever could, e.g. with a git-annex key.Readable.get_fingerprint()(dandi/misctypes.py): new optional hook returning a fingerprint of the resource's own content. The defaultNonemeans "unknown", so existingReadablesubclasses are unaffected.pynwb_utils.readable_fingerprint(source): returns(file name, fingerprint)for aReadablewith a fingerprint,Noneotherwise, for use ascustom_fingerprint. Paths and path-like readables (LocalReadableFile) keep being cached bystat()as before.get_metadata,get_neurodata_typesandnwb_has_external_links, the memoized functions that already accept aReadable. Each depends only on the one NWB file's own content (no sidecars or other files), so sharing results between files with the same content and name is safe.Streaming git-annex'ed content:
dandi.support.annexgit-annexbranch, using onlygit(neither git-annex nor DataLad needs to be installed).AnnexReadableFilestreams the content on demand from those URLs with fsspec (block cache suited to h5py's random access, first URL that opens wins);get_fingerprint()is the key. So metadata of a streamed file is cached under its key, and shared by files with the same key and name (e.g. across clones of a Dandiset).get_annex_readable(path)returns one for an annexed file, orNonefor anything else.Side effect:
get_metadata's return annotation claimeddict | Nonealthough it never returnsNone(fixed todict[str, Any]), andisortreordered a pair of local imports inRemoteReadableAsset.open().Tested locally with fscacher 0.5.0: the above pass;
test_bids_nwb_metadata_integrationfails withoutbids-validator-deno, as on master. flake8, black, isort and mypy clean.Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA
https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr