Parallel execution of scikit-learn integration tests broken due to non-deterministic test collection

[csadorf]

With the merge of #6866, we introduced a patch to sklearn's all_estimators to prioritize proxy estimators.

Unfortunately, this change also introduced a source of non-determinism in the test collection order, which means that executing scikit-learn integration tests in parallel is broken.

While we don't run tests in parallel in CI, developers cannot reliably run tests in parallel for faster feedback

Example:

./python/cuml/cuml_accel_tests/upstream/scikit-learn/run-tests.sh -n 4 -v

This will likely fail with an eror like Different tests were collected between gw0 and gw2 error due to non-deterministic test ordering.

Expected Behavior

Scikit-learn integration tests should execute deterministically in parallel without ordering issues.

Current Behavior

The order in which tests are collected is currently non-deterministic, which leads to failures when attempting to run the test suite in parallel. In particular, the sequence of parameterized test instances for a given estimator can change from one run to another. For example, tests involving LinearSVC(max_iter=20) and LinearSVC() may be collected in different orders depending on the run. The underlying cause of this issue is not fully understood, but it appears to be connected to the way the all_estimators function is patched—possibly due to the patch not being applied consistently or completely.

Expected Error Output

When running the command above, you'll see an error like:

ERROR collecting gw2
Different tests were collected between gw0 and gw2. The difference is:
--- gw0
+++ gw2
@@ -25704,6 +25704,70 @@
 tests/test_common.py::test_estimators[LinearRegression()-check_fit1d]
 tests/test_common.py::test_estimators[LinearRegression()-check_fit2d_predict1d]
 tests/test_common.py::test_estimators[LinearRegression()-check_requires_y_none]
+tests/test_common.py::test_estimators[LinearSVC()-check_estimator_cloneable0]
+tests/test_common.py::test_estimators[LinearSVC()-check_estimator_cloneable1]
+...
+tests/test_common.py::test_estimators[LinearSVC()-check_requires_y_none]
 tests/test_common.py::test_estimators[LinearSVC(max_iter=20)-check_estimator_cloneable0]
 tests/test_common.py::test_estimators[LinearSVC(max_iter=20)-check_estimator_cloneable1]
 ...
 tests/test_common.py::test_estimators[LinearSVC(max_iter=20)-check_requires_y_none]
-tests/test_common.py::test_estimators[LinearSVC()-check_estimator_cloneable0]
-tests/test_common.py::test_estimators[LinearSVC()-check_estimator_cloneable1]
-...
-tests/test_common.py::test_estimators[LinearSVC()-check_requires_y_none]
 tests/test_common.py::test_estimators[LinearSVR()-check_estimator_cloneable0]

The key issue is that LinearSVC() and LinearSVC(max_iter=20) appear in different orders between test workers, causing pytest-xdist to fail with "Different tests were collected" errors.

Proposed Solutions

1. Investigate the patch: Review the changes in PR Support LinearSVC and LinearSVR in cuml.accel #6866 to understand what causes the non-determinism
2. Fix ordering logic: Ensure the proxy estimator prioritization maintains deterministic ordering
3. Improve monkeypatching reliability: The current patch may not be applied reliably, leading to inconsistent behavior
4. Consider alternative approaches: Instead of monkeypatching, consider other ways to handle duplicate estimator discovery

[betatim]

Is there a discussion somewhere of what is happening? I wanted to take a look at how all_estimators() works, so it would be good to read what others have found out before/how they figured out that patching all_estimators is the thing to do

[betatim]

A few notes from investigating.

The problem seems to only effect LinearSVC, but not similar classes like LinearSVR. Figuring out what is different between the two is probably useful.

If you activate the accelerator, but do not patch all_estimators the following code reproduces the problem:

from collections import defaultdict
from sklearn.utils.discovery import all_estimators
from cuml.accel.estimator_proxy import is_proxy

ests = all_estimators()

estimator_groups = defaultdict(list)
for name, cls in ests:
  estimator_groups[name].append(cls)

for name, ests in estimator_groups.items():
    if any(is_proxy(e) for e in ests):
        print(name, ests, [is_proxy(e) for e in ests])

This prints LinearSVC [<class 'sklearn.svm._classes.LinearSVC'>, <class 'sklearn.svm.LinearSVC'>] [False, True]

The class from svm._classes isn't a proxy, which might explain why the set() call in all_estimators did not get rid of this duplication. Unclear to me why this doesn't also happen for LinearSVR.

[viclafargue]

If you comment out this line, the problem disappears. The issue is related to the fact that during its exploration the all_estimators function re-imports the original non-proxy sklearn estimator there. The patched function should solve this issue.

I think that the non-determinism issue when using pytest-xdist is unrelated to the patching of the all_estimator function though, it the addition of LinearSVC made the issue visible. Since the cuml-accel installation is also done in the same pytest_load_initial_conftests function in the conftest.py file, it was probably already non-deterministically installed on the pytest workers before.

[betatim]

Does this mean that we will run into this issue with IsotonicRegression as well (if we proxy it), as it is imported a few lines about LinearSVC? If yes, it would mean that it could happen for any estimator at "any moment" just by there being a change in what is imported where in scikit-learn.

