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

Deprecate use of global PRNG in strategies #3871

Merged
merged 2 commits into from
Feb 4, 2024

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Feb 3, 2024

Fixes #3810.

Copy link
Contributor

@jobh jobh left a comment

Choose a reason for hiding this comment

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

Looks good!

An idea: What if we check more randomness sources, but for performance reasons only occasionally? For example every nth draw, or random spot checks with p<0.1?

I'm saying this because I'd typically reach to np.random instead of math.random myself.

since="RELEASEDAY",
has_codemod=False,
stacklevel=1,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe consider factoring this out as

with checked_random_state():
    result = self.conjecture_data.draw(strategy, observe_as=f"generate:{desc}")
    ...

?

That would avoid the duplication w/control.py, plus improve readability in these central functions

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I like it - and we can check a "drew from st.random_module() flag there too.

@Zac-HD
Copy link
Member Author

Zac-HD commented Feb 4, 2024

An idea: What if we check more randomness sources, but for performance reasons only occasionally? For example every nth draw, or random spot checks with p<0.1? I'm saying this because I'd typically reach to np.random instead of math.random myself.

Unfortunately Hypothesis will then complain about Flaky errors when it goes to shrink such things.

Maybe we check that less frequently? Or only if you set a flag? Actually I should check the perf cost of doing it every draw, it might not be that bad with if "numpy" in sys.modules:...

@jobh
Copy link
Contributor

jobh commented Feb 4, 2024

Unfortunately Hypothesis will then complain about Flaky errors when it goes to shrink such things.

Ah, yes, of course. We would have to flag the test and warn when done, I guess. No need to go that far in this PR IMO, it can be added later.

@Zac-HD Zac-HD merged commit 237bbeb into HypothesisWorks:master Feb 4, 2024
48 checks passed
@Zac-HD Zac-HD deleted the no-random-strategies branch April 1, 2024 04:50
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 if strategies depend on the random module without using st.random_module()
2 participants