ENH Simplify pytest global random test plugin #27963

lesteve · 2023-12-14T17:00:36Z

Reference Issues/PRs

@Charlie-XIAO can you double-check this fixes it for you? This fixes it in @fcharras Windows VM and mine too, but one more confirmation would be nice.

What does this implement/fix? Explain your changes.

I think we were in a weird edge-case of pytest by combining registering a plugin in the sklearn tree calling it via setup.cfg and sometimes registering inside conftest.py some reason. It seems like by moving all the code to conftest.py we avoid the issue.

pytest_generate_tests seems a slightly simpler way to parameterize the global_random_seeds tests based on the variable SKLEARN_TESTS_GLOBAL_RANDOM_SEED.

cc @jeremiedbb who was involved in the global random seed plugin originally IIRC #22749. Edit: actually looks like @ogrisel was the one who created the global random seed plugin.

github-actions · 2023-12-14T17:01:55Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 1801de0. Link to the linter CI: here}

Charlie-XIAO · 2023-12-15T03:02:56Z

@lesteve Yes I confirm this PR fixes the issue also on my local machine. Thank you very much!

…nto simplify-pytest-plugin

lesteve · 2024-01-03T06:21:23Z

As asked by @jeremiedbb I double-checked that the following command works (he remembered some issues when working on the original PR):

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='1-10' pytest --pyargs sklearn.tests.test_dummy

The only weird behaviours I noticed:

pytest --pyargs sklearn.tests.test_dummy doesn't print the header with the global random seed info, but the global random seed is set correctly. My guess is that in this case sklearn/conftest.py is run but too late to add a header
as I mentioned in one of the comment pytest --pyargs sklearn.tests.test_dummy -n2 does not seem to run pytest_configure_node so in the 'any' case we set a fixed random_state (the default value). I don't think this matters that much in practice.

ogrisel

I think fixing the "any" case with xdist is important. See below:

sklearn/conftest.py

ogrisel · 2024-01-10T16:37:26Z

sklearn/conftest.py

+            # In these edge cases, random_seeds is set to a fixed value
+            random_seeds = getattr(
+                config.workerinput, "random_seeds", default_random_seeds
+            )


It does not seem to pickup the seed randomly selected by the controller:

SKLEARN_TESTS_GLOBAL_RANDOM_SEED="any" pytest -n 8 -v sklearn/cluster/tests/test_k_means.py -k test_sample_weight_init ====================================================================================================== test session starts ======================================================================================================= platform darwin -- Python 3.11.6, pytest-7.4.2, pluggy-1.3.0 -- /Users/ogrisel/miniforge3/envs/dev/bin/python3.11 cachedir: .pytest_cache To reproduce this test run, set the following environment variable: SKLEARN_TESTS_GLOBAL_RANDOM_SEED="89" See: https://scikit-learn.org/dev/computing/parallelism.html#sklearn-tests-global-random-seed rootdir: /Users/ogrisel/code/scikit-learn configfile: setup.cfg plugins: repeat-0.9.2, anyio-4.0.0, cov-4.1.0, xdist-3.3.1 8 workers [2 items] scheduling tests via LoadScheduling sklearn/cluster/tests/test_k_means.py::test_sample_weight_init[42-random] sklearn/cluster/tests/test_k_means.py::test_sample_weight_init[42-k-means++] [gw0] [ 50%] PASSED sklearn/cluster/tests/test_k_means.py::test_sample_weight_init[42-random] [gw1] [100%] PASSED sklearn/cluster/tests/test_k_means.py::test_sample_weight_init[42-k-means++] ======================================================================================================= 2 passed in 1.69s ========================================================================================================

It's always running the tests with 42 while it mentions 89 in the test report header message.

It does work as expected when I do not enable xdist though:

