Skip to content

MAINT remove side-effects in test_partial_dependence #30039

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

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Oct 9, 2024

Towards #30007.

Note that this is similar to some work I started a while ago (for a different reason) but forgot to complete in this draft PR: #29584. However this is not about the same test module.

Copy link

github-actions bot commented Oct 9, 2024

✔️ Linting Passed

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

Generated for commit: a5f3149. Link to the linter CI: here

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

The pattern for which we pass different estimators via a pytest parametrization and then fit it in the body of the test is quite common.

What is the reason why this is an issue for the partial dependence but not in other places?

@ogrisel
Copy link
Member Author

ogrisel commented Oct 10, 2024

What is the reason why this is an issue for the partial dependence but not in other places?

It is also a problem in other places, either when randomizing the order of the tests (as discovered in #29584) or when using parallel threads, as investigated in #30007.

To fully solve #30007 we would need to fix all occurrences of such side effects caused by mutable pytest parametrizations but also, avoid running some tests that are fundamentally thread-unsafe (e.g. tests that check warnings).

This will be an incremental maintenance effort, and I decided to open a small PR to resolve a single problem at a time in a single test module.

@ogrisel ogrisel added Build / CI module:test-suite everything related to our tests and removed Build / CI labels Oct 10, 2024
@lesteve
Copy link
Member

lesteve commented Oct 10, 2024

I don't feel super good about this ... this is a typical pytest pattern and I feel like in an ideal world we should not have to change our tests too much to stress-test free-threaded (probably easier said than done ...). I guess adding .clone is not a big change but we are going to add it in many many places just for free-threaded reasons.

Note that this is slightly different than #29584 which happens because the same list of mutable objects is reused to parametrize different tests. That means side-effects in one test can impact the next test that run and that the ordering of tests may matter. This issue is specific to pytest-run-parallel (or similar plugin like pytest-freethreading) which runs the same test function in multiple threads with the goal to find issues when using free-threaded.

@ogrisel
Copy link
Member Author

ogrisel commented Oct 10, 2024

we are going to add it in many many places just for free-threaded reasons

Maintaining a side effect free test suite is helpful to ease debugging in general, even if I understand that there is a difference with what is being fixed in #29584.

The alternative would be to require the .fit method of scikit-learn estimators to be thread-safe, but this would be a much more complex endeavor.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

This seems quite fair to me, and exactly what we do in common tests anyway. When we pass an instance, we always clone it first.

@adrinjalali
Copy link
Member

I think this is a good fix regardless of @lesteve 's point, and those fixes can happen later?

@adrinjalali adrinjalali merged commit 1d6cfde into scikit-learn:main Oct 14, 2024
46 of 47 checks passed
@ogrisel ogrisel deleted the maint-test_partial_dependence branch October 16, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:test-suite everything related to our tests No Changelog Needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants