-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
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
MAINT remove side-effects in test_partial_dependence #30039
Conversation
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.
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?
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. |
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 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. |
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 |
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.
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.
I think this is a good fix regardless of @lesteve 's point, and those fixes can happen later? |
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.