Skip to content

Add hypothesis derandomize setting for reproducible builds #206

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

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

jakevdp
Copy link
Contributor

@jakevdp jakevdp commented Nov 14, 2023

@asmeurer
Copy link
Member

Not sure if I necessarily agree that this should be used in the CI here. It is useful for us to see all the errors that can come up. Although I will say it has generally been hard for us to keep up with all the various CI failures in this repo, mostly due to the nature of the test suite that there are expected failures.

@rgommers
Copy link
Member

Although I will say it has generally been hard for us to keep up with all the various CI failures in this repo, mostly due to the nature of the test suite that there are expected failures.

I think we should try a little harder to prevent Hypothesis from using floats like 1.5e+308 or ints like 2**63. It results in a lot of spurious failures that are costing us a lot of time to deal with and make using the test suite harder, but are of little interest. Isn't there something like a "use only reasonable ranges" setting?

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

The one failure is for test_square, which gh-204 deals with. This looks like it's doing the right thing, the CI log contains:

hypothesis profile 'xp_override' -> database=None, deadline=None, derandomize=True

So LGTM, thanks @jakevdp!

Copy link
Member

@honno honno left a comment

Choose a reason for hiding this comment

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

Not sure if I necessarily agree that this should be used in the CI here. It is useful for us to see all the errors that can come up. Although I will say it has generally been hard for us to keep up with all the various CI failures in this repo, mostly due to the nature of the test suite that there are expected failures.

Agree with what you say—I want to mull it over still myself, so removed its use on our CI. The feature add of having --derandomize for adopters to use defo makes sense tho, so lets get this in. Thanks again @jakevdp 🙂

Although I will say it has generally been hard for us to keep up with all the various CI failures in this repo, mostly due to the nature of the test suite that there are expected failures.

I think we should try a little harder to prevent Hypothesis from using floats like 1.5e+308 or ints like 2**63. It results in a lot of spurious failures that are costing us a lot of time to deal with and make using the test suite harder, but are of little interest. Isn't there something like a "use only reasonable ranges" setting?

Yeah we do try avoiding spurious failures, but its been on an adhoc basis when issues have come up with adopters. A blanket "use only reasonable ranges" of Hypothesis would require some re-thinking on how the test suite is architected 🤔

Like 1.5e+308 shouldn't ever be a problem for manipulation functions, so for me it is pretty nice to make sure that is actually the case—on the other hand, yeah we don't really care if some element-wise functions don't play nice with massive floats. And well "nice to have" testing isn't also that important if it shouldn't matter anyway, so I defo see your point.

@honno honno merged commit 6fa42b3 into data-apis:master Nov 15, 2023
@rgommers
Copy link
Member

Like 1.5e+308 shouldn't ever be a problem for manipulation functions, so for me it is pretty nice to make sure that is actually the case

It's very unlikely to fail though, and very unlikely to be real-world relevant. So there's 2 reasons that it's basically a dont-care. I encourage you to think about something like limiting the default range to something like 1e20 for float64, 1e8 for float32, etc. This should be easy to do I'd expect, and a significant improvement in making the test suite more usable/robust, and hence more useful for folks implementing support in their array library.

And well "nice to have" testing isn't also that important if it shouldn't matter anyway, so I defo see your point.

+1

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.

4 participants