FIX Avoid dead-lock with OpenBLAS 0.3.26 on Windows inside pairwise distances calculation by lesteve · Pull Request #28692 · scikit-learn/scikit-learn

lesteve · 2024-03-25T11:14:27Z

Fix #28625. More context in scipy/scipy#20294.

It is easy to add a test, but for now we only have a single Windows build using MKL. The main question is do we want to add another Windows build with OpenBLAS?

github-actions · 2024-03-25T11:15:47Z

✔️ Linting Passed

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

_{Generated for commit: 13c6fdb. Link to the linter CI: here}

sklearn/metrics/_pairwise_distances_reduction/_base.pyx.tp

jeremiedbb · 2024-03-25T11:46:21Z

sklearn/metrics/_pairwise_distances_reduction/_base.pyx.tp


-    for idx in prange(n, schedule='static', nogil=True, num_threads=num_threads):
-        squared_row_norms[idx] = _dot(d, X_ptr + idx * d, 1, X_ptr + idx * d, 1)
+    with threadpool_limits(limits=1, user_api='blas'):


I would add a comment here because it's surprising that it's necessary while _dot is already single threaded. Maybe with a link to the issue

_dot is already single threaded.

Actually it looks like it's not. see scipy/scipy#20294 (comment). It's weird cause every time I tested it, I only saw a single thread running, no matter the length of the arrays.

Maybe we should just use nrm2 as suggested here scipy/scipy#20294 (comment), even though it implies take a square root and then square back. From an efficiency point of view, it should not have a visible impact since the computation of the norms is not the most costly part of the pairwise distance reduction compuations.

I did not know this but ddot is not necessarily single-threaded, see scipy/scipy#20294 (comment)

ogrisel · 2024-03-25T13:49:55Z

Instead of calling threadpool_limits inside _sqeuclidean_row_norms64_dense which is itself called by the constructor of EuclideanArgKmin64 which is itself called by the ArgKmin64.compute class method, I would prefer to move the existing call to threadpool_limits in ArgKmin64.compute to include both the call to the EuclideanArgKmin64 and the _parallel_on_*:

scikit-learn/sklearn/metrics/_pairwise_distances_reduction/_argkmin.pyx.tp

Lines 59 to 93 in 64ae371

    
           if metric in ("euclidean", "sqeuclidean"): 
        
               # Specialized implementation of ArgKmin for the Euclidean distance 
        
               # for the dense-dense and sparse-sparse cases. 
        
               # This implementation computes the distances by chunk using 
        
               # a decomposition of the Squared Euclidean distance. 
        
               # This specialisation has an improved arithmetic intensity for both 
        
               # the dense and sparse settings, allowing in most case speed-ups of 
        
               # several orders of magnitude compared to the generic ArgKmin 
        
               # implementation. 
        
               # For more information see MiddleTermComputer. 
        
               use_squared_distances = metric == "sqeuclidean" 
        
               pda = EuclideanArgKmin{{name_suffix}}( 
        
                   X=X, Y=Y, k=k, 
        
                   use_squared_distances=use_squared_distances, 
        
                   chunk_size=chunk_size, 
        
                   strategy=strategy, 
        
                   metric_kwargs=metric_kwargs, 
        
               ) 
        
           else: 
        
               # Fall back on a generic implementation that handles most scipy 
        
               # metrics by computing the distances between 2 vectors at a time. 
        
               pda = ArgKmin{{name_suffix}}( 
        
                   datasets_pair=DatasetsPair{{name_suffix}}.get_for(X, Y, metric, metric_kwargs), 
        
                   k=k, 
        
                   chunk_size=chunk_size, 
        
                   strategy=strategy, 
        
               ) 
        
           # Limit the number of threads in second level of nested parallelism for BLAS 
        
           # to avoid threads over-subscription (in GEMM for instance). 
        
           with threadpool_limits(limits=1, user_api="blas"): 
        
               if pda.execute_in_parallel_on_Y: 
        
                   pda._parallel_on_Y() 
        
               else: 
        
                   pda._parallel_on_X()