====================================================================================================== test session starts ======================================================================================================= platform darwin -- Python 3.11.6, pytest-7.4.2, pluggy-1.3.0 -- /Users/ogrisel/miniforge3/envs/dev/bin/python3.11 cachedir: .pytest_cache To reproduce this test run, set the following environment variable: SKLEARN_TESTS_GLOBAL_RANDOM_SEED="82" See: https://scikit-learn.org/dev/computing/parallelism.html#sklearn-tests-global-random-seed rootdir: /Users/ogrisel/code/scikit-learn configfile: setup.cfg plugins: repeat-0.9.2, anyio-4.0.0, cov-4.1.0, xdist-3.3.1 collected 280 items / 278 deselected / 2 selected sklearn/cluster/tests/test_k_means.py::test_sample_weight_init[82-k-means++] PASSED [ 50%] sklearn/cluster/tests/test_k_means.py::test_sample_weight_init[82-random] PASSED [100%] =============================================================================================== 2 passed, 278 deselected in 0.17s ================================================================================================

Hum this is what you commented above:

as I mentioned in one of the comment pytest --pyargs sklearn.tests.test_dummy -n2 does not seem to run pytest_configure_node so in the 'any' case we set a fixed random_state (the default value). I don't think this matters that much in practice.

I don't understand why you think it does not matter. The "any" mode is used by our nightly CI to discover unexpected seed-sensitive test failures over time. I think it's important.

As I commented above this is not related to the use of --pyargs: I do not use it in the reproducer.

Thanks for testing, I thought I tested a variety of combinations:

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest sklearn -n2

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest --pyargs sklearn -n2

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest sklearn/tests/test_dummy.py -n2

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest --pyargs sklearn.tests.test_dummy -n2

and only 4. did not work for some unknown reason, which is why I said it is not that important.

Apparently it is more complicated than this, so I need to check again!

So I fixed the bug I think. I was using getattr(config.workerinput, ...) rather than config.workerinput.get ...

I tested the cases mentioned in my previous comment and now only 4. does not work (as expected).

For completeness, I believe the issue is actually pytest-dev/pytest-xdist#917. pytest_configure is not run in the xdist controller in 4. for some reason so the mechanism to create the random seed in the main controller and access it in the xdist workers does not work.

Would it be possible to generate the seed in each worker instead of having to use the controller?

I am thinking of something like calling the random number generator with a shared seed in each worker. To have a shared seed we need some form of state that is easily accessed by each worker. I don't know if such a thing exists in pytest, maybe each run is assigned some kind of "run ID" or something?

If each worker can derive the correct seed itself we don't need to have to use the controller to coordinate the workers.

Maybe https://pytest-xdist.readthedocs.io/en/stable/how-to.html#uniquely-identifying-the-current-test-run?

Interesting idea, I'll look into it!

Thinking about it, you actually want the controller to know about the seed, be it only to display it in pytest_report_header.

I looked a bit at PYTEST_XDIST_TESTRUNUID env variable but it is only available in the workers and not the controller, there is the testrun_uid fixture but I am not sureit can be made to fit nicely with the pytest_generate_tests with metafunc thing.

…nto simplify-pytest-plugin

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

…nto simplify-pytest-plugin

lesteve · 2024-06-03T12:04:58Z

Question from Discord by @thomasjpfan:

We ended up with the current implemention to get --pyargs sklearn.tests.test_dummy to work.

Was the PR updated to work for this use case?

The only thing that does not quite work as well as main is that the following:

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest --pyargs sklearn.tests.test_dummy -n2 -v

will always use 42 as the random seed. The underlying Pytest issue seems to be pytest-dev/pytest-xdist#917.

Apart from using any anything other settings of the global random seed will work for example:

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='1-10' pytest --pyargs sklearn.tests.test_dummy -n2 -v

I think the edge case is used seldom enough that it is not such an issue compared to the advantages:

everything is in conftest.py, rather than having a plugin activated in setup.cfg and also in conftest.py
comments have been added to document the edge cases
pytest_generate_tests(metafunc) is a neater way to parametrise the test than the closure that was previously used

thomasjpfan

@lesteve I agree this PR is a cleaner solution. In the nightly builds, we set SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' here:

scikit-learn/build_tools/azure/test_script.sh

Line 14 in a67ebbe

export SKLEARN_TESTS_GLOBAL_RANDOM_SEED="any"

which will be used with --pyargs:

scikit-learn/build_tools/azure/test_script.sh

Line 63 in a67ebbe

TEST_CMD="$TEST_CMD --pyargs sklearn"

After this PR is merged, will this seed always be 42?

