pschanely / hypothesis-crosshair

Level-up your Hypothesis tests with CrossHair
MIT License
8 stars 0 forks source link

`CrosshairInternal` on `st.dates` / `st.datetimes` #13

Closed tybug closed 4 months ago

tybug commented 5 months ago

Somehow, kwargs["max_value"] (and only max_value) on an integer ir node becomes symbolic here and errors when we go to cache based off it. This only happens on dates/datetimes so I'm suspecting crosshair interception there causes some of our code to be colored symbolically, but hypothesis' date strategy code may also be at fault.

@given(st.dates())
def f(d):
    pass
f()
  ... (truncated)
  File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/internal/conjecture/engine.py", line 475, in test_function
    self._cache(data)
  File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/internal/conjecture/engine.py", line 381, in _cache
    self.__data_cache_ir[key] = result
    ~~~~~~~~~~~~~~~~~~~~^^^^^
  File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/internal/cache.py", line 111, in __setitem__
    i = self.keys_to_indices[key]
        ~~~~~~~~~~~~~~~~~~~~^^^^^
  File "/opt/homebrew/lib/python3.12/site-packages/crosshair/libimpl/builtinslib.py", line 1143, in __hash__
    return self.__index__().__hash__()
           ^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.12/site-packages/crosshair/libimpl/builtinslib.py", line 1154, in __index__
    space = context_statespace()
            ^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.12/site-packages/crosshair/statespace.py", line 247, in context_statespace
    raise CrosshairInternal
crosshair.util.CrosshairInternal

patch

Here's a logging patch that narrows the problem, if it's helpful:

diff --git a/hypothesis-python/src/hypothesis/internal/conjecture/engine.py b/hypothesis-python/src/hypothesis/internal/conjecture/engine.py
index eb326f59a..708e48904 100644
--- a/hypothesis-python/src/hypothesis/internal/conjecture/engine.py
+++ b/hypothesis-python/src/hypothesis/internal/conjecture/engine.py
@@ -456,6 +456,7 @@ class ConjectureRunner:
                 if self.settings.backend != "hypothesis":
                     for node in data.examples.ir_tree_nodes:
                         value = data.provider.post_test_case_hook(node.value)
