TST Skip test when unstable openblas configuration #21702

jjerphan · 2021-11-18T09:59:44Z

Reference Issues/PRs

Temporarily fixes #21361.

What does this implement/fix? Explain your changes.

This is a temporary fix for some tests on main.

This fixes some CI jobs on main.

This is a first fix toward preventing contributors spending their days exploring logs
of issues they don't understand the problems being reported because they aren't
related to their PRs.

As informally proposed by @ogrisel, a long term fix would be to pin the maximum
version dependencies and have a process to update each dependency version
when possible.

Any other comments?

This fix can be made more specific on some values of the parametrisation.

sklearn/utils/__init__.py

jjerphan · 2021-11-18T10:48:13Z

Force-pushing to include @adrinjalali in 544d564 as a co-author.

ogrisel · 2021-11-18T10:52:36Z

sklearn/utils/_testing.py

@@ -452,6 +457,10 @@ def set_random_state(estimator, random_state=0):
        not joblib.parallel.mp, reason="joblib is in serial mode"
    )

+    fails_if_unstable_openblas = pytest.mark.xfail(


not sure if xfail is a good idea for code that can segfault. Maybe skip is a better idea.

This was marked resolved without being addressed or commented on: I am pretty sure this is a bad idea to execute code that can segfault, especially when pytest-xdist is not used. XFAIL will run the code, SKIP will not.

My mistake multitasking and forgetting to take your suggestion in the patch of this commit.

adrinjalali

A green CI is a good one.

.circleci/config.yml

sklearn/calibration.py

Co-authored-By: Adrin Jalali <adrin.jalali@gmail.com>

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

jjerphan · 2021-11-18T13:26:00Z

I force-pushed to:

remove duplicated commits (hence a correct diff now)
integrate the forgotten suggestion from @ogrisel

ogrisel

The prescott kernel will cause a segfault on any OpenBLAS version when called on non-memory aligned data, whatever the version of OpenBLAS. What changes in old version of OpenBLAS is that the prescott kernel is detected instead of Haswell on SkylakeX machines indirectly causing the bug.

But the bug could still happen if the test is actually run on a prescott machine with a recent version of OpenBLAS.

Instead of introducing a utility function in sklearn.utils with a generic name such as _in_unstable_openblas_configuration, I would rather change the function sklearn.utils.estimators_checks.check_classifiers_train

to call create_memmap_backed_data with aligned=has_prescott_openblas once #21654 is merged, has_prescott_openblas is defined as:

    # OpenBLAS is known to segfault with unaligned data on the prescott architecture
    has_prescott_openblas = any(
        True
        for info in threadpool_info()
        if info["internal_api"] == "openblas"
        and info.get("architecture").lower() == "prescott"
    )

Also for the sake of keeping the git history easier to understand, would it be possible to separate the fix for the calibration plots labels separate from the fix for openblas segfaults? Those are unrelated problems, right? Since we squash merge PRs, I would rather have those 2 fixes in 2 separate commits in the git history, hence 2 separate PRs.

sklearn/linear_model/tests/test_omp.py

ogrisel · 2021-11-18T13:37:17Z

remove duplicated commits (hence a correct diff now)

I wrote the previous review against before this fix, please ignore that part of the review.

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

ogrisel

Assuming CI is green, +1 for merge with the last comment below:

sklearn/utils/estimator_checks.py

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

ogrisel · 2021-11-18T17:28:18Z

Green! @adrinjalali still +1 for merge with the new way to fix this?

adrinjalali

A much cleaner solution indeed

ogrisel · 2021-11-18T18:28:59Z

Actually I just found out that it does not work :)

$ OPENBLAS_CORETYPE=Prescott pytest -v sklearn/tests/test_common.py -k "check_classifiers_train and svc"
[...]
    def create_memmap_backed_data(data, mmap_mode="r", return_folder=False, aligned=False):
        """
        Parameters
        ----------
        data
        mmap_mode : str, default='r'
        return_folder :  bool, default=False
        aligned : bool, default=False
            If True, if input is a single numpy array and if the input array is aligned,
            the memory mapped array will also be aligned. This is a workaround for
            https://github.com/joblib/joblib/issues/563.
        """
        temp_folder = tempfile.mkdtemp(prefix="sklearn_testing_")
        atexit.register(functools.partial(_delete_folder, temp_folder, warn=True))
        if aligned:
            if isinstance(data, np.ndarray) and data.flags.aligned:
                # https://numpy.org/doc/stable/reference/generated/numpy.memmap.html
                filename = op.join(temp_folder, "data.dat")
                fp = np.memmap(filename, dtype=data.dtype, mode="w+", shape=data.shape)
                fp[:] = data[:]  # write data to memmap array
                fp.flush()
                memmap_backed_data = np.memmap(
                    filename, dtype=data.dtype, mode=mmap_mode, shape=data.shape
                )
            else:
>               raise ValueError("If aligned=True, input must be a single numpy array.")
E               ValueError: If aligned=True, input must be a single numpy array.

ogrisel · 2021-11-18T18:29:32Z

I will open a PR with a quick fix for the above.