…nto simplify-pytest-plugin

lesteve · 2024-06-23T05:31:58Z

@thomasjpfan thanks a lot for taking a look 🙏, this PR is definitely not completely trivial to review! I still think it kind of "simplifies" things a bit, but each time I have a new look, it takes me a bit of time to put things back in my short-term memory 😅 ...

After this PR is merged, will this seed always be 42?

With this PR merged, everything will work the same as previously: the CI will pick a random seed each time.

For example, this does work locally so I this should work in the CI:

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest --pyargs sklearn -n2 -k test_median_strategy_regressor -v

Here is the ouptut, notice SKLEARN_TESTS_GLOBAL_RANDOM_SEED="31" in the header and the test does use 31: test_median_strategy_regressor[31]:

=========================================================================================================== test session starts ===========================================================================================================
platform linux -- Python 3.12.2, pytest-8.2.2, pluggy-1.5.0 -- /home/lesteve/micromamba/envs/scikit-learn-dev/bin/python
cachedir: .pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase(PosixPath('/home/lesteve/dev/scikit-learn/.hypothesis/examples'))
To reproduce this test run, set the following environment variable:
    SKLEARN_TESTS_GLOBAL_RANDOM_SEED="31"
See: https://scikit-learn.org/dev/computing/parallelism.html#sklearn-tests-global-random-seed
rootdir: /home/lesteve/dev/scikit-learn
configfile: setup.cfg
plugins: anyio-4.2.0, hypothesis-6.97.3, cov-4.1.0, xdist-3.5.0, repeat-0.9.3
2 workers [1 item]      
scheduling tests via LoadScheduling

sklearn/tests/test_dummy.py::test_median_strategy_regressor[31] 
[gw0] [100%] PASSED sklearn/tests/test_dummy.py::test_median_strategy_regressor[31] 

===================================================================================================== 1 passed, 170 warnings in 7.76s =====================================================================================================

Full disclosure: if you run the same command outside of the scikit-learn root folder, you will be in the pathological case and get SKLEARN_TESTS_GLOBAL_RANDOM_SEED="42".

I have never noticed this, but the current behaviour of main is an error though, so I guess this is not something that we ever do, and this PR is an improvement even in this "not quite perfectly working" case?

The error comes from the fact that random seeds are different in the two workers:

__________________________________________________________________________________________________________ ERROR collecting gw1 ___________________________________________________________________________________________________________
Different tests were collected between gw0 and gw1. The difference is:
--- gw0

+++ gw1

@@ -1 +1 @@

-dev/scikit-learn/sklearn/tests/test_dummy.py::test_median_strategy_regressor[88]
+dev/scikit-learn/sklearn/tests/test_dummy.py::test_median_strategy_regressor[20]

lesteve · 2024-06-23T05:46:43Z

Let me try to do some kind of summary to make it easier.

To be explicit, this is when running from scikit-learn root folder, which I think is 90-99% of our usage.

	`main`	this PR
full tests + path	works	works
full tests + `--pyargs`	works	works
partial tests + path	works	works
partial tests + `--pyargs`	works	global_random_seed=42

I would argue the partial tests + --pyargs with SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' case is not something we care too much about. Using paths is more convenient because for example you have tab-completion that works.

Here is an explanation of the different cases:

full tests with path

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest sklearn -n2 -k test_median_strategy_regressor -v

full tests with --pyargs

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest --pyargs sklearn -n2 -k test_median_strategy_regressor -v

partial tests with path

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest sklearn/tests/test_dummy.py -n2 -k test_median_strategy_regressor -v

partial tests with --pyargs

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest --pyargs sklearn.tests.test_dummy -n2 -k test_median_strategy_regressor -v

thomasjpfan · 2024-06-25T19:28:00Z

In the CI, we do move out of the root directory:

https://github.com/scikit-learn/scikit-learn/blob/a67ebbebc173007735e62eef7878c08435d28d89/build_tools/azure/test_script.sh#L31C1-L31C13

Does this PR work when pytest is not running from the root directory?

lesteve · 2024-06-26T06:50:25Z

In the CI, we do move out of the root directory:

Indeed I forgot about this ...

