FEA Controller for scipy_openblas #175
Conversation
|
Should I change |
I guess so since they do not share common symbols. |
|
The new pip-dev CI job confirms that it works: |
|
About #175 (comment), I am not so sure. Maybe @rgommers and @mattip have a different opinion. I am not sure about the scope of scipy-openblas: numpy/numpy#26240 (comment) |
|
I commented on the scope in numpy/numpy#26240 (comment) As for whether this should be a separate controller: it feels a bit wrong conceptually. It is a fairly regular build of OpenBLAS with a symbol prefix set. That's a build-time flag; there are many such build-time flags, and there may be other distributions that do something similar. Actually I already see a next change coming: the ILP64 symbol suffix for OpenBLAS is going to become Probably the more correct way to model this is to consider the symbol naming scheme, which is: where:
That way this is extensible and wouldn't require separate controllers for different builds of the same library. |
|
I changed to have a same controller for all versions of OpenBLAS. I try do determine first what are the specific prefix, suffix and compiler_suffix and then use them for all symbol lookups. However I face an issue with the above description of the combinatorics of these affixes. Running but the I can blacklist some combinations but we should take care in future openblas builds to not use these combinations as valid symbol name because it will make things hard for threadpoolctl to support all openblas versions :) For instance, |
|
Actually it's the same with the 32bit int version of openblas: and |
|
Ok so looking at OpenBLAS source made it clear, who would have guessed 😄 |
ogrisel
left a comment
ogrisel
left a comment
There was a problem hiding this comment.
A couple of nitpicks but otherwise, LGTM. Thanks for the fix!
threadpoolctl.py
Outdated
| _sym_prefixes = ("", "scipy_") | ||
| _sym_suffixes = ("", "64_", "_64") |
There was a problem hiding this comment.
I would rename to _symbol_prefixes / _symbol_suffixes to make the meaning of those variables more straightforward to understand.
There was a problem hiding this comment.
I'm pretty sure I did that to fit within 88 chars 😄
I renamed them
threadpoolctl.py
Outdated
| self.prefix = prefix | ||
| self.filepath = filepath | ||
| self.dynlib = ctypes.CDLL(filepath, mode=_RTLD_NOLOAD) | ||
| self._sym_prefix, self._sym_suffix = self._find_affixes() |
There was a problem hiding this comment.
Similar comment for the singular versions of those attributes.
threadpoolctl.py
Outdated
| if (symbol := self._get_symbol("openblas_get_corename")) is not None: | ||
| symbol.restype = ctypes.c_char_p | ||
| return symbol().decode("utf-8") |
There was a problem hiding this comment.
I preferred naming the Python variables holding the functions objects by their symbol name as before:
| if (symbol := self._get_symbol("openblas_get_corename")) is not None: | |
| symbol.restype = ctypes.c_char_p | |
| return symbol().decode("utf-8") | |
| if (get_corename := self._get_symbol("openblas_get_corename")) is not None: | |
| get_corename.restype = ctypes.c_char_p | |
| return get_corename().decode("utf-8") |
but this is a minor nitpick. Feel free to ignore. But if you accept, don't forget to rename all other occurrences of the symbol local variable in other methods.
3.5.0 (2024-04-29) - Added support for the Scientific Python version of OpenBLAS (https://github.com/MacPython/openblas-libs), which exposes symbols with different names than the ones of the original OpenBLAS library. joblib/threadpoolctl#175
3 participants
Numpy, Scipy, ... will likely be linked against a common build of OpenBLAS for the sci-py ecosystem (https://github.com/MacPython/openblas-libs). At some point it might even become a runtime dependency for these packages.
This special build of OpenBLAS comes with a different naming pattern than OpenBLAS, the lib name being
libscipy_openblas..., andthe symbols being prefixed byscipy_. I made a new controller inherited from the OpenBLAS controller.I also added a build in the CI to test against the dev versions of numpy and scipy. It would probably have allowed us to face the issue earlier.