Added note to BLAS.[g|s]et_num_threads about Apple Accelerate not supporting it

[ajinkya-k]

Per the documentation for AppleAccelerate.jl, BLAS.get_num_threads() returns a hard coded default when using AppleAccelerate. This PR simply adds a warning message when this function is called to alert the user in the console itself.

[ajinkya-k]

Adding @staticfloat and @danielwe for input

[danielwe]

Linking JuliaLinearAlgebra/AppleAccelerate.jl#75 for the discussion on how to improve support for accelerate's threading API in Julia/LBT

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.89%. Comparing base (e7da19f) to head (adef1ed).
Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1195   +/-   ##
=======================================
  Coverage   91.89%   91.89%           
=======================================
  Files          34       34           
  Lines       15360    15360           
=======================================
  Hits        14115    14115           
  Misses       1245     1245           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ajinkya-k]

How can I test my change which uses AppleAccelerate without adding that as a dependency to the project itself?

[mcabbott]

One pattern is to save & reset the number of BLAS threads:

n = get_num_threads()
set_num_threads(1)
Threads.@threads for ...
    # do some work, calling BLAS
end
set_num_threads(n)  # restore original setting

This isn't ideal with Accelerate, but making packages which do this suddenly print warnings seems sub-optimal.

[ajinkya-k]

@mcabbott Apple Accelerate does not support set_num_threads() either. So this wouldn't work either. This PR won't affect other platforms.

[danielwe]

I think the issue is that someone may write packages or other cross-platform code with the pattern @mcabbott showed. This currently works just fine with Accelerate---it doesn't have any effect, but it doesn't cause anything bad to happen either. With this PR, every user of such code also using Accelerate will start seeing warnings they can't easily control or silence. (Imagine if this pattern is used in an objective function passed to an optimizer---before you know it, hundreds of warnings have flooded your terminal.)

A compromise could be to add maxlog=1, such that the warning will only be shown once per session.

[staticfloat]

I agree that maxlog=1 is important

[danielwe]

Another thing to consider is whether libblastrampoline will see any updates to this behavior in the foreseeable future. Users will probably only get this PR once it's made it into the sysimage of a Julia release, so whatever change is made here should reflect the expected behavior at that point.

[ajinkya-k]

I see. Sorry I misunderstood. I added maxlog=1

[ajinkya-k]

Another thing to consider is whether libblastrampoline will see any updates to this behavior in the foreseeable future. Users will probably only get this PR once it's made it into the sysimage of a Julia release, so whatever change is made here should reflect the expected behavior at that point.

Should I add "As of LinearAlgebra v'x.y.z' Apple Accelerate does not ......"

[ajinkya-k]

I do think that this warning should be thrown from inside libblastrampoline or AppleAccelerate.jl but I am not knowledgeable enough to edit either of those codebases, so I pushed a PR changing lbt.jl here. Also I am not aware of any other package that implements a version of get_num_threads so I thought its fine to do it in lbt.jl

[mcabbott]

maxlog=1 is better.

But I still don't really see why this needs to print a warning, in code that is probably working fine.

Can you explain what's wrong with BLAS.get_num_threads() == 1? Are you concerned this is confusing -- then maybe it can be explained in the docstring? That's also visible to the user at the prompt.

[danielwe]

@ajinkya-k what would you think of, instead of this PR, putting something like the following in the get_num_threads docstring:

!!! note
    Some BLAS libraries, such as Apple Accelerate, cannot be configured to use a fixed number of threads. For these backends, `get_num_threads()` always returns `1`.

While we're at it the docstring should probably also have an xref See also [`set_num_threads`](@ref), and the set_num_threads docstring should have a similar note that also mentions the VECLIB_MAXIMUM_THREADS environment variable.

[ajinkya-k]

Changing the docstrong sounds like a good option. I personally would prefer a warning so that the user knows this is not going to work upfront, so how about adding a warning to AppleAccelerate.jl in the initial function itself. So that the warning is triggered when (and if) AppleAccelerate is loaded"

We can also add the note to the docstring in this repo to let the user know

[staticfloat]

My concern with making this a warning is that most users will not manually import AppleAccelerate, they’ll import some package that imports it instead. Same goes for setting threads. So I also prefer doc string over warning.

[ajinkya-k]

That’s fair. I’ll make a push with the docstring to this PR by tomorrow

[ajinkya-k]

@staticfloat @mcabbott I modified the PR to only add a note in the docstring. Let me know what you think

[ajinkya-k]

@mcabbott @staticfloat Is the docstring note good? Or should i make any more changes?