Measure what the lineage ids cost, and fix the tool that measures - #959
Conversation
The differential check proves a refactor changed no values by digesting every patch and comparing against a reference. Its digest left out history but not the two lineage ids, and `patch_id` is minted per patch for anything not read from a file -- so every patch differed, and a check which always reports a difference reports nothing. The ids join history, for the same reason and a sharper one: the comparison is against a patch built by other code, and these name where a patch came from rather than what the answer is. 775 calls now compare identical against dev. Benchmarks for the charge itself, at both ends: a tiny patch where it is all there is, an array argument where it is not flat because the array is hashed, and real filtering where it should not be findable. The charge is flat, about 35-56 us to canonicalize a call and digest it. That is invisible next to a pass filter (+3%) and doubles a transpose on a two-by-two patch (+156%), so `patch_provenance` earns its keep and no knob comes out. Writing the promised `new`/`update` paragraph turned up a footgun worth saying out loud: they carry both ids through untouched, because they are how a patch function builds its own result and stamping there would count every operation twice -- so changing data through `new` yourself leaves the ids saying it did not change.
|
Warning Review limit reached
Next review available in: 29 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d-chambers
added
ready_for_review
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b53157201f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with config_context(patch_provenance="disabled"): | ||
| tiny_patch.transpose() |
There was a problem hiding this comment.
When the enabled and disabled timings are compared, only this control includes constructing a new configuration and entering/resetting a ContextVar; CodSpeed times that work along with transpose(). The resulting delta therefore understates, and could even obscure, the provenance overhead the pair is intended to measure. Enter the configuration in fixture setup or otherwise outside the timed benchmark body.
Useful? React with 👍 / 👎.
| @pytest.mark.benchmark | ||
| def test_identity_overhead_array_argument(self, example_patch, big_mask): | ||
| """An array parameter is hashed, which is the one real cost.""" | ||
| example_patch.where(big_mask) |
There was a problem hiding this comment.
For the large-mask scenario, this benchmark records only the total cost of where; without an otherwise identical patch_provenance="disabled" measurement, the result cannot distinguish the array fingerprinting cost from the underlying mask operation. Consequently it cannot measure or track the identity overhead this test was added for, so this case needs a paired disabled control.
Useful? React with 👍 / 👎.
| @pytest.mark.benchmark | ||
| def test_identity_overhead_tiny_patch(self, tiny_patch): | ||
| """The flat charge, with nothing else in the way.""" | ||
| tiny_patch.transpose() |
There was a problem hiding this comment.
When this benchmark runs after any code that has already made the same argument-free transpose() call, fingerprint_call uses the process-wide _FINGERPRINTS cache, whereas a standalone or differently ordered run may pay the initial binding and digest cost. Because that state change is comparable to the small overhead being measured, the recorded result can alternate between cold- and warm-cache behavior based on test order; warm the exact call or reset the cache in fixture setup so the intended state is explicit and outside the timed body.
Useful? React with 👍 / 👎.
| built = dc.Patch(data=np.asarray(patch.data), coords=patch.coords, dims=patch.dims) | ||
| assert built.attrs.patch_id != patch.attrs.patch_id |
There was a problem hiding this comment.
When a user follows this example as the suggested alternative to patch.new(data=...), omitting attrs makes the constructor create default PatchAttrs, silently discarding the source patch's data_units, acquisition key, tag, and any custom metadata. The example should copy the non-lineage attributes while clearing the lineage fields before construction, or explicitly show that relevant metadata must be reapplied.
Useful? React with 👍 / 👎.
The control was measuring the wrong thing: entering `config_context` builds and validates a whole config, and it was doing so inside the timed body. It moves to a fixture. `fingerprint_call` also memoizes, so repeating one call timed a lookup rather than a digest -- 53 us against 77 us on a two-by-two transpose, and a real loop varies its arguments and pays the second. Both are timed now, and the array case gains the control it was missing. `processing_id` goes back into the differential digest. It is a digest of the route rather than a minted value and is the same in both processes, so keeping it catches a call which stopped being stamped or started canonicalizing its arguments differently -- which the data hash cannot see. Only `patch_id` is excluded. 775 calls still compare identical. The policy test started from an unprocessed patch, so preserving the route was satisfied by dropping it; it starts from an operated one. And the prose said the ids are carried untouched, which is not true of someone who names one and sets it.
a550838 to
c1a1e5d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #959 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 195 195
Lines 25268 25268
=========================================
Hits 25268 25268
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:
|
1 participant
Description
PR 5 of the processors plan: the harness, the benchmarks, and the knob decision the 4a timing table was supposed to make. No behaviour changes — this is the measuring step, plus one live bug in the measuring tool.
The differential harness was blind to itself
scripts/differential_check.pyproves a refactor changed no values by digesting every patch and comparing against a pre-change reference. Its digest excludedhistoryandcoordsbut notpatch_id, which is minted per patch for anything not read from a file. Since the harness dumps in two processes, two identical patches digested differently:Every patch would have reported a difference, which means the check would have said nothing at all — for exactly the PRs (0, 3, 4a, 5) the plan says to run it against.
patch_idjoinshistoryin the exclusion.processing_iddeliberately does not. It is a digest of the route rather than a minted value, and it is identical in both processes (verified), so keeping it lets the harness catch a call which stopped being stamped or started canonicalizing its arguments differently — regressions the data hash alone would miss.775 calls compared against origin/dev; all identical.Benchmarks
TestIdentityOverheadinbenchmarks/test_patch_benchmarks.py, on CodSpeed:test_identity_overhead_tiny_patch— the charge on a call which does nothing else.test_identity_overhead_uncached— the same call with the memo cleared, which is what a loop over varying arguments pays.test_identity_overhead_array_argument—wherewith a big mask, the one place the charge is not flat, because an array parameter is hashed.test_identity_overhead_real_work—pass_filter, where the charge should not be findable.test_processor_fingerprint— building an operation and asking what it is.Each of the four overhead benchmarks has a
_disabledtwin. The control turns the ids off in a fixture, not inside the timed body: enteringconfig_contextbuilds and validates a whole config, which is not what a control should be measuring.The plan also asked for
test_provenance_graph_overhead. There is no graph — it was cut on 2026-08-20 — so that one simply does not exist.The knob decision
Measured on this branch, best-of-7, ids on vs
patch_provenance="disabled", with the config context outside the timed region:tiny.transpose()(memo hit)tiny.transpose()(memo missed)tiny.abs()Patch(...)constructionpatch.abs()patch.where(big mask)patch.normalize("time")patch.pass_filter(time)patch.select(time=…)spool.chunk(time=None)The charge is roughly 53 µs to canonicalize a call and look up its digest, and 77 µs when the digest actually has to be computed —
fingerprint_callmemoizes, so repeating one call is the cheap case and a loop over varying arguments is not. Either way it is invisible next to a pass filter (+1%) and more than doubles a transpose on a two-by-two patch.whereis the exception to flatness: its array argument is hashed, so the charge scales with the mask and the memo cannot help.So
patch_provenancestays. The plan said to remove whichever knobs the table made unnecessary; the table says none of them are.selectcosting nothing is the no-op short-circuit from #943 handing the patch straight back.Patch.new/updatepolicyThe plan asked for a paragraph, and writing it turned up a footgun worth stating plainly:
new,updateandupdate_attrscarry both ids through unless you name one and set it yourself — they are how a patch function assembles its own result, so stamping there would count every operation twice. The consequence is that changing data throughnewyourself leaves the ids claiming the data did not change:Documented in the patch tutorial under "What builds a patch, and what operates on one", and pinned by tests, because it is a policy rather than an accident. Building from arrays rather than from another patch is the honest case and mints a new
patch_id.A note on the red checks
This branch is merged up to
dev, anddevis currently red for reasons unrelated to this PR. Verified by runningorigin/devon its own in a detached worktree — identical counts and names:tests/test_viz/test_inventory_viz.py— 61 errorstests/test_viz/test_lanes.py::TestLayout::test_default_labelsandTestColors::test_boolean_lane— 2 failuresdascore/viz/_lanes.py::plot_lanesdoctestIt looks like a crossed merge: #950 made label membership mean no value and now refuses
value=True, while the lanes and inventory-viz work still constructs labels that way. Both were green on their own.Excluding that one file, this branch is
12,247 passed. Nothing here touches viz.Changelog
scripts/differential_check.pyno longer reports a difference for every patch;patch_idis excluded from its digest, ashistoryalready was.Patch.newandPatch.updatedo to the lineage ids, and what that means if you change data through them.Checklist
I have:
docs/contributing/general_guidelines.qmd).I have (if applicable):