Merge Bundle as subclass of SampledFromStrategy
#4084
Conversation
reaganjlee
force-pushed
the
consumes
branch
2 times, most recently
from
6c08dc0
to
e2e7f93
Compare
|
👋 thanks for taking this on Reagan! Quick note: I generally review PRs when either CI is green, or the contributor explicitly requests a review; happy to do that here too. (although I'll be offline from later this week until early September for a camping trip and thus not reviewing then) |
|
Of course and thanks for the note! I'll make a few more attempts at this final test and let you know otherwise. Either way hope you have a good time on your trip! |
|
@Zac-HD Mind taking a look to see if it's going in the right direction? Please excuse all the extra |
hypothesis-python/src/hypothesis/strategies/_internal/strategies.py
Outdated
Show resolved
Hide resolved
|
Don't think I fully understand the logs |
There was a problem hiding this comment.
Forgive the somewhat rambly notes, I figured reviewed was better than not 🙂
Overall looks pretty good, though I think it'd be helpful to have a test where we define a consumes bundle which can recursively draw from itself (🤯) no, that makes no sense, bundles just pick a known element so they can't be recursive.
The These lines were always and only run by failing examples thing is from Phase.explain; if you disable that in the settings for these tests it'll stop.
Finally, thanks again Reagan - this has turned out to be a lot tricker than I was expecting, and I really appreciate your contribution here! I'm sure many users of stateful testing will agree with me too.
| self.elements = bundle | ||
| self.reference_to_value = partial( | ||
| self.reference_to_val_func, machine.names_to_values | ||
| ) | ||
|
|
||
| ref = super().do_draw(data) | ||
| if self.consume: | ||
| bundle.remove(ref) # pragma: no cover # coverage is flaky here |
There was a problem hiding this comment.
I'd be inclined to restore our placeholder self.elements here.
I'm also wondering what happens if you have a bundle which holds a tree of some kind. Is it possible to get an indexing error from recursive references if something is consumed at the wrong time? I'll need to think about this; we might be fine or we might need something a touch more complicated for consumes bundles...
There was a problem hiding this comment.
I think there's also a latent bug here where bundle.remove(ref) can remove a different object to the one we selected, if they're equal, which could be a really subtle problem for someone.
The solution is probably to do elements = range(len(bundle))[::-1] and modify reference_to_val_func to also accept the bundle list and do a two-step lookup + record the index, and finally pop from that index. Ouch.
hypothesis-python/src/hypothesis/strategies/_internal/strategies.py
Outdated
Show resolved
Hide resolved
| res = self.elements[i] | ||
| if self.reference_to_value: | ||
| value = self.reference_to_value(res) | ||
| if self._transform(value) is filter_not_satisfied: | ||
| return filter_not_satisfied | ||
| return res | ||
| return self._transform(res) |
There was a problem hiding this comment.
I think we still want to self._transform on line 564, right? So something like
| res = self.elements[i] | |
| if self.reference_to_value: | |
| value = self.reference_to_value(res) | |
| if self._transform(value) is filter_not_satisfied: | |
| return filter_not_satisfied | |
| return res | |
| return self._transform(res) | |
| value = self.elements[i] | |
| if self.reference_to_value: | |
| value = self.reference_to_value(value) | |
| return self._transform(value) |
| speculative_index = data.draw_integer( | ||
| 0, max_good_indices - 1, shrink_towards=self.shrink_towards | ||
| ) |
There was a problem hiding this comment.
Hmm... having gone to all this effort to plumb through shrink_towards; I wonder if we actually just want a reverse_elems boolean so that our speculation will efficiently work from the end that we want to shrink to.
Or. Having said that, we could just self.elements = bundle[::-1] in Bundle.do_draw 🤦♂️
|
Thanks so much for the fast review! I'll continue editing tomorrow but hope you have a great time on your trip! |
|
After taking some time to think about it, I can't seem to make a useful recursive test. Any thoughts here? Maybe a potential use case would be helpful and I can see if I can finish the rest. Also the last failing tests seems unrelated |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Having just tried to sketch a 'recursive bundle'... there's no such thing, bundles draw from a flat list of values that can't include invoking bundle draws 🤦. So I think all we need is to see CI passing without those weird timeouts, and then I'll do a from-scratch review to see where we're up to - probably pretty close! If you want to get something out sooner, I'd be happy to take a refactoring-only PR which does the "delete BundleReference" etc cleanups; that should be pretty straightforward and would shrink this one a bit for easier handling and review. Not doing that is also fine of course! |
reaganjlee
force-pushed
the
consumes
branch
6 times, most recently
from
ae7e0de
to
080653b
Compare
|
Should be good for review now! |
reaganjlee
changed the title
Bundle as subclass of SampledFromStrategyBundle as subclass of SampledFromStrategy
| def get_element(self, i): | ||
| return self._transform(self.elements[i]) | ||
| element = self.elements[i] | ||
| value = self._transform(self.reference_to_value(element)) | ||
| if is_identity_function(self.reference_to_value): | ||
| return value | ||
| return element if value is not filter_not_satisfied else filter_not_satisfied |
There was a problem hiding this comment.
Instead of expanding this, I think we can define Bundle.get_element() - and then also drop the _SHRINK_TOWARDS changes by having that new method index from the end instead of the beginning of the sampled sequence.
| # We use both self.bundle and self.elements to make sure an index is used to safely pop | ||
| self.elements = range(len(self.bundle)) | ||
|
|
||
| idx = super().do_draw(data) | ||
| reference = self.bundle[idx] |
There was a problem hiding this comment.
I think this is missing handling for the .map() case; we need to use .get_element() for that - and I guess either return (value, index) or track the latest index as state on the class.
Seems like the initial values of the states get reversed with the new implementation where e.g.
state.fail_fast(a1=a_5, a2=a_4, a3=a_3, a4=a_2, a5=a_1, a6=a_0, b1=b_2, b2=b_1, b3=b_0)become...(a1=a_0, a2=a_1, a3=a_2, a4=a_3, a5=a_4, a6=a_5, b1=b_0, b2=b_1, b3=b_2). I'll probably just need to just spend more time looking into theRule strategyandSampledFromStrategyCloses #3944