-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
TST introducing the random_seed fixture #22749
Conversation
If we agree that this is a good idea, I will open a meta-issue to progressively convert existing tests to use that fixture. Each time that will require running the test locally with:
if all seeds pass, then fine it's an easy PR. If it it's not the case, it will be revealing that the test is too brittle to use this fixture. This will require investigating if the test can be more robust by changing some hyper-parameters or increasing the size of a dataset or similar. If it's still too hard to make the test pass for any admissible seed, then we might want to keep it with a fixed seed but add a comment to explain that this test to acknowledge that this test is quite sensitive to the specific choice of the seed and move on to the next test. |
Hum, we have a bad interaction between the use of pytest-xdist and this new fixture:
Instead of picking a seed completely at random, I will have to make it depend on the year and the day for instance. |
I think I chose the right day to write this PR: >>> from random import Random
>>> from datetime import datetime
>>> RANDOM_SEED_RANGE = list(range(100))
... rng = Random(int(datetime.now().strftime("%Y%j")))
... rng.choice(RANDOM_SEED_RANGE)
42 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pytest.fixture
all the tests!
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
FIX Adds random_seed as a plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if the CI passes.
Great, the report header shows up on azure ! but now circleci breaks :'( |
Indeed I was afraid this might happen. It will also break when people run the tests from outside of the source folder, e.g. for instance when testing the installed wheels in |
I have edited the description of the PR with a TODO list of things to finalize before considering the merge of this PR. |
I think the the random seed should be fixed by default. With a non-fixed seed, tests in PRs can fail randomly because a different seed is used. |
This is the goal of this PR: to make sure that our CI explores over time all the seeds that we are expected to support so as to make sure that we will not have regressions or test code updates that reintroduce seed-specificity by mistake without realizing it. If that happens, it's easy to read the log of the CI to identify that test and run it locally to with the |
That should never happen since we will add the fixture to a test iff the test passes when setting "all". Then, if a PR does not touch the test or the code that this test cover, it wont fail on any seed. |
Indeed that's a valid point.
Also a valid point.
Let's instead have a dedicated explicit marker This will not be the default, but I will configure the CI to use it in the scheduled nightly builds. |
It could happen if a seed-specificity regression is introduced silently in |
Done. This is now silent by default and only display the seed in the pytest report when enabling the global seed randomization. I tested it locally and it works but it should now be disabled in the PRs. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
We could also define This might be a good trade-off between computational speed and protection against seed-sensitivity. |
Weird: the import lines of the new fixture are not covered because the pytest coverage plugin kicks in after this plugin has been imported: https://app.codecov.io/gh/scikit-learn/scikit-learn/compare/22749/diff :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I like the idea of setting different random seeds for different CI configurations.
I pushed that. For now I decided not to use range seeds to keep the CI build times unchanged. I will merge if green. |
Merged! Thanks all for the reviews and contributions to improve this fixture. I will open an issue to give guidelines to start using this fixture in the scikit-learn code base. |
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Closes #13913
This is a new fixture similar to what is being developed in #22690 that will make it possible to ensure that (some of) our tests do no rely on a specific value of a random seed while giving full control to make it possible to reproduce CI runs locally or to make it possible to use
The docstring of the fixture should be quite explicit. I tested it locally and it seems to work as expected:
TODO:
sklearn/conftests.py
?