Various typed-choice-sequence shrinking patches#4755
Conversation
Above MAX_PRECISE_INTEGER the gap between adjacent floats exceeds one, so delegating to the integer shrinker stalled: its n - 1 step rounds straight back to n. Shrink such floats directly instead, halving for coarse progress and stepping by next_down (a unit decrement of the bit pattern) for fine progress, with a top-byte short-circuit to land cross-boundary targets fast. https://claude.ai/code/session_01KbWXEu5qRTThaA61whuS2Y
The collection shrinker deleted elements one at a time from the back, costing O(n) test calls to remove a run of n deletable elements. Grow the deleted chunk via find_integer instead, so a deletable run costs O(log(n)) calls. Shrunk results are unchanged. https://claude.ai/code/session_01KbWXEu5qRTThaA61whuS2Y
The internals contributor guide still described the old byte-buffer shrinker (incorporate_new_buffer, minimizer.py, byte slicing). Rewrite it to describe the typed choice sequence and current shrinker APIs, and fix a stale code comment referencing the removed adaptive_example_deletion pass. https://claude.ai/code/session_01KbWXEu5qRTThaA61whuS2Y
Replace the per-item release notes with a single user-facing note covering the float and collection shrinking speedups, for review as one PR. https://claude.ai/code/session_01KbWXEu5qRTThaA61whuS2Y
When an early choice controls the size of a later collection (eg draw_integer() then draw_string(min_size=n, max_size=n)), lowering that choice misaligns the recorded collection value. The engine canonicalizes the misalignment to the simplest value, discarding the interesting contents, and the result is cached as an ordinary non-misaligned run -- so try_shrinking_nodes' realignment branch was effectively dead. Detect the realigned string/bytes node by comparing what we asked for against what was drawn, and retry with the recorded value truncated to fit, preserving content from either end. Also handle the case where the now-boring collection lets the test draw further and overrun (common in stateful tests) by re-running without the length limit to recover the realigned tree.
Replace the coarse-halve + ULP-step big-float branch with a single delegation to Integer over a bijection between non-negative floats and integer positions: int(f) below MAX_PRECISE_INTEGER, and MAX_PRECISE_INTEGER + (bit-pattern offset) above it. Adjacent representable floats become adjacent integers, so Integer.shrink's n - 1 step always corresponds to next_down(n) and never stalls. https://claude.ai/code/session_01KbWXEu5qRTThaA61whuS2Y
Property-based tests that _float_to_position and _position_to_float round-trip on their respective domains, that the mapping is monotonic, and that decrementing the position of a float above MAX_PRECISE_INTEGER lands on next_down -- the property that lets Integer.shrink make progress past the boundary. https://claude.ai/code/session_01KbWXEu5qRTThaA61whuS2Y
DRMacIver
left a comment
DRMacIver
left a comment
There was a problem hiding this comment.
Generally LGTM. I've made some comments on things I'd like clarified or fixed, but don't need to rereview once they're addressed.
| and not node.was_forced | ||
| and len(node.value) > 0 | ||
| for node in initial_attempt[after:] | ||
| ): |
There was a problem hiding this comment.
I don't think I understand what this check is doing or how it relates to the comment above. Elaborate?
There was a problem hiding this comment.
I think part of my confusion here is that we're talking about repairing collections (which we can do here! We have bits later that delete ranges) and this bit just assumes we're only handling strings and bytes.
| failing example to a minimal one - in two ways. | ||
|
|
||
| First, shrinking large floats (those above ``2**53``) and collections such as | ||
| :func:`~hypothesis.strategies.lists` is now substantially faster, especially for |
There was a problem hiding this comment.
I don't think this is correct. The Collection shrinker is terribly named, and is only actually used for shrinking strings and bytes.
There was a problem hiding this comment.
I disagree shrinker.Collection is badly named! It's generically written to shrink any collection, given an element shrinker. This is the model failing to understand relative terminology.
There was a problem hiding this comment.
Well among other problems, it's not, it's designed to shrink any sequence.
Drop the narrow string/bytes-only gate before retrying with extend="full": removing it makes the comment match what the code does, and the small extra cost (an occasional retry that the repair logic below then declines) is bounded by cached_test_function's cache. Also fix the release note: the Collection shrinker only drives strings and bytestrings, not lists. https://claude.ai/code/session_01KbWXEu5qRTThaA61whuS2Y
| def _float_to_position(f: float) -> int: | ||
| """Map a non-negative float to a linear integer position such that adjacent | ||
| representable floats correspond to adjacent integers. |
There was a problem hiding this comment.
this comment is wrong: we are mapping such that adjacent integer portions of floats correspond to adjacent integers. As written, this comment implies the invariant _float_to_position(f) + 1 == _float_to_position(next_up(f)), which is not true for f < MAX_PRECISE_INTEGER.
| # here rather than trying to persevere. | ||
| if attempt.status is Status.OVERRUN: | ||
| return False | ||
| # Lowering a size-controlling choice can make the realigned (and | ||
| # now boring) collection stop triggering the failure, so the test | ||
| # draws further and overruns before we see the realignment -- this | ||
| # is common in stateful tests, where a non-failing step is followed | ||
| # by more steps. Re-run without the length limit to recover the | ||
| # realigned tree, which the repair logic below can then act on. | ||
| attempt = self.engine.cached_test_function( | ||
| [n.value for n in initial_attempt], extend="full" | ||
| ) | ||
| if attempt.status is Status.OVERRUN: | ||
| return False | ||
|
|
There was a problem hiding this comment.
non-blocking: I somewhat worry this could have bad performance implications in pathological cases. extend="full" can get quite expensive, because it can draw many more choices relative to the current size of our shrink target (up to BUFFER_SIZE in the absolute worst case, which I agree is unlikely)
| # the recorded value for that collection no longer fits the constraints | ||
| # the test function actually used, so the engine realigns the tree by | ||
| # substituting a freshly-generated (simplest) value -- discarding | ||
| # whatever made the collection interesting. (We can't rely on | ||
| # ``attempt.misaligned_at`` to detect this, because the realigned choice | ||
| # sequence is often independently cached as an ordinary, non-misaligned | ||
| # result.) We detect a string/bytes node whose recorded value is now too |
There was a problem hiding this comment.
I don't quite understand this comment. Is the implication here that:
- We have some choice sequence A corresponding to some data data_A
- In this shrinking pass, we modify A to get B, corresponding to data data_B (=
attempt) - data_B would have
misaligned_atif it were allowed to run fully, but we actually had B in cache, so the returned data_B doesn't have misaligned_at, and therefore we can't rely onmisaligned_atbecause we don't know if B is cached or not
If this is the implication, I think this is either a bug or a misunderstanding from the model, because I disagree with it. B should run to the same data_B every time, including misaligned_at, so we shouldn't have to think about whether B was cached when checking misaligned_at.
| # end (see test_can_shrink_variable_string_draws). | ||
| for i in range(min(len(initial_attempt), len(attempt.nodes))): | ||
| node = initial_attempt[i] |
There was a problem hiding this comment.
Previously, we only tried editing the misaligned_at node. Now we try modifying all {string, bytes} nodes whose max_size decreased in the attempt.
It's almost certainly only valuable to do this editing on the first misaligned node. Dropping the misaligned_at check here means we now edit nodes after the first misalignment:
- if the first misalignment was a non-candidate node (where "candidate" means "is a {string, byte} node with a decreased max_size)
- if the first misalignment was a candidate node, but there are multiple such candidate nodes, so we try editing all candidate nodes
| ): | ||
| max_size = attempt_node.constraints["max_size"] | ||
| for truncated in (node.value[:max_size], node.value[-max_size:]): |
There was a problem hiding this comment.
Previously, we anchored on min_size. Now, we anchor on max_size. This doesn't make a difference for draw_string(min_size=n, max_size=n). But I actually think draw_string(min_size=n) is more common than draw_string(min_size=n, max_size=n) (or even draw_string(max_size=n)).
I'm surprised this change didn't result in any regressions.
There was a problem hiding this comment.
If we don't set max_size, I don't think we need this logic to get unstuck though, right?
4 participants
Closes #4006