Does this PR work when pytest is not running from the root directory?

No it does not 😓, when not running from the scikit-learn root folder, the global random seed will always be 42.

SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' pytest --pyargs sklearn -n2 -k test_median_strategy_regressor -v

Part of the output (that shows the global random seed is 42):

tests/test_dummy.py::test_median_strategy_regressor[42]

I am wondering whether it would actually not be a lot simpler to:

remove the SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any' functionality. This is the really tricky case where the controller needs to generate the random seed and pass it to the workers.
to have the CI try different random seeds, draw a random number between 1 and 100 in bash in the CI and set SKLEARN_TESTS_GLOBAL_RANDOM_SEED accordingly

For visibility, the underlying pytest-xdist limitation seems to be pytest-dev/pytest-xdist#917.

lesteve · 2024-06-26T10:49:57Z

In my last commit I implemented what I mentioned in my previous comment. I think by removing the 'any' case, this simplifies the hardest case.

I am wondering whether it would actually not be a lot simpler to:

* remove the `SKLEARN_TESTS_GLOBAL_RANDOM_SEED='any'` functionality. This is the really tricky case where the controller needs to generate the random seed and pass it to the workers.

* to have the CI try different random seeds, draw a random number between 1 and 100 in bash in the CI and set `SKLEARN_TESTS_GLOBAL_RANDOM_SEED` accordingly

jeremiedbb · 2024-06-26T12:08:55Z

build_tools/azure/test_script.sh

@@ -11,7 +11,7 @@ if [[ "$BUILD_REASON" == "Schedule" ]]; then
    # Enable global random seed randomization to discover seed-sensitive tests
    # only on nightly builds.
    # https://scikit-learn.org/stable/computing/parallelism.html#environment-variables
-    export SKLEARN_TESTS_GLOBAL_RANDOM_SEED="any"
+    export SKLEARN_TESTS_GLOBAL_RANDOM_SEED=$(($RANDOM % 100))


I like this alternative, it's a lot more intuitive.

@lesteve Can we do a quick "echo" here to show SKLEARN_TESTS_GLOBAL_RANDOM_SEED in the CI output?

We can even move the message of the header here

jeremiedbb · 2024-06-26T12:14:13Z

sklearn/conftest.py

+def pytest_report_header(config):
+    random_seed_var = environ.get("SKLEARN_TESTS_GLOBAL_RANDOM_SEED")
+    random_seeds = random_seed_var
+
+    return [
+        "To reproduce this test run, set the following environment variable:",
+        f'    SKLEARN_TESTS_GLOBAL_RANDOM_SEED="{random_seeds}"',
+        (
+            "See: https://scikit-learn.org/dev/computing/parallelism.html"
+            "#sklearn-tests-global-random-seed"
+        ),
+    ]
+


With this PR, the header doesn't show up in the CI runs anymore. I think it has limited value since we always see the seed that was used in the pytest report of failed tests because it's in the parametrisation of the test. Also I don't think that any contributor has ever needed and even less seen this.

So I think it's fine to just remove this header. We can document the pytest command to run for a given seed in parallelism.rst.

jeremiedbb

LGTM. I would even remove the header report because I don't it useful anymore since it now doesn't show up in the CI anymore, but I'm fine with or without it.

lesteve · 2024-07-01T14:07:42Z

Great to see this one merged!

It took some time to get there, but as often through feed-back and discussions, we reached a more robust solution in the end!

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

ENH Simplify pytest global random test plugin

49ea247

lesteve added the No Changelog Needed label Dec 14, 2023

Improve comments+doc

167cacc

lesteve added 2 commits December 15, 2023 11:51

Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…

ad3cd50

…nto simplify-pytest-plugin

tweak

440cc3a

lesteve added the Waiting for Reviewer label Jan 2, 2024

lesteve added 2 commits January 3, 2024 07:07

Always add header even when global_random_seed is not any

8417116

Add comment

9e2dc13

ogrisel reviewed Jan 10, 2024

View reviewed changes

lesteve and others added 7 commits March 19, 2024 10:15

Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…

07c4358

…nto simplify-pytest-plugin

debug

19b7ed6

tweak comment

414e8ff

remove debug

8df26ef

