docs: split the purity contract out of file semantics; digest CompletedProcess - #831
Conversation
…edProcess Addresses the four inline review threads on #797. - The "argument mutation" rules were stated as a file-specific caveat, but they are general: a mutated list argument behaves identically (verified — cold call mutates, hit leaves the argument untouched, and a mutated argument that *is* returned is stored post-mutation). Move the contract to a new `usage/purity` page covering keyed-as-passed, mutation not replayed, and side effects being cold-call-only; `file_semantics` keeps the path instance and links out. - Cross-reference destructuring from "Paths nested inside containers", since that is the mechanism the section is really about: only destructured children reach the path machinery, so `_DESTRUCTURERS` *is* the list of places a nested path gets content treatment. Explains the container list, the exact-types caveat, and the opaque-container section in one stroke. - Move the `CompletedProcess` digester out of `notebooks/Files.ipynb` and into `digest.py` as a match arm (args + returncode + stdout + stderr). `run()`'s result is neither a dataclass nor iterable, so it was `Indigestible` and every shell-wrapping function needed the same hand-rolled hook. Purely additive — those values raised before, so no stored digest changes and no `hash_version` bump. - Save the executed `Files.ipynb` (the notebook now demonstrates the built-in digest, no `add_hook` cell). Same for `PathsInContainers.ipynb`, which was added without a `docs/notebooks/` symlink, a toctree entry, or a slot in `test_notebooks.py` — all three now wired up. Also fixes a short title underline in `usage/helpers`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RascUYS7JgCMHHpXsYbPTf
|
Reproduced locally with CI's pinned
The three, and where they came from — both are your commits on
So every PR into I checked that suppressions clear it, anchored on the exact diagnostic lines ( The I've not pushed any of that: it's your in-flight code on One correction while I'm here: both PR bodies say "4 diagnostics" — that was me running an unpinned newer Generated by Claude Code |
| ``dataclasses`` and ``attrs`` classes. See :ref:`extending-destructurer` for | ||
| the mechanism and how to add your own container to it. | ||
|
|
||
| Within those, paths are found nested to any depth, as values *or* as dict |
There was a problem hiding this comment.
any depth might actually only apply when remaining_depth is 0 :[=].
There was a problem hiding this comment.
Checked this rather than trusting the wording — it holds at any remaining_depth, and the reason is worth stating, so 62a2c3c now says why instead of just asserting "any depth".
A Path matches no destructurer, so in _intern_rec it falls through the _DESTRUCTURERS loop with depth still at float("inf"). The inline test is if depth < self.remaining_depth, and inf is never less than anything — so a path is always written out via super().save(...), which is precisely where PathValueMixin intercepts it. remaining_depth only chooses inline-vs-separate-entry for nodes that are destructurable; it has no lever on a path.
(It also propagates: a container holding a path gets 1 + inf, so the parents above it are written out separately too.)
Verified empirically — remaining_depth ∈ {0, 1, 3, 10} × nesting depth ∈ {0, 1, 2, 3, 5, 8}, alternating list/dict/dataclass wrappers, deleting the original file before loading so only content addressing can survive. All 24 combinations materialize with the right content.
Added to the docs:
"Any depth" is not a figure of speech, and no storage setting narrows it. A
Pathmatches no destructurer, so it is always written out as its own stored entry rather than inlined into the container above it — and being written out is exactly what hands it to the content machinery. Theremaining_depthknob only decides how eagerly destructurable nodes are split into separate entries, so it cannot put a path out of reach however it is set.
Generated by Claude Code
| So treat received paths as read-only and write outputs to a fresh directory | ||
| (``tempfile.mkdtemp``). If the mutation is the point, return the path: a | ||
| mutated argument that *is* returned is stored in its final, post-mutation | ||
| state. |
There was a problem hiding this comment.
second clause is correct and intended behavior, but the first one makes it read as it if was some edge behavior.
There was a problem hiding this comment.
Fixed in 62a2c3c. You're right that the ordering did the damage — leading with "treat received paths as read-only" implies the returning case is the exception you fall back to, when it's the normal shape, and the one the shell pipeline in Files.ipynb actually uses.
Flipped it: returning what you wrote to now leads and is stated as intended, and the read-only advice is scoped to paths that are pure input.
Returning what you wrote to is the normal shape, not a workaround for this: a function that writes into a directory and hands that directory back is recorded with it in its final, post-mutation state, and that is the intended way to produce files. What is not replayed is a write to something you never return — so a path that is pure input should be treated as read-only, with new files written somewhere you do return (
tempfile.mkdtemp).
Generated by Claude Code
There was a problem hiding this comment.
Cell 9 is not path specific, delete or make it applicable to the path/file caching.
There was a problem hiding this comment.
Agreed — write.query().table() was generic query introspection that happened to be sitting in a file-caching notebook. Made it path-specific rather than deleting it, since the slot is useful: 62a2c3c replaces it with the value store's own view of what a stored path actually is.
# What a `Path` actually becomes in storage: its content as plain `bytes` under
# its own digest, plus a small record pairing that content with a name. Note
# that left.txt and right.txt reference the *same* content digest.
for blob in c.values.storage.values():
if type(blob).__name__ in ("FileBlob", "DirectoryBlob"):
print(blob)FileBlob('out.txt', '785d68f8426805e292630852bdedb46dd56ac44dcb7047740d30704f3d84d4fa')
FileBlob('sentence.txt', '0f61b76af53fa2dc41528c3866206d22ceb3b7f560e7f42aacc006fc5ace228c')
DirectoryBlob({'leaf.bin': 'c70f6db1a5371bc6046fb5a040fd13bd5220c78908eac8126c1361daad854904'})
DirectoryBlob({'sub': '7aa02a10cd58fe7b0f16ef0e06b255d7842ce362687e489e941281d27761d95a', 'top.txt': '8eb42147b1727df4b082ebc0bdfc5fbaea064308411a4d801cae67c908ce4287'})
FileBlob('left.txt', '32cbd77d1dbff488cd42dc84ea72ebd47358fbf321412b35a5c1084e36f5b775')
FileBlob('right.txt', '32cbd77d1dbff488cd42dc84ea72ebd47358fbf321412b35a5c1084e36f5b775')
It follows the content-addressing section directly, so it lands well: the cell above asserts the shared body is stored once, and this one shows why — left.txt and right.txt are two small name records pointing at one content digest, and the directory's root name is nowhere to be seen while its children's names are. The same split the dev/path_storage page describes, made concrete.
Generated by Claude Code
There was a problem hiding this comment.
edges 3,5,6 are real, the others are from struct pure function perspective always been wrong, so rephrase these more in a 'beware that ...' kind of way. Currently it reads as if this is a defect that might get fixed.
There was a problem hiding this comment.
That's a real distinction I'd flattened — calling all six "Edge N" made them read as one list of things that are wrong, with the implication that someone will get around to them. Fixed in 62a2c3c by splitting the vocabulary.
Beware (1, 2, 4 — consequences of the model, not going to change). Each now says why it isn't a defect:
- a hit gives you a copy, somewhere else — "the function's value is the file it returned, and a location is not part of a file's content, so this is content addressing working exactly as advertised, not a fidelity gap." The sibling footgun follows: the sibling was never part of the value.
- aliasing is not part of a value — "object identity is a property of one process's memory, not of the value being cached — nothing about 'these two files are the same object' survives a round trip through storage, and nothing could."
- each hit is its own copy, with temp-file lifetime — "the cache owns the content and hands you a copy; it cannot know when you're finished with it except by watching the reference you were given."
Caveat (3, 5, 6 — the real ones), unchanged in substance: Path dict keys, opaque containers, container subclasses.
The intro now draws the line explicitly rather than promising "the edge cases where a cache hit is not a faithful replay":
- Beware — consequences of what caching a pure function by content means. A cached call is replayed by its value, so a file comes back as a copy: its location, its identity, and whatever sat next to it on disk were never part of that value. Code that leaned on them was relying on something the cache never promised. These are not defects and they are not going to change.
- Caveat — real limits of the mending machinery, where a hit hands back something less faithful than it could: paths used as dict keys, and paths hidden inside containers fleche does not destructure.
Summary rewritten to match. The code cells are untouched — the demonstrations were fine, it was the framing around them that was wrong.
Generated by Claude Code
| # Wrapping command-line tools is a first-class use case (see | ||
| # notebooks/Files.ipynb), and `run()`'s return value is neither a | ||
| # dataclass nor iterable, so without this arm every such function | ||
| # is Indigestible. The four public fields are the whole result: | ||
| # what was run, how it exited, and what it wrote. Purely | ||
| # additive — these values raised before, so no stored digest | ||
| # changes and no `hash_version` bump is implied. |
There was a problem hiding this comment.
| # Wrapping command-line tools is a first-class use case (see | |
| # notebooks/Files.ipynb), and `run()`'s return value is neither a | |
| # dataclass nor iterable, so without this arm every such function | |
| # is Indigestible. The four public fields are the whole result: | |
| # what was run, how it exited, and what it wrote. Purely | |
| # additive — these values raised before, so no stored digest | |
| # changes and no `hash_version` bump is implied. |
There was a problem hiding this comment.
Dropped in 62a2c3c — the arm is (value.args, value.returncode, value.stdout, value.stderr), which says it itself.
The one non-obvious bit was that it's purely additive (those values raised Indigestible before, so no stored digest changes and no hash_version bump); that's recorded in the DEVELOPING design-theme entry instead, where the other digest-arm additions are listed.
Generated by Claude Code
Review revisions on #831. - `file_semantics` argument mutation: led with "treat received paths as read-only", which framed writing-into-a-directory-and-returning-it as an odd case to work around. It is the normal shape — and the one the shell pipeline in Files.ipynb uses. Lead with that instead; the read-only advice now applies only to paths that are pure input. - `file_semantics` nesting: "any depth" was asserted without saying why, and `remaining_depth` looked like it might narrow it. It cannot, at any setting: a Path matches no destructurer, so `_intern_rec` leaves its depth at infinity and always writes it out as its own entry rather than inlining it — and being written out is exactly what hands it to PathValueMixin. `remaining_depth` only tunes inline-vs-separate for destructurable nodes. Verified across remaining_depth 0/1/3/10 at nesting depths 0-8. - `Files.ipynb`: the `write.query().table()` cell demonstrated generic query introspection, nothing path-specific. Replaced with the value store's own view of a stored path — the content `bytes` under their digest plus the FileBlob/DirectoryBlob record naming them — which also shows left.txt and right.txt sharing one content digest, the dedup from the cell above seen from the storage side. - `PathsInContainers.ipynb`: the six "Edge N" sections read as a defect list awaiting fixes. Three of them aren't defects — location changes, aliasing loss, and per-hit copies follow from caching a pure function by value, and code relying on location, identity, or unreturned siblings was relying on something never promised. Those are now "Beware:" sections that say so. The genuinely sharp ones — Path dict keys, opaque containers, container subclasses — are "Caveat:", and the intro and summary draw the line. - Dropped the explanatory comment on the CompletedProcess digest arm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RascUYS7JgCMHHpXsYbPTf
Answers your comment on #797, ["Interactions with ssh cache unclear"](#797 (comment)). Targets `temppath`. The four **inline** review threads on #797 moved to #831, which has since **merged** — `temppath` was merged back in here (`f938975`, no conflicts), so this PR is the `SshCache` × `Path` work only. ## What the interaction actually was Probed with a real `python -m fleche remote --serve` subprocess, client and server in different working directories so the same relative name denotes different files on each side. Three behaviours, all silent: | | before | |---|---| | server *can* see something at that name | stores **its** bytes, returns **its** digest — `digest(x) == save_value(x)` broken, so the record is filed under a key no client recomputes, and a load returns the wrong content | | server can see nothing there | `Indigestible` surfaces from inside an RPC | | loading a path stored remotely | server materializes into a temp dir on **its** disk and sends back only the name — dangling here, and unlinked there as soon as its own reference dies | The last one is not fixable by holding a reference: `PurePath.__reduce__` rebuilds from `parts` alone, so `TempPath`'s `_temp_root` is dropped in transit and `_live_roots` is per-process. The `save_value` comment already said "paths over SSH remain unsupported" — nothing enforced it. ## What this does Refuses, in both directions, via a new `RemotePathUnsupported`. It subclasses `SaveError`, so the two-phase-save degradations already in the codebase carry it and no caller special-cases anything: - path **argument** → digest-only reference, **digest computed locally**. The seal stays intact, lookups hit and miss correctly; only the bytes aren't retrievable from the remote record. - path **result** → `Rejected`. The call runs, returns the file, is logged as not cached. - **load** → raises, lazily. A record whose result is a path still loads and queries; only touching the path value raises. Detection is `storage.paths.find_path`, walking a value through a new `storage.destructuring.child_slots` — the read-only half of `_intern_rec`'s dispatch — so nesting is covered exactly as far as a destructuring save reaches, and no further (a path inside an opaque object is still stored by location, same as locally). Escape hatches, both documented: return `bytes`, or put a local layer in front of the remote one so saves never reach the wire. ## Follow-up (second commit) Per your review, the problem itself is tracked as **#829** rather than left as a comment. The shape is already implied by `PathValueMixin`: it reduces a path to `bytes` plus a `FileBlob`/`DirectoryBlob`, all of which ship fine, and those blobs' `__digest__` is *defined* to match the `Path` digest arm — so running the reduction **client-side** keeps the seal intact by construction, and needs no server change for saves (a `FileBlob` isn't a `Path`, so it falls straight through the server's path layer). The one genuinely new verb is an unmended `load_value` that returns the blob instead of materializing it on the server. Recorded in `RemotePathUnsupported`, `dev/path_storage`, and the DEVELOPING design-theme cheat sheet. ## Docs - `usage/file_semantics.rst`: new "Paths stop at a remote (SSH) cache" contract section + a quick-reference row. - `dev/path_storage.rst`: the mechanism, and the #829 sketch. - `recipes/files_and_paths.rst`: a recipe. `SshCache`'s own docstring too. ## Tests `tests/unit/test_remote.py` — bare and nested (list/tuple/nested-dict/dataclass) rejection with nothing reaching the server; the `bytes` hatch still working; `prepare()` keeping `prepared.key == call.to_lookup_key()` for a path argument (the regression the guard buys); live-`Call` rejection; load-side rejection incl. nested; a `LazyCall` whose arguments still read fine and only `.result` raises. `tests/unit/storage/test_paths.py` — `find_path` descent, opaque-leaf boundary, cycle termination. `tests/integration/test_remote.py` — the divergent-cwd reproduction above, and a path-returning `@fleche` function running uncached against a remote instead of breaking. Re-verified after merging `temppath`: **1788 passed / 11 skipped**, docs build succeeds. `ty check src/` reports **3** diagnostics — all already on `temppath`, none from this PR; see [the #831 thread](#831 (comment)) for the diagnosis and a verified fix, still pending your call on where it should land. --------- Co-authored-by: Claude <noreply@anthropic.com>
Split out of #828 per review. Answers the four inline review threads on #797; #828 keeps the
SshCache×Pathwork. Independent of #828 — no shared hunks, either can land first."feels almost more like it should go into a more generic section rather than file specific"
You were right, and it's stronger than "feels like": I checked, and the rules aren't path-specific at all. A mutated list argument behaves identically —
— and a mutated argument that is returned comes back post-mutation. So there's now a
usage/puritypage carrying the general contract (keyed as passed, mutation not replayed, other side effects cold-call-only, plus theNone-not-cached tie-in), andfile_semanticskeeps only the path instance and links out. Wired into the "Using Fleche" toctree right aftertldr."cross ref destructuring here, since that's the key"
Done, and I reframed the section around it rather than appending a link. Only destructured children reach the path machinery, so
_DESTRUCTURERSis the list of places a nested path gets content treatment — one fact that explains the container list, the exact-types caveat, and the "opaque containers store paths by location" section below it. Links:ref:extending-destructurer``."move the completedprocess digester into fleche itself"
Now a match arm in
digest.py—args,returncode,stdout,stderr.run()'s result is neither a dataclass nor iterable, so it wasIndigestibleand every shell-wrapping function needed the same hand-rolled hook.Purely additive: those values raised before, so no stored digest changes and no
hash_versionbump. The type name is already mixed in upstream of the match, so it salts like every other arm."save executed version"
Files.ipynbcommitted executed — same commandrendernb.ymlruns — and itsadd_hookcell is gone now that the digest is built in, so the pipeline section demonstrates the built-in path (second run fully cached, no[shell]prints).While in there:
PathsInContainers.ipynbhad been added with nodocs/notebooks/symlink, no toctree entry, and no slot intest_notebooks.py, so it wasn't in the docs or under test at all. Wired up all three and committed it executed too.Review round 2 (
62a2c3c)Files.ipynbuses. Flipped: returning leads and is stated as intended; read-only now scopes to paths that are pure input.remaining_depth. Verified rather than asserted, and the docs now say why: aPathmatches no destructurer, so_intern_recleaves its depth atfloat("inf"),inf < remaining_depthis never true, and it is therefore always written out as its own entry — which is exactly wherePathValueMixinintercepts.remaining_depthonly tunes inline-vs-separate for destructurable nodes. Checked acrossremaining_depth∈ {0, 1, 3, 10} × nesting depth ∈ {0, 1, 2, 3, 5, 8}, original deleted before load: 24/24 by content.Files.ipynbquery cell was generic introspection, not path-specific. Now prints the value store's own view of a stored path — contentbytesunder their digest plus theFileBlob/DirectoryBlobnaming records — landing right after the dedup cell, whereleft.txtandright.txtvisibly share one content digest.PathsInContainers.ipynbframing. The six "Edge N" sections read as a defect list awaiting fixes. Three aren't defects — location changes, aliasing loss, per-hit copies follow from caching a pure function by value — and are now Beware: sections that say so. The genuinely sharp ones (Path dict keys, opaque containers, container subclasses) are Caveat:. Intro and summary draw the line.CompletedProcessdigest arm.Tests
tests/unit/digest/test_digest.py—CompletedProcessfield-by-field discrimination (args / returncode / stdout / stderr), not-digest-equal to its own field tuple, text-modestrand uncapturedNonestreams, realsubprocess.runresults, and nesting in a tuple/dict.tests/integration/test_notebooks.pygainsPathsInContainers.ipynb.1763 passed / 11 skipped. Docs build succeeds; only non-autoapi warning is the pre-existing
fleche.Dduplicate (I fixed a short title underline inusage/helpersthat surfaced once the build got that far).ty check src/reports 3 diagnostics — all of them already ontemppath, none added here. (An earlier revision of this description said 4; that was an unpinned localty, see the comment.)tyis red on the base branch and green onmain; that thread has the diagnosis and a verified fix, pending your call on where it should land.