More precisely, move the with threadpool_limits(limits=1, user_api="blas") to start at line 59 instead of 89.

This will this will reduce the number of successive calls to openblas_set_num_threads from 4 to 2 when calling ArgKmin64.compute. And this will also make threadpool_limits(limits=1, user_api="blas") always happen at a consistent API level with the pairwise distance reduction codebase.

Note that: in addition to working around the thread-safety problem of recent OpenBLAS on Windows, this change can also protect against oversubscription problems for all platforms.

ogrisel

LGTM!

sklearn/metrics/_pairwise_distances_reduction/_argkmin.pyx.tp

ogrisel · 2024-03-26T10:37:50Z

Not sure if we need a changelog entry or not for this. Probably we should (probably as a fix).

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

lesteve · 2024-03-26T11:04:44Z

I added a short changelog entry. Not sure if it is worth trying to mention all the estimators that can be affected by this.

Also another question: should this be back-ported to 1.4.2?

jeremiedbb · 2024-03-26T11:08:16Z

Not sure if it is worth trying to mention all the estimators that can be affected by this.

If you want to keep it simple, you can just say "neighbor based algorithms" :)

Also another question: should this be back-ported to 1.4.2?

I think so since 1.4 users are impacted

jeremiedbb

I moved the changelog entry to target 1.4.2. LGTM, thanks.

…istances calculation (scikit-learn#28692) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: jeremie du boisberranger <jeremiedbb@yahoo.fr>

FIX Avoid dead-lock on Windows inside pairwise distances calculation

7d4bdbf

github-actions bot added module:metrics cython labels Mar 25, 2024

jeremiedbb reviewed Mar 25, 2024

View reviewed changes

Handle comments

4d4aee8

Limit OpenBLAS number of threads in compute

dd74e10

ogrisel approved these changes Mar 26, 2024

View reviewed changes

sklearn/metrics/_pairwise_distances_reduction/_argkmin.pyx.tp Outdated Show resolved Hide resolved

sklearn/metrics/_pairwise_distances_reduction/_argkmin.pyx.tp Show resolved Hide resolved

lesteve and others added 2 commits March 26, 2024 11:56

Apply suggestions from code review

838feb0

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

Add changelog entry

e1f5c6e

jeremiedbb added this to the 1.4.2 milestone Mar 26, 2024

jeremiedbb added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Mar 26, 2024

lesteve and others added 3 commits March 26, 2024 13:46

Tweak changelog

b3b11b3

Tweak changelog

53d3ef0

target 1.4.2

13c6fdb

jeremiedbb approved these changes Mar 26, 2024

View reviewed changes

jeremiedbb enabled auto-merge (squash) March 26, 2024 13:19

jeremiedbb merged commit 1bbb228 into scikit-learn:main Mar 26, 2024

lesteve deleted the avoid-openblas-windows-hang branch March 26, 2024 14:25

lesteve mentioned this pull request Mar 26, 2024

BUG: Hang on Windows in scikit-learn with 1.13rc1 and 1.14.dev (maybe due to OpenBLAS 0.3.26 update?) scipy/scipy#20294

Closed

larsoner mentioned this pull request Mar 27, 2024

MAINT: Restore OMP_NUM_THREADS=2 on Windows mne-tools/mne-python#12520

Merged

jeremiedbb mentioned this pull request Apr 8, 2024

Release 1.4.2 #28774

Merged

15 tasks

joshuacwnewton mentioned this pull request Oct 4, 2024

dipy==1.9.0 is causing our macOS GitHub Actions runners to stall on unrelated torch tests dipy/dipy#3125

Open

6 tasks

Uh oh!

Conversation

lesteve commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

Uh oh!

jeremiedbb Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

jeremiedbb Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

jeremiedbb Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

lesteve Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ogrisel commented Mar 26, 2024

Uh oh!

lesteve commented Mar 26, 2024

Uh oh!

jeremiedbb commented Mar 26, 2024

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lesteve commented Mar 25, 2024 •

edited

Loading

github-actions bot commented Mar 25, 2024 •

edited

Loading

ogrisel commented Mar 25, 2024 •

edited

Loading