What makes you think this problem has always been there? I've run the tests in parallel with -n 8 "many times" (like 50 or 100 times?) and never seen this happen. Now it happens reliably with -n 8 and not without. It seems -n 2 also works?

My guess is that for some reason we sort the result different, maybe because the accelerator patching happens in a different order/at a different time/isn't quite done when different workers start up?

[viclafargue]

Does this mean that we will run into this issue with IsotonicRegression as well (if we proxy it), as it is imported a few lines about LinearSVC?

It is very possible, yes. But, the patching of the all_estimator function should solve this.

What makes you think this problem has always been there? I've run the tests in parallel with -n 8 "many times" (like 50 or 100 times?) and never seen this happen.

I may be wrong there, but the patching of the function is done along the installation of cuml-accel, so it would make sense that some workers did not install cuml-accel successfully before either, but this may not always have resulted in test collection issues.

[csadorf]

I may be wrong there, but the patching of the function is done along the installation of cuml-accel, so it would make sense that some workers did not install cuml-accel successfully before either, but this may not always have resulted in test collection issues.

In case that you're implying that we don't reliably install cuml.accel, I'd be very doubtful of that since we would have seen way more issues with unexpected xfails/xpasses in parallel runs.

[betatim]

One thing to clarify, I should have made it more clear: when I asked

Does this mean that we will run into this issue with IsotonicRegression as well (if we proxy it), as it is imported a few lines about LinearSVC?

I should have added "if we don't patch all_estimators.

My thinking about the problem of parallel execution:

• something must be different between the different workers
• the problem started to be super reliably reproducible once we patched all_estimators. Which happened together with patching of LinearSVC - so maybe that is the root cause?
• the problem might have already existed but was extremely rare before patching all_estimators

What could be different?

• Python dicts are sorted differently every time you start Python, so maybe something relies on the order of a dict but because each worker has a different random order we get different results?
• there is a race between when a worker starts collecting tests and when the patching finishes - some workers see no patching/some patching/full patching?
• maybe the way pytest plugins are handled in the xdist case has something to do with this? Do all workers run all the hooks that we rely on to install the accelerator?

I don't know anything about how pytest-xdist is implemented (does it use threads? Processes? how do plugin hooks/conftest.py parsing work) but it does make me wonder if #7047 is worth trying out for this?

[betatim]

Another bit of information: When I run the upstream tests with -n 8 one of the outputs shows

 tests/test_common.py::test_check_inplace_ensure_writeable[LedoitWolf()]
 tests/test_common.py::test_check_inplace_ensure_writeable[LinearDiscriminantAnalysis()]
 tests/test_common.py::test_check_inplace_ensure_writeable[LinearRegression()]
+tests/test_common.py::test_check_inplace_ensure_writeable[LinearSVC(max_iter=20)]
 tests/test_common.py::test_check_inplace_ensure_writeable[LinearSVC()]
-tests/test_common.py::test_check_inplace_ensure_writeable[LinearSVC(max_iter=20)]
 tests/test_common.py::test_check_inplace_ensure_writeable[LinearSVR()]
 tests/test_common.py::test_check_inplace_ensure_writeable[LocalOutlierFactor()]
 tests/test_common.py::test_check_inplace_ensure_writeable[LocallyLinearEmbedding(max_iter=5)]

Which suggests that the order of LinearSVC and LinearSVC(max_iter=20) got swapped between two workers. In another output diff there are a lot of additional tests like:

 tests/test_common.py::test_estimators[LinearSVC(max_iter=20)-check_requires_y_none]
+tests/test_common.py::test_estimators[LinearSVC()-check_estimator_cloneable0]
+tests/test_common.py::test_estimators[LinearSVC()-check_estimator_cloneable1]
+tests/test_common.py::test_estimators[LinearSVC()-check_estimator_tags_renamed]
+tests/test_common.py::test_estimators[LinearSVC()-check_valid_tag_types]
+tests/test_common.py::test_estimators[LinearSVC()-check_estimator_repr]
+tests/test_common.py::test_estimators[LinearSVC()-check_no_attributes_set_in_init]
+tests/test_common.py::test_estimators[LinearSVC()-check_fit_score_takes_y]
+tests/test_common.py::test_estimators[LinearSVC()-check_estimators_overwrite_params]
+tests/test_common.py::test_estimators[LinearSVC()-check_dont_overwrite_parameters]
+tests/test_common.py::test_estimators[LinearSVC()-check_estimators_fit_returns_self]

The interesting thing is, if I run pytest -sv test_common.py::test_check_inplace_ensure_writeable -k LinearSVC in a local checkout of scikit-learn I see only one test: test_common.py::test_check_inplace_ensure_writeable[LinearSVC(max_iter=20)] - which matches what I'd expect from reading _construct_instances from sklearn/utils/_test_common/instance_generator.py. Because LinearSVC is mentioned in INIT_PARAMS the only instance that we should see is the one using max_iter=20. There shouldn't be tests with a default constructed LinearSVC.

[csadorf]

There shouldn't be tests with a default constructed LinearSVC

I'm wondering whether there is an identity issue, i.e., there are two different incarnations of LinearSVC, one which matches the hash signature of the INIT_PARAMS key, and one that doesn't.