+                        print("kwargs should not be symbolic", type(node.kwargs["max_value"]))
                         expected_type = {
                             "string": str,
                             "float": float,

With this, I get

kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'int'>
kwargs should not be symbolic <class 'crosshair.libimpl.builtinslib.SymbolicInt'>

followed by the above stacktrace error.

pschanely commented 5 months ago

Oh this is fun. So I think hypothesis generates a max_value bound when it draws the integer for the day part (because the max day number varies depending on the year and month) - here. I hadn't anticipated that the draw parameters might be symbolic, but after a skim of the code, I think this should still work from the crosshair side.

I'd look for you on suggestions about what to do next though. We might just need to realize every key or something?

tybug commented 5 months ago

Nice call, that's what's happening. We'll run into similar issues with dependent draws in user strategies too (eg draw an int then draw a list of that size), so we'll have to support symbolic kwargs.

I think in principle we can just realize the kwargs...I get a new error when trying to do so though:

```patch diff --git a/hypothesis-python/src/hypothesis/internal/conjecture/engine.py b/hypothesis-python/src/hypothesis/internal/conjecture/engine.py index eb326f59a..b5584d3db 100644 --- a/hypothesis-python/src/hypothesis/internal/conjecture/engine.py +++ b/hypothesis-python/src/hypothesis/internal/conjecture/engine.py @@ -467,9 +467,11 @@ class ConjectureRunner: raise HypothesisException( f"expected {expected_type} from " f"{data.provider.post_test_case_hook.__qualname__}, " - f"got {type(value)} ({value!r})" + f"got {type(value)}" # printing symbolic values = bad ) + node.value = value + node.kwargs = {k: data.provider.post_test_case_hook(v) for k, v in node.kwargs.items()} self._cache(data) if data.invalid_at is not None: # pragma: no branch # coverage bug? ``` ```python You can add @seed(58283928597073102201375559339070102338) to this test to reproduce this failure. Traceback (most recent call last): File "/opt/homebrew/lib/python3.12/site-packages/hypothesis_crosshair_provider/crosshair_provider.py", line 100, in per_test_case_context_manager yield File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/core.py", line 964, in execute_once result = self.test_runner(data, run) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/core.py", line 737, in default_executor return function(data) ^^^^^^^^^^^^^^ File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/core.py", line 875, in run kw, argslices = context.prep_args_kwargs_from_strategies( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/control.py", line 157, in prep_args_kwargs_from_strategies obj = check(self.data.draw(s, observe_as=f"generate:{k}")) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/internal/conjecture/data.py", line 2425, in draw return strategy.do_draw(self) ^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/strategies/_internal/lazy.py", line 167, in do_draw return data.draw(self.wrapped_strategy) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/internal/conjecture/data.py", line 2419, in draw return strategy.do_draw(self) ^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/strategies/_internal/datetime.py", line 148, in do_draw result = self.draw_naive_datetime_and_combine(data, tz) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/strategies/_internal/datetime.py", line 162, in draw_naive_datetime_and_combine result = draw_capped_multipart(data, self.min_value, self.max_value) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/strategies/_internal/datetime.py", line 117, in draw_capped_multipart val = data.draw_integer(low, high) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tybug/Desktop/Liam/coding/hypothesis/hypothesis-python/src/hypothesis/internal/conjecture/data.py", line 2091, in draw_integer value = self.provider.draw_integer( ^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/opt/homebrew/lib/python3.12/site-packages/hypothesis_crosshair_provider/crosshair_provider.py", line 178, in draw_integer symbolic = proxy_for_type(int, self._next_name("int"), allow_subtypes=False) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/opt/homebrew/lib/python3.12/site-packages/crosshair/core.py", line 606, in proxy_for_type return proxy_factory(recursive_proxy_factory, *type_args) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/opt/homebrew/lib/python3.12/site-packages/crosshair/libimpl/builtinslib.py", line 4069, in make if space.fork_parallel( ^^^^^^^^^^^^^^^^^^^^ File "/opt/homebrew/lib/python3.12/site-packages/crosshair/statespace.py", line 791, in fork_parallel assert isinstance(node, ParallelNode) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError while generating 'd' from datetimes() ```

any ideas?

tybug commented 4 months ago

well, I have something that works, but I doubt you'll be happy about it šŸ˜„

I think the root cause of the above error is nondeterminism from hypothesis operating on symbolic kwargs, before we do anything with them. For instance, caching kwargs to reduce memory.

I've worked around this by realizing all symbolic kwargs as soon as we get them, via post_test_case_hook (renamed realize): https://github.com/HypothesisWorks/hypothesis/compare/master...tybug:hypothesis:backend-realize?expand=1. If I understand correctly this handicaps crosshair's ability to reason about the value after the point this occurs. At the same time, I'm not sure avoiding prematurely realizing kwargs is practical in the hypothesis engine...though @Zac-HD may have opinions there. For instance, we'd probably have to disable this pooling behavior.

Here's a minimal example exhibiting this behavior (currently broken, fixed with the above branch):

@given(st.data())
def f(data):
    n = data.draw(st.integers())
    n2 = data.draw(st.integers(n, n))
f()

Under the branch, I believe n would become non-symbolic as soon as it's passed to a strategy, preventing crosshair from reasoning about it further.

Zac-HD commented 4 months ago

well... strategies where the arguments are provided by other strategies is a core feature for Hypothesis - see for example the .flatmap() method! We're going to have to find some way of dealing with this, at whatever cost to our internals šŸ« 

tybug commented 4 months ago

Would a avoid_realization = False variable that backends like crosshair can set to True be acceptable? We can disable anything that would realize if set, like DataTree or kwargs pooling. I'd like to avoid disabling optimizations for other (slightly more sane šŸ˜„) backends that don't need this.

Zac-HD commented 4 months ago

Yeah, that seems pretty good to me - and skipping DataTree when it's basically redundant with crosshair's internals seems pretty reasonable tbh. Or maybe we can insert into the DataTree after we realize the sequence at the end of the test?

tybug commented 4 months ago

and skipping DataTree when it's basically redundant with crosshair's internals

I also thought this at first, but if we mix backends in the future it'd be nice to share deduplication information! That's far enough away that I'm not thinking about it too much though. post-inserting into DataTree is probably doable, though there's enough subtle interactions there that I'd have to try it to see.

tybug commented 4 months ago

Seems to work well enough: https://github.com/HypothesisWorks/hypothesis/compare/master...tybug:hypothesis:backend-realize?expand=1. I also had to disable @cacheable strategies - we'll see how bad of a performance hit that is šŸ˜„

tybug commented 4 months ago

works with hypothesis-crosshair v0.0.7 šŸ‘ (+ the above branch)