Update sklearn/conftest.py

9575778

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Add comment pointing to Pytest issue

e3a3658

Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…

c8fc1bf

…nto simplify-pytest-plugin

thomasjpfan reviewed Jun 22, 2024

View reviewed changes

Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…

573223a

…nto simplify-pytest-plugin

Draw the random seed in the CI and remove "any" case

fed2473

Remove unused function

9b624bc

jeremiedbb reviewed Jun 26, 2024

View reviewed changes

lesteve added 2 commits June 26, 2024 14:25

Simplify further

f056a81

Simplify more

2937330

jeremiedbb approved these changes Jun 27, 2024

View reviewed changes

lesteve and others added 3 commits June 28, 2024 07:30

Replace pytest_report_header by echo in CI test script

1b778d2

tweak comment

f7fe34f

Merge branch 'main' into simplify-pytest-plugin

1801de0

thomasjpfan approved these changes Jun 28, 2024

View reviewed changes

thomasjpfan merged commit a4ebe19 into scikit-learn:main Jun 28, 2024
30 checks passed

lesteve deleted the simplify-pytest-plugin branch July 1, 2024 10:00

jeremiedbb mentioned this pull request Jul 2, 2024

Release 1.5.1 #29382

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Simplify pytest global random test plugin #27963

ENH Simplify pytest global random test plugin #27963

lesteve commented Dec 14, 2023 •

edited

Loading

github-actions bot commented Dec 14, 2023 •

edited

Loading

Charlie-XIAO commented Dec 15, 2023

lesteve commented Jan 3, 2024 •

edited

Loading

ogrisel left a comment

ogrisel Jan 10, 2024

ogrisel Jan 10, 2024

ogrisel Jan 10, 2024

lesteve Jan 10, 2024 •

edited by ogrisel

Loading

lesteve Mar 19, 2024 •

edited

Loading

lesteve Mar 19, 2024 •

edited

Loading

betatim Mar 20, 2024

betatim Mar 20, 2024

lesteve Mar 20, 2024

lesteve Mar 20, 2024

lesteve commented Jun 3, 2024

thomasjpfan left a comment

lesteve commented Jun 23, 2024 •

edited

Loading

lesteve commented Jun 23, 2024 •

edited

Loading

thomasjpfan commented Jun 25, 2024

lesteve commented Jun 26, 2024 •

edited

Loading

lesteve commented Jun 26, 2024

jeremiedbb Jun 26, 2024

thomasjpfan Jun 27, 2024

jeremiedbb Jun 27, 2024

lesteve Jun 28, 2024

jeremiedbb Jun 26, 2024

jeremiedbb left a comment

lesteve commented Jul 1, 2024

ENH Simplify pytest global random test plugin #27963

ENH Simplify pytest global random test plugin #27963

Conversation

lesteve commented Dec 14, 2023 • edited Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

github-actions bot commented Dec 14, 2023 • edited Loading

✔️ Linting Passed

Charlie-XIAO commented Dec 15, 2023

lesteve commented Jan 3, 2024 • edited Loading

ogrisel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve Jan 10, 2024 • edited by ogrisel Loading

Choose a reason for hiding this comment

lesteve Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

lesteve Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve commented Jun 3, 2024

thomasjpfan left a comment

Choose a reason for hiding this comment

lesteve commented Jun 23, 2024 • edited Loading

lesteve commented Jun 23, 2024 • edited Loading

thomasjpfan commented Jun 25, 2024

lesteve commented Jun 26, 2024 • edited Loading

lesteve commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremiedbb left a comment

Choose a reason for hiding this comment

lesteve commented Jul 1, 2024

lesteve commented Dec 14, 2023 •

edited

Loading

github-actions bot commented Dec 14, 2023 •

edited

Loading

lesteve commented Jan 3, 2024 •

edited

Loading

lesteve Jan 10, 2024 •

edited by ogrisel

Loading

lesteve Mar 19, 2024 •

edited

Loading

lesteve Mar 19, 2024 •

edited

Loading

lesteve commented Jun 23, 2024 •

edited

Loading

lesteve commented Jun 23, 2024 •

edited

Loading

lesteve commented Jun 26, 2024 •

edited

Loading