WIP: Azure Windows Openblas experiment #11965
Conversation
|
@WarrenWeckesser do changes made to your fork trigger CI runs? If they do, then it may be possible to close this PR, and just rely on CI runs based on your fork. Once your fork works you could reopen the PR. This would save workload on the CI suppliers. |
|
@andyfaff, I'm not sure, I can try. But I'm done experimenting for the next day or so. If anyone can continue working on this in the mean time, please do. And don't take any of the changes that I made so far in this PR as the correct way to solve the problem. I've just been trying some changes based on what Matti is doing in the numpy PR. If pushing to my fork without a PR doesn't trigger the test suite, what I can do when I get back to this is temporarily comment out all the other CI tests except the Azure Windows runs. |
|
The last change made some progress; now three of the four Azure Windows test suites pass. The ILP64 version still fails. |
| } | ||
| Write-Host "::: $env:OPENBLAS" | ||
| Write-Host "::: $env:OPENBLAS64_" | ||
|
|
There was a problem hiding this comment.
This might help to figure out what is going on, adapted from what I used in numpy on linux (after downloading the openblas tarball and expanding it in /tmp/local/):
OPENBLAS64_=/tmp/local python -c \
"import builtins; builtins.__SCIPY_SETUP__=1; \
from scipy._build_utils.system_info \
import get_info; print(get_info('lapack_opt', 2))"
I am pretty sure lapack_opt is the right thing to use, I also see blas_opt but I do not see anywhere the 64-bit variants like in numpy/core/setup.py line 752
Edit: updated and clarified
There was a problem hiding this comment.
If the environment variables are a suitable replacement for the site.cfg approach below, then presumably both are needed for 64-bit int case while only the one for 32-bit int case. That might be a more concise modification.
|
Closing temporarily. |
…nv. variable OPENBLAS_ILP64
|
It looks like a better approach is to fix the underlying problem in NumPy's distutils, so closing. |
|
Hmm not yet sure if it's better - if you had already fixed 3 out of 4 failures, maybe we should merge this either way? |
|
Maybe? With some of these changes, I'm in the dark. For example, I have no idea why, when I also don't know what is going on with that fourth failing test. It might be related to Tyler's comment over in #11990. |
|
You need both 32-bit and 64-bit libs to build the ilp64 version, so the build should fail without (as it seems it does). |
|
Hey Warren, this diff deals with the final remaining failure, thanks to some help from Pauli (SciPy building ILP64 and tests looking good so far -- see fork PR: tylerjereddy#31): May not need the whole diff, and obviously needs a bit of cleaning up/conditionals maybe when the matrix is restored. diff --git a/azure-pipelines.yml b/azure-pipelines.yml
index 2ad4ef48b..072fd06e8 100644
--- a/azure-pipelines.yml
+++ b/azure-pipelines.yml
- $target = "C:\\hostedtoolcache\\windows\\Python\\$pyversion\\$(PYTHON_ARCH)\\lib\\$target_name"
+ #$target = "C:\\hostedtoolcache\\windows\\Python\\$pyversion\\$(PYTHON_ARCH)\\lib\\$target_name"
+ $target = "$pwd\openblaslib\$target_name"
Write-Host "target path: $target"
$old_value = $env:NPY_USE_BLAS_ILP64
$env:NPY_USE_BLAS_ILP64 = $ilp64
@@ -109,11 +59,29 @@ jobs:
$env:NPY_USE_BLAS_ILP64 = $old_value
cp $openblas $target
}
+ mkdir openblaslib
Download-OpenBLAS('0')
If ($env:NPY_USE_BLAS_ILP64 -eq '1') {
Download-OpenBLAS('1')
}
displayName: 'Download / Install OpenBLAS'
+ - bash: |
+ echo "[openblas]" >> site.cfg
+ echo "libraries = openblas" >> site.cfg
+ echo "library_dirs = d:\a\1\s\openblaslib" >> site.cfg
+ echo "include_dirs = d:\a\1\s\openblaslib" >> site.cfg
+ echo "runtime_library_dirs = d:\a\1\s\openblaslib" >> site.cfg
+
+ echo " " >> site.cfg
+
+ echo "[openblas64_]" >> site.cfg
+ echo "libraries = openblas64_" >> site.cfg
+ echo "library_dirs = d:\a\1\s\openblaslib" >> site.cfg
+ echo "include_dirs = d:\a\1\s\openblaslib" >> site.cfg
+ echo "runtime_library_dirs = d:\a\1\s\openblaslib" >> site.cfg
+ cat site.cfg
+ displayName: 'Set site.cfg for ilp64 case'
+ condition: eq(variables['NPY_USE_BLAS_ILP64'], 1)
- powershell: |
# wheels appear to use mingw64 version 6.3.0, but 6.4.0
# is the closest match available from choco package manager
@@ -121,6 +89,11 @@ jobs:
displayName: 'Install 32-bit mingw for 32-bit builds'
condition: and(succeeded(), eq(variables['BITS'], 32))
- powershell: |
+ # perform choco installs on same root drive as
+ # cwd/SciPy clone
+ Move-Item -Path "c:\\programdata\\chocolatey" -Destination "$pwd\\programdata\\chocolatey" -force
+ $env:ChocolateyInstall = "$pwd\\programdata\\chocolatey"
+ $env:PATH = "$pwd\\programdata\\chocolatey\\bin;" + $env:PATH
choco install -y mingw --force --version=6.4.0
displayName: 'Install 64-bit mingw for 64-bit builds'
condition: and(succeeded(), eq(variables['BITS'], 64))
@@ -156,7 +129,7 @@ jobs:
$env:LDFLAGS = "-m32"
refreshenv
}
- $env:PATH = "C:\\ProgramData\\chocolatey\\lib\\mingw\\tools\\install\\mingw$(BITS)\\bin;" + $env:PATH
+ $env:PATH = "$pwd\\ProgramData\\chocolatey\\lib\\mingw\\tools\\install\\mingw$(BITS)\\bin;" + $env:PATH |
|
We'll eventually want to get rid of all references to absolute paths/drive letters (as per their docs), but one thing at a time maybe. |
|
@tylerjereddy, thanks, that's great. Are you going to convert that to a PR on the scipy repo? |
|
Up to you--if you want to poach what you need for this PR that's fine too. I didn't want to open a third PR for this issue in the main repo. If you want to just cleanly fix the 32-bit integer entries and then have me try to just get the last one after in another PR that's probably fine too. |
|
It looks like you have all the pieces in your PR, so it would probably be easier to use yours, and get it done in one shot. |
|
alright, I'll try to clean it up a bit then |
|
Closing (again!); superseded by #11993. |
No description provided.