Use new Accelerate from OSX>=13.3 #105

isuruf · 2023-10-17T00:46:05Z

Checklist

Used a personal fork of the feedstock to propose changes
Bumped the build number (if the version is unchanged)
Reset the build number to 0 (if the version changed)
Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
Ensured the license file is being packaged.

Closes #103

conda-forge-webservices · 2023-10-17T00:46:11Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

h-vetinari

This is amazing!

I only get the broad strokes, but AFAICT:

Apple's implementation has a $NEWLAPACK suffix for all the BLAS/LAPACK symbols
We create a mapping from the normal expected names to the Apple ones and register them in an alias layer, together with re-exporting the libraries that actually contain those symbols.
For the legacy accelerate we could take BLAS but needed to replace LAPACK wholesale (hence both liblapack & liblapacke)
Apparently we still need to re-export liblapacke even in the new setup
There are some missing/incompatible symbols and you're providing a wrapper layer
All that (wrapper layer, aliases, re-exported symbols) is bundled into one new shared library

Probably still missing a bunch of details, perhaps we could add some comments in build.sh and document the intent a bit?

recipe/meta.yaml

…nda-forge-pinning 2023.10.18.13.10.31

isuruf · 2023-10-27T13:35:18Z

Okay, this is ready. I'll build these locally and upload when this PR is merged.

h-vetinari

Okay, this is ready. I'll build these locally and upload when this PR is merged.

Not sure if you were waiting for review or anything, but basically this LGTM. I mostly have some nits and suggestions for clarification.

recipe/build.sh

recipe/meta.yaml

Co-authored-by: h-vetinari <[email protected]>

h-vetinari · 2023-12-19T02:01:37Z

The reported errors are:
- Encountered problems while solving:
-   - nothing provides __osx >=13.3 needed by libblas-3.9.0-20_osx64_newaccelerate

You said above you'll build these locally, so I'm assuming there's a reason why we cannot just set MACOSX_SDK_VERSION to 13.3 for the new_accelerate builds? AFAICT that should be possible since conda-forge/conda-forge-ci-setup-feedstock#281.

isuruf · 2023-12-19T03:03:03Z

We only allow users to install newaccelerate builds for 13.3 and therefore we use the __osx>=13.3 virtual package. Since the runners are not on 13.3, the package is not installable there.

jakirkham · 2023-12-19T03:44:35Z

recipe/conda_build_config.yaml

@@ -3,6 +3,7 @@ blas_impl:
 - mkl                      # [x86 or x86_64]
 - blis                     # [x86 or x86_64]
 - accelerate               # [osx]
+- newaccelerate            # [osx]


Would it make sense to version this somehow (maybe OS or library version if specified)?

Just thinking about whether we might wind up with newnewaccelerate in a future update 😉

Apple calls it NEWLAPACK, so if they call a newer one NEWNEWLAPACK then we can call it newnewaccelerate. :D

Fair enough :)

h-vetinari · 2023-12-19T06:27:01Z

Since the runners are not on 13.3, the package is not installable there.

I was surprised that the runners don't offer 13.x yet, and actually they've been added a while ago -- though for some reason they recently got marked as in preview months later 🤷. The readme link from the azure pipeline docs goes to here - I guess the separation between Azure Pipelines and GHA is pretty thin, since it's all MSFT in the end.

In any case, we could probably build & test this in CI if we upgrade the runners.

h-vetinari · 2023-12-19T06:55:31Z

In any case, we could probably build & test this in CI if we upgrade the runners.

I tested this in #112. The good news is that the image can be used and the resolution for __osx>=13.3 passes, the bad news is that there are segfaults in the wrappers still:

103/107 Test #103: BLAS-xblat3z .....................***Failed    0.04 sec
Running: /Users/runner/miniforge3/conda-bld/blas-split_1702968007651/work/build/bin/xblat3z
ARGS= INPUT_FILE;/Users/runner/miniforge3/conda-bld/blas-split_1702968007651/work/BLAS/TESTING/zblat3.in
M cannot be less than zero 0, but has value -1. BLAS error: Parameter number 4 passed to cblas_zgemm had an invalid value
** On entry to ZGEMM , parameter number  1 had an illegal value
** On entry to ZGEMM , parameter number  1 had an illegal value
** On entry to ZGEMM , parameter number  1 had an illegal value
** On entry to ZGEMM , parameter number  2 had an illegal value
** On entry to ZGEMM , parameter number  2 had an illegal value
** On entry to ZGEMM , parameter number  2 had an illegal value
CMake Error at /Users/runner/miniforge3/conda-bld/blas-split_1702968007651/work/TESTING/runtest.cmake:36 (message):
  Test
  /Users/runner/miniforge3/conda-bld/blas-split_1702968007651/work/build/bin/xblat3z
  returned 255

h-vetinari · 2024-01-03T08:56:39Z

Unfortunately, we're (still) getting

Test ERROR:
dyld[15784]: missing symbol called

everywhere....

