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

Warn if search space for inferred strategy is small (no variation) #3668

Merged
merged 16 commits into from
Jun 4, 2023

Conversation

jobh
Copy link
Contributor

@jobh jobh commented Jun 1, 2023

Fixes #3653.

@jobh jobh force-pushed the warn-on-small-searchspace branch from c30b397 to 8cacc9b Compare June 1, 2023 10:54
@jobh jobh force-pushed the warn-on-small-searchspace branch from 0db6d7d to 9b647a8 Compare June 1, 2023 13:41
@jobh jobh force-pushed the warn-on-small-searchspace branch from 9b647a8 to 8f13ecf Compare June 1, 2023 13:43
@jobh jobh force-pushed the warn-on-small-searchspace branch from 64abb34 to b1cc6ef Compare June 2, 2023 07:00
@jobh

This comment was marked as outdated.

@jobh jobh force-pushed the warn-on-small-searchspace branch from dcca72f to 530ae7f Compare June 2, 2023 11:31
@jobh

This comment was marked as outdated.

@jobh jobh force-pushed the warn-on-small-searchspace branch from 1239972 to 24c506d Compare June 2, 2023 11:49
@jobh jobh force-pushed the warn-on-small-searchspace branch from 24c506d to 042c2f5 Compare June 2, 2023 11:58
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for another great PR @jobh! Quite a few comments below sorry, but I think we'll get through them pretty quickly 🚀

hypothesis-python/RELEASE.rst Outdated Show resolved Hide resolved
hypothesis-python/tests/cover/test_annotations.py Outdated Show resolved Hide resolved
Comment on lines 125 to 126
# To avoid SmallSearchSpaceWarning
st.register_type_strategy(Elem, st.builds(Elem))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think SmallSearchSpaceWarning should only be emitted if signature(typ) has parameters other than self (without caring about the name).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think SmallSearchSpaceWarning should only be emitted if signature(typ) has parameters other than self (without caring about the name).

Agreed. I did actually consider this, but skipped it since I didn't know off-hand how to write a simple-yet-robust check for this. I'm sure I can figure it out, but maybe you already know?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an internal get_signature() helper, then check whether the .parameters attribute is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done. A few lines above it is indicated that get_signature.parameters might also have an item for return, but I couldn't see that behaviour in a quick test. So I'm ignoring that possibility, just checking for empty .parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @Zac-HD! I think all issues you raised have been addressed, with only the question above potentially remaining.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can ignore that - it's only possible if you've manually constructed and assigned an illegal signature, because using return as an argument name is of course a syntax error. We'll be able to drop that check at the 3.10 EOL in 2026, thanks to python/cpython#92065.

hypothesis-python/tests/cover/test_lookup.py Outdated Show resolved Hide resolved
hypothesis-python/tests/cover/test_lookup.py Outdated Show resolved Hide resolved
hypothesis-python/tests/numpy/test_from_dtype.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/errors.py Outdated Show resolved Hide resolved
@jobh jobh force-pushed the warn-on-small-searchspace branch 2 times, most recently from 0b6939e to 4a22076 Compare June 4, 2023 15:35
@jobh jobh force-pushed the warn-on-small-searchspace branch from 4a22076 to 1b1d27b Compare June 4, 2023 15:54
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed some final small tweaks, and I think we're ready to merge 🚀

@Zac-HD Zac-HD enabled auto-merge June 4, 2023 17:01
It looks like we're exposing existing problems now, so revisit in a future issue.
@Zac-HD Zac-HD merged commit 591dcbe into HypothesisWorks:master Jun 4, 2023
@Zac-HD
Copy link
Member

Zac-HD commented Jun 4, 2023

@jobh you should find an invite in your email to join the Hypothesis team. There's no obligation to accept, nor to do anything having accepted, but rather an offer to continue hanging around and contributing to the project in whatever way you wish to - and the right to click "merge" on PRs when you think they meet the standards laid out in our contributor guides.

Regardless, thanks again for the amazing stuff you've done over the last week or two!

@jobh
Copy link
Contributor Author

jobh commented Jun 5, 2023

@jobh you should find an invite in your email to join the Hypothesis team. There's no obligation to accept, nor to do anything having accepted, but rather an offer to continue hanging around and contributing to the project in whatever way you wish to - and the right to click "merge" on PRs when you think they meet the standards laid out in our contributor guides.

Regardless, thanks again for the amazing stuff you've done over the last week or two!

Thanks for the kind words @Zac-HD , and thanks for the invite. I will accept, but I don't know yet how active I will be — I haven't actually started using it yet, but it's been a fun experience so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn (or fail) when inferred strategy is the trivial default object
2 participants