From 5dd6c7e1257c1b419497dcaf1317094fae154683 Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Sun, 13 Oct 2024 18:01:40 -0400 Subject: [PATCH 1/6] fold integer endpoint upweighting to weights= --- hypothesis-python/RELEASE.rst | 5 ++ .../hypothesis/internal/conjecture/data.py | 61 +++++++++++-------- .../strategies/_internal/numbers.py | 19 +++--- hypothesis-python/tests/conjecture/common.py | 49 +++++---------- .../tests/conjecture/test_alt_backend.py | 4 +- .../tests/conjecture/test_forced.py | 28 ++++++++- 6 files changed, 92 insertions(+), 74 deletions(-) create mode 100644 hypothesis-python/RELEASE.rst diff --git a/hypothesis-python/RELEASE.rst b/hypothesis-python/RELEASE.rst new file mode 100644 index 0000000000..2c4477ab8f --- /dev/null +++ b/hypothesis-python/RELEASE.rst @@ -0,0 +1,5 @@ +RELEASE_TYPE: patch + +This release improves integer shrinking by folding the endpoint upweighting for :func:`~hypothesis.strategies.integers` into the ``weights`` parameter of our IR (:issue:`3921`). + +If you maintain an alternative backend as part of our (for now explicitly unstable) :ref:`alternative-backends`, this release changes the type of the ``weights`` parameter to ``draw_integer`` and may be a breaking change for you. diff --git a/hypothesis-python/src/hypothesis/internal/conjecture/data.py b/hypothesis-python/src/hypothesis/internal/conjecture/data.py index 3664d76729..22803aa5cc 100644 --- a/hypothesis-python/src/hypothesis/internal/conjecture/data.py +++ b/hypothesis-python/src/hypothesis/internal/conjecture/data.py @@ -88,7 +88,7 @@ def wrapper(tp): class IntegerKWargs(TypedDict): min_value: Optional[int] max_value: Optional[int] - weights: Optional[Sequence[float]] + weights: Optional[dict[int, float]] shrink_towards: int @@ -1287,7 +1287,7 @@ def draw_integer( max_value: Optional[int] = None, *, # weights are for choosing an element index from a bounded range - weights: Optional[Sequence[float]] = None, + weights: Optional[dict[int, float]] = None, shrink_towards: int = 0, forced: Optional[int] = None, fake_forced: bool = False, @@ -1456,8 +1456,7 @@ def draw_integer( min_value: Optional[int] = None, max_value: Optional[int] = None, *, - # weights are for choosing an element index from a bounded range - weights: Optional[Sequence[float]] = None, + weights: Optional[dict[int, float]] = None, shrink_towards: int = 0, forced: Optional[int] = None, fake_forced: bool = False, @@ -1475,22 +1474,35 @@ def draw_integer( assert min_value is not None assert max_value is not None - sampler = Sampler(weights, observe=False) - gap = max_value - shrink_towards - - forced_idx = None - if forced is not None: - if forced >= shrink_towards: - forced_idx = forced - shrink_towards - else: - forced_idx = shrink_towards + gap - forced - idx = sampler.sample(self._cd, forced=forced_idx, fake_forced=fake_forced) + # format of weights is a mapping of ints to p, where sum(p) < 1. + # The remaining probability mass is uniformly distributed over + # *all* ints (not just the unmapped ones; this is somewhat undesirable, + # but simplifies things). + # + # We assert that sum(p) is strictly less than 1 because it simplifies + # handling forced values when we can force into the unmapped probability + # mass. We should eventually remove this restriction. + sampler = Sampler( + [1 - sum(weights.values()), *weights.values()], observe=False + ) + # if we're forcing, it's easiest to force into the unmapped probability + # mass and then force the drawn value after. + idx = sampler.sample( + self._cd, forced=None if forced is None else 0, fake_forced=fake_forced + ) - # For range -2..2, interpret idx = 0..4 as [0, 1, 2, -1, -2] - if idx <= gap: - return shrink_towards + idx - else: - return shrink_towards - (idx - gap) + return ( + self._draw_bounded_integer( + min_value, + max_value, + forced=forced, + center=shrink_towards, + fake_forced=fake_forced, + ) + if idx == 0 + # implicit reliance on dicts being sorted for determinism + else list(weights)[idx - 1] + ) if min_value is None and max_value is None: return self._draw_unbounded_integer(forced=forced, fake_forced=fake_forced) @@ -2116,8 +2128,7 @@ def draw_integer( min_value: Optional[int] = None, max_value: Optional[int] = None, *, - # weights are for choosing an element index from a bounded range - weights: Optional[Sequence[float]] = None, + weights: Optional[dict[int, float]] = None, shrink_towards: int = 0, forced: Optional[int] = None, fake_forced: bool = False, @@ -2127,9 +2138,11 @@ def draw_integer( if weights is not None: assert min_value is not None assert max_value is not None - width = max_value - min_value + 1 - assert width <= 255 # arbitrary practical limit - assert len(weights) == width + assert len(weights) <= 255 # arbitrary practical limit + # We can and should eventually support total weights. But this + # complicates shrinking as we can no longer assume we can force + # a value to the unmapped probability mass if that mass might be 0. + assert sum(weights.values()) < 1 if forced is not None and (min_value is None or max_value is None): # We draw `forced=forced - shrink_towards` here internally, after clamping. diff --git a/hypothesis-python/src/hypothesis/strategies/_internal/numbers.py b/hypothesis-python/src/hypothesis/strategies/_internal/numbers.py index 033577f2c8..2e4bf01732 100644 --- a/hypothesis-python/src/hypothesis/strategies/_internal/numbers.py +++ b/hypothesis-python/src/hypothesis/strategies/_internal/numbers.py @@ -66,24 +66,21 @@ def __repr__(self): def do_draw(self, data): # For bounded integers, make the bounds and near-bounds more likely. - forced = None + weights = None if ( self.end is not None and self.start is not None and self.end - self.start > 127 ): - bits = data.draw_integer(0, 127) - forced = { - 122: self.start, - 123: self.start, - 124: self.end, - 125: self.end, - 126: self.start + 1, - 127: self.end - 1, - }.get(bits) + weights = { + self.start: (2 / 128), + self.start + 1: (1 / 128), + self.end - 1: (1 / 128), + self.end: (2 / 128), + } return data.draw_integer( - min_value=self.start, max_value=self.end, forced=forced + min_value=self.start, max_value=self.end, weights=weights ) def filter(self, condition): diff --git a/hypothesis-python/tests/conjecture/common.py b/hypothesis-python/tests/conjecture/common.py index af67637030..4ae72cdf6a 100644 --- a/hypothesis-python/tests/conjecture/common.py +++ b/hypothesis-python/tests/conjecture/common.py @@ -144,11 +144,9 @@ def integer_kwargs( draw(st.booleans()) if (use_min_value and use_max_value) else False ) - # this generation is complicated to deal with maintaining any combination of - # the following invariants, depending on which parameters are passed: - # + # Invariants: # (1) min_value <= forced <= max_value - # (2) max_value - min_value + 1 == len(weights) + # (2) sum(weights.values()) < 1 # (3) len(weights) <= 255 if use_shrink_towards: @@ -158,39 +156,22 @@ def integer_kwargs( if use_weights: assert use_max_value assert use_min_value - # handle the weights case entirely independently from the non-weights case. - # We'll treat the weights as our "key" draw and base all other draws on that. - # weights doesn't play well with super small floats, so exclude <.01 + min_value = draw(st.integers(max_value=forced)) + min_val = max(min_value, forced) if forced is not None else min_value + max_value = draw(st.integers(min_value=min_val)) + + # Sampler doesn't play well with super small floats, so exclude them weights = draw( - st.lists(st.just(0) | st.floats(0.01, 1), min_size=1, max_size=255) + st.dictionaries(st.integers(), st.floats(0.001, 1), max_size=255) ) - # zero is allowed, but it can't be all zeroes - assume(sum(weights) > 0) - - # we additionally pick a central value (if not forced), and then the index - # into the weights at which it can be found - aka the min-value offset. - center = forced if use_forced else draw(st.integers()) - min_value = center - draw(st.integers(0, len(weights) - 1)) - max_value = min_value + len(weights) - 1 - - if use_forced: - # can't force a 0-weight index. - # we avoid clamping the returned shrink_towards to maximize - # bug-finding power. - _shrink_towards = clamped_shrink_towards( - { - "shrink_towards": shrink_towards, - "min_value": min_value, - "max_value": max_value, - } - ) - forced_idx = ( - forced - _shrink_towards - if forced >= _shrink_towards - else max_value - forced - ) - assume(weights[forced_idx] > 0) + # invalid to have a weighting that disallows all possibilities + assume(sum(weights.values()) != 0) + target = draw(st.floats(0.001, 0.999)) + # re-normalize probabilities to sum to some arbitrary value < 1 + weights = {k: v / target for k, v in weights.items()} + # float rounding error can cause this to fail. + assume(sum(weights.values()) == target) else: if use_min_value: min_value = draw(st.integers(max_value=forced)) diff --git a/hypothesis-python/tests/conjecture/test_alt_backend.py b/hypothesis-python/tests/conjecture/test_alt_backend.py index dd9edfe809..c9b12e5938 100644 --- a/hypothesis-python/tests/conjecture/test_alt_backend.py +++ b/hypothesis-python/tests/conjecture/test_alt_backend.py @@ -10,7 +10,6 @@ import math import sys -from collections.abc import Sequence from contextlib import contextmanager from random import Random from typing import Optional @@ -67,8 +66,7 @@ def draw_integer( min_value: Optional[int] = None, max_value: Optional[int] = None, *, - # weights are for choosing an element index from a bounded range - weights: Optional[Sequence[float]] = None, + weights: Optional[dict[int, float]] = None, shrink_towards: int = 0, forced: Optional[int] = None, fake_forced: bool = False, diff --git a/hypothesis-python/tests/conjecture/test_forced.py b/hypothesis-python/tests/conjecture/test_forced.py index 85ace494a3..c6846be00e 100644 --- a/hypothesis-python/tests/conjecture/test_forced.py +++ b/hypothesis-python/tests/conjecture/test_forced.py @@ -68,7 +68,7 @@ def test_forced_many(data): "min_value": -1, "max_value": 1, "shrink_towards": 1, - "weights": [0.1] * 3, + "weights": {-1: 0.2, 0: 0.2, 1: 0.2}, "forced": 0, }, ) @@ -80,11 +80,35 @@ def test_forced_many(data): "min_value": -1, "max_value": 1, "shrink_towards": -1, - "weights": [0.1] * 3, + "weights": {-1: 0.2, 0: 0.2, 1: 0.2}, "forced": 0, }, ) ) +@example( + ( + "integer", + { + "min_value": 10, + "max_value": 1_000, + "shrink_towards": 17, + "weights": {20: 0.1}, + "forced": 15, + }, + ) +) +@example( + ( + "integer", + { + "min_value": -1_000, + "max_value": -10, + "shrink_towards": -17, + "weights": {-20: 0.1}, + "forced": -15, + }, + ) +) @example(("float", {"forced": 0.0})) @example(("float", {"forced": -0.0})) @example(("float", {"forced": 1.0})) From 5b3e33eef2e8e4260d06c8f7a37025b618b5e550 Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Sun, 13 Oct 2024 18:35:08 -0400 Subject: [PATCH 2/6] fix tests --- .../tests/conjecture/test_test_data.py | 15 --------------- hypothesis-python/tests/conjecture/test_utils.py | 4 +++- .../tests/cover/test_filtered_strategy.py | 16 ++++++++++------ 3 files changed, 13 insertions(+), 22 deletions(-) diff --git a/hypothesis-python/tests/conjecture/test_test_data.py b/hypothesis-python/tests/conjecture/test_test_data.py index aafdc80e75..f48d1e8ba7 100644 --- a/hypothesis-python/tests/conjecture/test_test_data.py +++ b/hypothesis-python/tests/conjecture/test_test_data.py @@ -90,21 +90,6 @@ def test_can_mark_invalid(): assert x.status == Status.INVALID -@given(st.data(), st.integers(1, 100)) -def test_can_draw_weighted_integer_range(data, n): - weights = [1] * n + [0] * n - for _ in range(10): - # If the weights are working, then we'll never draw a value with weight=0 - x = data.conjecture_data.draw_integer(1, 2 * n, weights=weights) - assert x <= n - - -@given(st.binary(min_size=10)) -def test_can_draw_weighted_integer_range_2(buffer): - data = ConjectureData.for_buffer(buffer) - data.draw_integer(0, 7, weights=[1] * 8, shrink_towards=6) - - def test_can_mark_invalid_with_why(): x = ConjectureData.for_buffer(b"") with pytest.raises(StopTest): diff --git a/hypothesis-python/tests/conjecture/test_utils.py b/hypothesis-python/tests/conjecture/test_utils.py index 3bf50772d8..ec80952540 100644 --- a/hypothesis-python/tests/conjecture/test_utils.py +++ b/hypothesis-python/tests/conjecture/test_utils.py @@ -191,7 +191,9 @@ def test_single_bounds(lo, hi, to): def test_bounds_and_weights(): for to in (0, 1, 2): data = ConjectureData.for_buffer([0] * 100 + [2] * 100) - val = data.draw_integer(0, 2, shrink_towards=to, weights=[1, 1, 1]) + val = data.draw_integer( + 0, 2, shrink_towards=to, weights={0: 0.2, 1: 0.2, 2: 0.2} + ) assert val == to, to diff --git a/hypothesis-python/tests/cover/test_filtered_strategy.py b/hypothesis-python/tests/cover/test_filtered_strategy.py index 8ba40c1753..1607d872e1 100644 --- a/hypothesis-python/tests/cover/test_filtered_strategy.py +++ b/hypothesis-python/tests/cover/test_filtered_strategy.py @@ -9,20 +9,24 @@ # obtain one at https://mozilla.org/MPL/2.0/. import hypothesis.strategies as st -from hypothesis.control import BuildContext from hypothesis.internal.conjecture.data import ConjectureData from hypothesis.strategies._internal.strategies import FilteredStrategy +from tests.conjecture.common import run_to_buffer + def test_filter_iterations_are_marked_as_discarded(): variable_equal_to_zero = 0 # non-local references disables filter-rewriting - x = st.integers(0, 255).filter(lambda x: x == variable_equal_to_zero) - - data = ConjectureData.for_buffer([0, 2, 1, 0]) + x = st.integers().filter(lambda x: x == variable_equal_to_zero) - with BuildContext(data): - assert data.draw(x) == 0 + @run_to_buffer + def buf(data): + data.draw_integer(forced=1) + data.draw_integer(forced=0) + data.mark_interesting() + data = ConjectureData.for_buffer(buf) + assert data.draw(x) == 0 assert data.has_discards From 116bbd36d40410771fe05cb5082f69cb45c2619b Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Sun, 13 Oct 2024 21:33:28 -0400 Subject: [PATCH 3/6] also force in the idx case to ensure good shrinks not writing the idx case to the bytestream could result in the idx != 0 case being much bytestream-simpler but not ir-simpler --- .../src/hypothesis/internal/conjecture/data.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/hypothesis-python/src/hypothesis/internal/conjecture/data.py b/hypothesis-python/src/hypothesis/internal/conjecture/data.py index 22803aa5cc..d1fd359837 100644 --- a/hypothesis-python/src/hypothesis/internal/conjecture/data.py +++ b/hypothesis-python/src/hypothesis/internal/conjecture/data.py @@ -1491,17 +1491,13 @@ def draw_integer( self._cd, forced=None if forced is None else 0, fake_forced=fake_forced ) - return ( - self._draw_bounded_integer( - min_value, - max_value, - forced=forced, - center=shrink_towards, - fake_forced=fake_forced, - ) - if idx == 0 + return self._draw_bounded_integer( + min_value, + max_value, # implicit reliance on dicts being sorted for determinism - else list(weights)[idx - 1] + forced=forced if idx == 0 else list(weights)[idx - 1], + center=shrink_towards, + fake_forced=fake_forced, ) if min_value is None and max_value is None: From 0df70ec371ca03d23091a50f52224e6f41bb706b Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Sun, 13 Oct 2024 22:20:16 -0400 Subject: [PATCH 4/6] use ir_kwargs_key for pooled kwargs key --- .../src/hypothesis/internal/conjecture/data.py | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/hypothesis-python/src/hypothesis/internal/conjecture/data.py b/hypothesis-python/src/hypothesis/internal/conjecture/data.py index d1fd359837..8945f758b6 100644 --- a/hypothesis-python/src/hypothesis/internal/conjecture/data.py +++ b/hypothesis-python/src/hypothesis/internal/conjecture/data.py @@ -2374,18 +2374,7 @@ def _pooled_kwargs(self, ir_type, kwargs): if self.provider.avoid_realization: return kwargs - key = [] - for k, v in kwargs.items(): - if ir_type == "float" and k in ["min_value", "max_value"]: - # handle -0.0 vs 0.0, etc. - v = float_to_int(v) - elif ir_type == "integer" and k == "weights": - # make hashable - v = v if v is None else tuple(v) - key.append((k, v)) - - key = (ir_type, *sorted(key)) - + key = (ir_type, *ir_kwargs_key(ir_type, kwargs)) try: return POOLED_KWARGS_CACHE[key] except KeyError: From bf31b5e586cc837c5abb3a359b285a216f1fade0 Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Sun, 13 Oct 2024 22:20:34 -0400 Subject: [PATCH 5/6] improve redistribute_integer_pairs --- .../internal/conjecture/shrinker.py | 61 ++++++++----------- .../tests/conjecture/test_shrinker.py | 6 +- .../tests/quality/test_shrink_quality.py | 13 ++++ 3 files changed, 41 insertions(+), 39 deletions(-) diff --git a/hypothesis-python/src/hypothesis/internal/conjecture/shrinker.py b/hypothesis-python/src/hypothesis/internal/conjecture/shrinker.py index a1a3424963..91f86a2bb4 100644 --- a/hypothesis-python/src/hypothesis/internal/conjecture/shrinker.py +++ b/hypothesis-python/src/hypothesis/internal/conjecture/shrinker.py @@ -23,7 +23,6 @@ ConjectureData, ConjectureResult, Status, - bits_to_bytes, ir_value_equal, ir_value_key, ir_value_permitted, @@ -681,7 +680,7 @@ def greedy_shrink(self): "reorder_examples", "minimize_duplicated_nodes", "minimize_individual_nodes", - "redistribute_block_pairs", + "redistribute_integer_pairs", "lower_blocks_together", ] ) @@ -1227,42 +1226,32 @@ def minimize_duplicated_nodes(self, chooser): self.minimize_nodes(nodes) @defines_shrink_pass() - def redistribute_block_pairs(self, chooser): + def redistribute_integer_pairs(self, chooser): """If there is a sum of generated integers that we need their sum to exceed some bound, lowering one of them requires raising the other. This pass enables that.""" + # TODO_SHRINK let's extend this to floats as well. - node = chooser.choose( + # look for a pair of nodes (node1, node2) which are both integers and + # aren't separated by too many other nodes. We'll decrease node1 and + # increase node2 (note that the other way around doesn't make sense as + # it's strictly worse in the ordering). + node1 = chooser.choose( self.nodes, lambda node: node.ir_type == "integer" and not node.trivial ) + node2 = chooser.choose( + self.nodes, + lambda node: node.ir_type == "integer" + # Note that it's fine for node2 to be trivial, because we're going to + # explicitly make it *not* trivial by adding to its value. + and not node.was_forced + # to avoid quadratic behavior, scan ahead only a small amount for + # the related node. + and node1.index < node.index <= node1.index + 4, + ) - # The preconditions for this pass are that the two integer draws are only - # separated by non-integer nodes, and have the same size value in bytes. - # - # This isn't particularly principled. For instance, this wouldn't reduce - # e.g. @given(integers(), integers(), integers()) where the sum property - # involves the first and last integers. - # - # A better approach may be choosing *two* such integer nodes arbitrarily - # from the list, instead of conditionally scanning forward. - - for j in range(node.index + 1, len(self.nodes)): - next_node = self.nodes[j] - if next_node.ir_type == "integer" and bits_to_bytes( - node.value.bit_length() - ) == bits_to_bytes(next_node.value.bit_length()): - break - else: - return - - if next_node.was_forced: - # avoid modifying a forced node. Note that it's fine for next_node - # to be trivial, because we're going to explicitly make it *not* - # trivial by adding to its value. - return - - m = node.value - n = next_node.value + m = node1.value + n = node2.value def boost(k): if k > m: @@ -1272,11 +1261,11 @@ def boost(k): next_node_value = n + k return self.consider_new_tree( - self.nodes[: node.index] - + [node.copy(with_value=node_value)] - + self.nodes[node.index + 1 : next_node.index] - + [next_node.copy(with_value=next_node_value)] - + self.nodes[next_node.index + 1 :] + self.nodes[: node1.index] + + [node1.copy(with_value=node_value)] + + self.nodes[node1.index + 1 : node2.index] + + [node2.copy(with_value=next_node_value)] + + self.nodes[node2.index + 1 :] ) find_integer(boost) diff --git a/hypothesis-python/tests/conjecture/test_shrinker.py b/hypothesis-python/tests/conjecture/test_shrinker.py index e4b2b40ae1..7c35fcdcbb 100644 --- a/hypothesis-python/tests/conjecture/test_shrinker.py +++ b/hypothesis-python/tests/conjecture/test_shrinker.py @@ -562,7 +562,7 @@ def shrinker(data): assert list(shrinker.buffer) == [1, 0] + [0] * n_gap + [0, 1] -def test_redistribute_block_pairs_with_forced_node(): +def test_redistribute_integer_pairs_with_forced_node(): @run_to_buffer def buf(data): data.draw_integer(0, 100, forced=15) @@ -576,8 +576,8 @@ def shrinker(data): if n1 + n2 > 20: data.mark_interesting() - shrinker.fixate_shrink_passes(["redistribute_block_pairs"]) - # redistribute_block_pairs shouldn't try modifying forced nodes while + shrinker.fixate_shrink_passes(["redistribute_integer_pairs"]) + # redistribute_integer_pairs shouldn't try modifying forced nodes while # shrinking. Since the second draw is forced, this isn't possible to shrink # with just this pass. assert shrinker.buffer == buf diff --git a/hypothesis-python/tests/quality/test_shrink_quality.py b/hypothesis-python/tests/quality/test_shrink_quality.py index fb94d8246d..5106b26fd6 100644 --- a/hypothesis-python/tests/quality/test_shrink_quality.py +++ b/hypothesis-python/tests/quality/test_shrink_quality.py @@ -349,6 +349,19 @@ def test_sum_of_pair(): ) == (1, 1000) +def test_sum_of_pair_separated(): + @st.composite + def separated_sum(draw): + n1 = draw(st.integers(0, 1000)) + draw(st.text()) + draw(st.booleans()) + draw(st.integers()) + n2 = draw(st.integers(0, 1000)) + return (n1, n2) + + assert minimal(separated_sum(), lambda x: sum(x) > 1000) == (1, 1000) + + def test_calculator_benchmark(): """This test comes from https://github.com/jlink/shrinking-challenge/blob/main/challenges/calculator.md, From 84dbaeeb58b939ac1a7f81fdc2ac950c8c84597c Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Mon, 14 Oct 2024 16:33:28 -0400 Subject: [PATCH 6/6] enforce no-zero-weights --- hypothesis-python/src/hypothesis/internal/conjecture/data.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hypothesis-python/src/hypothesis/internal/conjecture/data.py b/hypothesis-python/src/hypothesis/internal/conjecture/data.py index 8945f758b6..53d48f71aa 100644 --- a/hypothesis-python/src/hypothesis/internal/conjecture/data.py +++ b/hypothesis-python/src/hypothesis/internal/conjecture/data.py @@ -2139,6 +2139,9 @@ def draw_integer( # complicates shrinking as we can no longer assume we can force # a value to the unmapped probability mass if that mass might be 0. assert sum(weights.values()) < 1 + # similarly, things get simpler if we assume every value is possible. + # we'll want to drop this restriction eventually. + assert all(w != 0 for w in weights.values()) if forced is not None and (min_value is None or max_value is None): # We draw `forced=forced - shrink_towards` here internally, after clamping.