…rn#21702) * TST XFAIL test when unstable openblas configuration * Adapt versions parsing Co-authored-By: Adrin Jalali <adrin.jalali@gmail.com> * DOC Prefer ifskip and reference scikit-learn#21361 Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> * Fix Julien's scheduler incorrect dispatch * Simplify Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> * Do not remove blank line * fixup! Do not remove blank line * Retrigger CI * Prudently assume Prescott might be the architecture if it is unknown Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

* TST XFAIL test when unstable openblas configuration * Adapt versions parsing Co-authored-By: Adrin Jalali <adrin.jalali@gmail.com> * DOC Prefer ifskip and reference #21361 Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> * Fix Julien's scheduler incorrect dispatch * Simplify Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> * Do not remove blank line * fixup! Do not remove blank line * Retrigger CI * Prudently assume Prescott might be the architecture if it is unknown Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

TST XFAIL test when unstable openblas configuration

a472a79

github-actions bot added module:linear_model module:utils labels Nov 18, 2021

jjerphan added the No Changelog Needed label Nov 18, 2021

adrinjalali reviewed Nov 18, 2021

View reviewed changes

sklearn/utils/__init__.py Outdated Show resolved Hide resolved

sklearn/utils/__init__.py Outdated Show resolved Hide resolved

jjerphan force-pushed the xfail-prescott branch from 544d564 to b4c5650 Compare November 18, 2021 10:48

ogrisel reviewed Nov 18, 2021

View reviewed changes

jjerphan marked this pull request as ready for review November 18, 2021 10:59

adrinjalali approved these changes Nov 18, 2021

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

sklearn/calibration.py Outdated Show resolved Hide resolved

Adapt versions parsing

9c46d26

Co-authored-By: Adrin Jalali <adrin.jalali@gmail.com>

jjerphan changed the title ~~TST XFAIL test when unstable openblas configuration~~ TST Skip test when unstable openblas configuration Nov 18, 2021

DOC Prefer ifskip and reference scikit-learn#21361

5ca918a

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

jjerphan force-pushed the xfail-prescott branch from 4c8374e to 5ca918a Compare November 18, 2021 13:24

ogrisel requested changes Nov 18, 2021

View reviewed changes

sklearn/linear_model/tests/test_omp.py Outdated Show resolved Hide resolved

Fix Julien's scheduler incorrect dispatch

8ea56bb

ogrisel mentioned this pull request Nov 18, 2021

[MRG] FIX segmentation fault on memory mapped contiguous memoryview #21654

Merged

jjerphan and others added 5 commits November 18, 2021 15:41

Merge branch 'main' into xfail-prescott

235b0fe

Simplify

e2c27f3

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

Do not remove blank line

21bb5cf

fixup! Do not remove blank line

a51a83f

Retrigger CI

5b422d1

ogrisel approved these changes Nov 18, 2021

View reviewed changes

sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved

Prudently assume Prescott might be the architecture if it is unknown

97397b7

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

adrinjalali reviewed Nov 18, 2021

View reviewed changes

adrinjalali merged commit 3f019bd into scikit-learn:main Nov 18, 2021

ogrisel mentioned this pull request Nov 18, 2021

Follow-up fix for check_classifiers_train on prescott OpenBLAS #21704

Merged

ogrisel mentioned this pull request Nov 24, 2021

DOC convert to notebook style SVM C scaling example #21776

Merged

jjerphan deleted the xfail-prescott branch October 21, 2022 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST Skip test when unstable openblas configuration #21702

TST Skip test when unstable openblas configuration #21702

jjerphan commented Nov 18, 2021 •

edited

Loading

jjerphan commented Nov 18, 2021

ogrisel Nov 18, 2021

ogrisel Nov 18, 2021 •

edited

Loading

jjerphan Nov 18, 2021

adrinjalali left a comment

jjerphan commented Nov 18, 2021

ogrisel left a comment •

edited

Loading

ogrisel commented Nov 18, 2021

ogrisel left a comment

ogrisel commented Nov 18, 2021

adrinjalali left a comment

ogrisel commented Nov 18, 2021 •

edited

Loading

ogrisel commented Nov 18, 2021

TST Skip test when unstable openblas configuration #21702

TST Skip test when unstable openblas configuration #21702

Conversation

jjerphan commented Nov 18, 2021 • edited Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

jjerphan commented Nov 18, 2021

ogrisel Nov 18, 2021

Choose a reason for hiding this comment

ogrisel Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

jjerphan Nov 18, 2021

Choose a reason for hiding this comment

adrinjalali left a comment

Choose a reason for hiding this comment

jjerphan commented Nov 18, 2021

ogrisel left a comment • edited Loading

Choose a reason for hiding this comment

ogrisel commented Nov 18, 2021

ogrisel left a comment

Choose a reason for hiding this comment

ogrisel commented Nov 18, 2021

adrinjalali left a comment

Choose a reason for hiding this comment

ogrisel commented Nov 18, 2021 • edited Loading

ogrisel commented Nov 18, 2021

jjerphan commented Nov 18, 2021 •

edited

Loading

ogrisel Nov 18, 2021 •

edited

Loading

ogrisel left a comment •

edited

Loading

ogrisel commented Nov 18, 2021 •

edited

Loading