Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stateful mode filtering a lot with one_of(Bundle, Bundle) #2078

Closed
touilleMan opened this issue Aug 23, 2019 · 4 comments · Fixed by #2259
Closed

Stateful mode filtering a lot with one_of(Bundle, Bundle) #2078

touilleMan opened this issue Aug 23, 2019 · 4 comments · Fixed by #2259
Labels

Comments

@touilleMan
Copy link
Member

@touilleMan touilleMan commented Aug 23, 2019

Hi,

Considering the following statemachine:

from hypothesis.stateful import Bundle, RuleBasedStateMachine, rule
from hypothesis.strategies import text

class StateMachine(RuleBasedStateMachine):
    Things = Bundle("things")
    Stuff = Bundle("stuff")
    StuffAndThings = Things | Stuff

    @rule(target=Things, name=text())
    def create_thing(self, name):
        return name

    @rule(target=Stuff, name=text())
    def create_stuff(self, name):
        return name

    @rule(item=StuffAndThings)
    def do(self, item):
        return

TestStateMachine = StateMachine.TestCase

Wich gave:

$ py.test test_statemachine.py::TestStateMachine --hypothesis-show-statistics
[...]
test_statemachine.py::TestStateMachine::runTest:

  - 100 passing examples, 0 failing examples, 52 invalid examples
  - Typical runtimes: 0-31 ms
  - Fraction of time spent in data generation: ~ 1%
  - Stopped because settings.max_examples=100

However if I replace @rule(item=StuffAndThings) by @rule(item=Stuff) I end up with

$ py.test test_statemachine.py::TestStateMachine --hypothesis-show-statistics
[...]
test_statemachine.py::TestStateMachine::runTest:

  - 100 passing examples, 0 failing examples, 0 invalid examples
  - Typical runtimes: 0-31 ms
  - Fraction of time spent in data generation: ~ 0%
  - Stopped because settings.max_examples=100

So using a one_of(Bundle, Bundle) in a rule seems to filter out a lot !

I guess there is a good reason about this, but I couldn't find anything in the doc about this.

As a matter of fact I didn't expect Hypothesis to drop examples without explicitly using filter and assume. Here again, there must be a reason, but it's far from obvious to me 😃

Do you think it would make sens to document those two behaviors ?

@DRMacIver

This comment has been minimized.

Copy link
Member

@DRMacIver DRMacIver commented Aug 23, 2019

I've not looked into this in detail, but my guess based on knowing how the code works would be that the following is what's happening:

  1. When a Bundle is empty (because nothing has been added to it yet) attempting to drawing from it causes a rejection because there's nothing that can be done.
  2. When drawing from a Bundle at the top level this is handled correctly by skipping over it, but when using it in a strategy this may not be.
  3. The result is that when drawing from a one_of(Bundle, Bundle) there are a lot of attempts to draw from an empty bundle.

If this is what's happening, there's a two part fix available:

  • Make it so that when the bundle is empty it shows up as an empty strategy (one that can't be successfully drawn from)
  • Make it so that one_of skips over empty children during drawing.

However this has a significant problem: The way the code for empty strategies currently works assumes that emptiness doesn't change once it's been established. That would no longer be the case, so we'd have to make some non-trivial internal refactorings to how that information is computed and accessed.

Do you think it would make sens to document those two behaviors ?

I think the behaviour with Bundles is just a bug so we should fix it rather than document it. It would probably be worth clarifying that examples can be invalid for all sorts of internal reasons even without an explicit filter or assume though, yeah.

@DRMacIver

This comment has been minimized.

Copy link
Member

@DRMacIver DRMacIver commented Aug 23, 2019

The way the code for empty strategies currently works assumes that emptiness doesn't change once it's been established.

Actually this is very easy to work around.

We can add a method:

def available(self, data):
    """Returns whether this strategy can *currently* draw any
    values."""
   return not self.empty

Then individual strategies (currently just Bundle) can override this. Then one_of and potentially other places that we use empty can switch over to calling strategy.available(data) if appropriate.

@touilleMan

This comment has been minimized.

Copy link
Member Author

@touilleMan touilleMan commented Aug 23, 2019

@DRMacIver Thanks a lot for the quick&complete answer ;-)

I've created PR #2079 which implement your suggestion, it indeed greatly reduce the number of rejected items !
I guess this PR lacks of tests before being merged, but no sure how to do that...

  • create a state machine check the example error ratio (seems a pretty hacky solution)
  • Manipulate Bundle alone in the test, something like:
bundle = Bundle('foo')
assert not bundle.available()
bundle.add(42)
assert bundle.available()

But I don't think we can directly do that (need a data object to draw examples, and doing Bundle('foo').example() out of the blue ends up with an hypothesis.errors.InvalidArgument: Cannot use runner() strategy with no associated runner or explicit default. exception...

  • create a one_of(Bundle('foo') | Bundle('bar')) strategy and test it directly, not sure if it's possible though
@Zac-HD

This comment has been minimized.

Copy link
Member

@Zac-HD Zac-HD commented Aug 24, 2019

However this has a significant problem: The way the code for empty strategies currently works assumes that emptiness doesn't change once it's been established. That would no longer be the case, so we'd have to make some non-trivial internal refactorings to how that information is computed and accessed.

As a related problem, currently-empty non-Bundle strategies may also have some problems related to changes over time. Consider the following:

In [1]: from hypothesis.strategies import *

In [2]: LS = []

In [3]: s = deferred(lambda: sampled_from(LS))

In [4]: s
Out[4]: deferred(lambda: sampled_from(LS))

In [5]: s.is_empty
    HypothesisDeprecationWarning: sampled_from() with nothing to sample is deprecated
    and will be an error in a future version.  It currently returns `st.nothing()`,
    which if unexpected can make parts of a strategy silently vanish.
Out[5]: True

In [6]: LS.append(1)

In [7]: s.is_empty
Out[7]: True

In [8]: s.example()
    Unsatisfiable: Unable to satisfy assumptions of hypothesis example_generating_inner_function.

In [9]: s.wrapped_strategy
Out[9]: sampled_from([1])

In [10]: s.wrapped_strategy.example()
    Unsatisfiable: Unable to satisfy assumptions of hypothesis example_generating_inner_function.

So I suspect that while a quick patch is worthwhile, we will need a deeper set of changes to fix the underlying issue.

@Zac-HD Zac-HD added the performance label Aug 24, 2019
touilleMan added a commit to touilleMan/hypothesis that referenced this issue Sep 2, 2019
touilleMan added a commit to touilleMan/hypothesis that referenced this issue Sep 2, 2019
touilleMan added a commit to touilleMan/hypothesis that referenced this issue Sep 26, 2019
touilleMan added a commit to touilleMan/hypothesis that referenced this issue Dec 4, 2019
touilleMan added a commit to touilleMan/hypothesis that referenced this issue Dec 4, 2019
touilleMan added a commit to touilleMan/hypothesis that referenced this issue Dec 5, 2019
@Zac-HD Zac-HD closed this in #2259 Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.