h-vetinari · 2024-03-24T03:09:18Z

FWIW, there's a PR in scipy that does something very similar, and which is nearing completion. Perhaps that can provide some inspiration or at least clues.

isuruf · 2024-03-25T15:49:41Z

Clues as to why the two tests fail?

h-vetinari · 2024-03-26T03:59:19Z

Perhaps? @thalassemia dug in pretty deeply into the topic, and fixed a couple of long-standing issues around this in scipy.

Use new Accelerate from OSX>=13.3

cce10af

h-vetinari reviewed Oct 17, 2023

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

isuruf added 5 commits October 18, 2023 09:14

new_accelerate -> newaccelerate

9195b68

New SDK version

085bd5a

fix typo

1dc6130

MNT: Re-rendered with conda-build 3.24.0, conda-smithy 3.27.1, and co…

b88ca46

…nda-forge-pinning 2023.10.18.13.10.31

Fix compiler for accelerate

f461b87

isuruf force-pushed the new_accelerate branch from 9cbe00c to f461b87 Compare October 18, 2023 14:20

isuruf added 6 commits October 18, 2023 09:40

link to accelerate

e58259b

use older sdk for old accelerate

e0fadca

No need to export appleblas

f418262

Remove catlas symbols too

c80256c

fix logic error

f3ccf63

Fix symbols

52fdcce

isuruf marked this pull request as ready for review October 27, 2023 13:33

isuruf requested review from jakirkham, ocefpaf and pelson as code owners October 27, 2023 13:33

h-vetinari reviewed Nov 21, 2023

View reviewed changes

recipe/build.sh Outdated Show resolved Hide resolved

recipe/build.sh Outdated Show resolved Hide resolved

recipe/build.sh Outdated Show resolved Hide resolved

recipe/build.sh Show resolved Hide resolved

recipe/meta.yaml Outdated Show resolved Hide resolved

h-vetinari and others added 4 commits December 19, 2023 10:09

Merge remote-tracking branch 'up/main' into new_accelerate

b1a1cfa

Add explanation

e6a62a6

Comments update

033e443

Co-authored-by: h-vetinari <[email protected]>

Use selectors

55b32a6

Co-authored-by: h-vetinari <[email protected]>

h-vetinari mentioned this pull request Dec 19, 2023

Numpy Conda currently slower than Numpy pip on Apple Silicon conda-forge/numpy-feedstock#303

Open

1 task

jakirkham reviewed Dec 19, 2023

View reviewed changes

h-vetinari mentioned this pull request Dec 19, 2023

Testing new image #112

Closed

isuruf added 3 commits January 3, 2024 08:43

use new image

11e3dda

rerender

f50ebc9

fix test

574ce76

isuruf and others added 2 commits January 11, 2024 11:36

Fix some missing symbols

4d87fcd

Merge remote-tracking branch 'upstream/main' into new_accelerate

890ac14

isuruf closed this Mar 25, 2024

isuruf reopened this Mar 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new Accelerate from OSX>=13.3 #105

Use new Accelerate from OSX>=13.3 #105

isuruf commented Oct 17, 2023 •

edited by h-vetinari

Loading

conda-forge-webservices bot commented Oct 17, 2023

h-vetinari left a comment •

edited

Loading

isuruf commented Oct 27, 2023

h-vetinari left a comment

h-vetinari commented Dec 19, 2023

isuruf commented Dec 19, 2023

jakirkham Dec 19, 2023

isuruf Dec 19, 2023

jakirkham Dec 19, 2023

h-vetinari commented Dec 19, 2023 •

edited

Loading

h-vetinari commented Dec 19, 2023

h-vetinari commented Jan 3, 2024

h-vetinari commented Mar 24, 2024

isuruf commented Mar 25, 2024

h-vetinari commented Mar 26, 2024

Use new Accelerate from OSX>=13.3 #105

Are you sure you want to change the base?

Use new Accelerate from OSX>=13.3 #105

Conversation

isuruf commented Oct 17, 2023 • edited by h-vetinari Loading

conda-forge-webservices bot commented Oct 17, 2023

h-vetinari left a comment • edited Loading

Choose a reason for hiding this comment

isuruf commented Oct 27, 2023

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari commented Dec 19, 2023

isuruf commented Dec 19, 2023

jakirkham Dec 19, 2023

Choose a reason for hiding this comment

isuruf Dec 19, 2023

Choose a reason for hiding this comment

jakirkham Dec 19, 2023

Choose a reason for hiding this comment

h-vetinari commented Dec 19, 2023 • edited Loading

h-vetinari commented Dec 19, 2023

h-vetinari commented Jan 3, 2024

h-vetinari commented Mar 24, 2024

isuruf commented Mar 25, 2024

h-vetinari commented Mar 26, 2024

isuruf commented Oct 17, 2023 •

edited by h-vetinari

Loading

h-vetinari left a comment •

edited

Loading

h-vetinari commented Dec 19, 2023 •

edited

Loading