conda-forge / openblas-feedstock

A conda-smithy repository for openblas.
BSD 3-Clause "New" or "Revised" License
9 stars 39 forks source link

Enable osx_arm64 for ilp64 via conda_build_config.yaml #146

Closed mkitti closed 2 years ago

mkitti commented 2 years ago

Checklist

Revisits #105

conda-forge-linter commented 2 years ago

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.

mkitti commented 2 years ago

@conda-forge-admin, please rerender

mkitti commented 2 years ago

cc: @ngam, also see https://github.com/conda-forge/openblas-feedstock/issues/131

mkitti commented 2 years ago

huh, this may actually work

h-vetinari commented 2 years ago

I don't have anything against this in principle, but with the lib names & infrastructure on osx-arm (which I don't feel I know well enough), I'm going to defer to @isuruf for this one.

mkitti commented 2 years ago

@h-vetinari , if you do know the other platforms, could you check that the OBJCONV=objconv make parameter does not create other issues?

ngam commented 2 years ago

Could you add the following to conda-forge.yml in the root of the repo to see what it produces for osx-arm64?

azure:
  store_build_artifacts: true
ngam commented 2 years ago

Similarly, I do not know enough to weigh in on this, but hopefully @isuruf can help us out. In the meanwhile I am happy to test this locally. I can also build it locally, but usually I prefer to download the artifacts as produced in the PR and then test locally.

isuruf commented 2 years ago

I now remember the last time I tried this. objconv doesn't seem to support arm64 binaries. I'm not sure the artifact produced would work.

mkitti commented 2 years ago

Upstream Julia seems to use vanilla objconv: https://github.com/JuliaLang/julia/blob/master/deps/objconv.mk https://github.com/JuliaLang/julia/blob/91f068c5c219275f1115056084417057a66240b7/deps/openblas.mk#L37 https://github.com/JuliaPackaging/Yggdrasil/blob/fd160480030edfa8eb7dd21e19db2e7aff323a2f/O/OpenBLAS/common.jl#L68

Perhaps it does work?

mkitti commented 2 years ago

Could you add the following to conda-forge.yml in the root of the repo to see what it produces for osx-arm64?

Done

ngam commented 2 years ago

Could you add the following to conda-forge.yml in the root of the repo to see what it produces for osx-arm64?

Done

Need to rerender 😛

@conda-forge-admin, please rerender

ngam commented 2 years ago

I now remember the last time I tried this. objconv doesn't seem to support arm64 binaries. I'm not sure the artifact produced would work.

@isuruf could you please double-check on the naming and suffix changes @mkitti made? Could we at least make sure these don't change the current setup? I will try to figure out a local test for this, but I need a little bit of thinking... (any advice btw? replace good ol openblas and see if things like scipy work? assuming the different suffixes conflict...)

ngam commented 2 years ago
>>> ctypes.cdll['/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64_.dylib']
<CDLL '/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64_.dylib', handle 20a04e380 at 0x105283ac0>
>>> 
>>> ctypes.cdll['/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64_.dylib']
<CDLL '/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64_.dylib', handle 20a04e380 at 0x105283ac0>
>>> ctypes.cdll['/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64__armv8p-r0.3.21.dylib']
<CDLL '/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64__armv8p-r0.3.21.dylib', handle 20a04e380 at 0x1054b9060>
>>> ctypes.cdll['/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64__vortexp-r0.3.21.dylib']
<CDLL '/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64__vortexp-r0.3.21.dylib', handle 20a04e380 at 0x1054b90c0>
>>> ctypes.cdll['/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64__vortexp-r0.3.21.dylib']
<CDLL '/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64__vortexp-r0.3.21.dylib', handle 20a04e380 at 0x1054b90c0>
>>> ctypes.cdll['/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64_p-r0.3.21.dylib']
<CDLL '/Users/ngam/.Mambaforge-MacOSX-arm64/envs/test/lib/libopenblas64_p-r0.3.21.dylib', handle 20a04e380 at 0x1054b8790>
>>> 
mkitti commented 2 years ago

Does this work?

isuruf commented 2 years ago

objconv doesn't seem to support CPU_TYPE_ARM64 if I read the source correctly at https://github.com/staticfloat/objconv/blob/master/src/macho.cpp#L18-L23

mkitti commented 2 years ago

That mirror of objconv is quite old. That said I know @staticfloat this figured out for Julia CI, so perhaps he could chime in.

mkitti commented 2 years ago

I just checked the latest source code from https://www.agner.org/optimize/ and those lines are exactly the same:

// Machine names
SIntTxt MacMachineNames[] = {
   {MAC_CPU_TYPE_I386,      "Intel 32 bit"},
   {MAC_CPU_TYPE_X86_64,    "Intel 64 bit"},
   {MAC_CPU_TYPE_ARM,       "Arm"},
   {MAC_CPU_TYPE_SPARC,     "Sparc"},
   {MAC_CPU_TYPE_POWERPC,   "Power PC 32 bit"},
   {MAC_CPU_TYPE_POWERPC64, "Power PC 64 bit"}
};

Perhaps the ARM support is generic?

Julia builds openblas-ilp64 just fine with version 2.49 of objconv: https://github.com/JuliaPackaging/Yggdrasil/blob/32d14ffa78c8f3deeb729cc1f93e53b3aebf6be3/O/Objconv/build_tarballs.jl#L10-L11

That's the exact same version in conda-forge: https://github.com/conda-forge/objconv-feedstock/blob/3dab785a41bebd0e91982441eac9c46341045dad/recipe/meta.yaml#L2

The objconv change log is as follows

2022-04-28 version 2.53
* add disassembler support for AVX512-FP16, etc.

2020-06-08 version 2.52
* fix  problem in asm syntax output for lea

2018-10-08 version 2.51
* fix bug in movq mm0,rax

2018-08-15 version 2.50
* minor change in omf2cof

2018-01-29 version 2.49
* fixed minor problem with disassembly of DLL
ngam commented 2 years ago

@isuruf what would be a good test here short of building Julia with this to see if it works?

Nothing else seems to depend on this thing... https://cs.github.com/?scopeName=All+repos&scope=&q=org%3Aconda-forge+openblas-ilp64

mkitti commented 2 years ago

Update: I spoke with Elliott Saba and https://github.com/staticfloat/objconv is now the latest version at v2.53. Perhaps the conda-forge version will update soon?

ngam commented 2 years ago

Update: I spoke with Elliott Saba and https://github.com/staticfloat/objconv is now the latest version at v2.53. Perhaps the conda-forge version will update soon?

should be done soon-ish. I will try to build julia natively again with the updated one (remember it got stuck previously...)

xref https://github.com/conda-forge/objconv-feedstock/pull/5

ngam commented 2 years ago

Alright, v2.53 is live for osx-arm64.

@conda-forge-admin, restart ci

ngam commented 2 years ago

The update didn't improve the issue with compiling natively on m1 fwiw, not sure if it is related after all..

mkitti commented 2 years ago

The update didn't improve the issue with compiling natively on m1 fwiw, not sure if it is related after all..

Did you switch back to the main branch of the julia-feedstock to try to compile with USE_SYSTEM_BLAS=1 ? https://github.com/conda-forge/julia-feedstock/blob/7fb588639bb7a857c91e5035423657dd5526c3e6/recipe/build.sh#L45

I don't think the update should have. It shouldn't have changed anything. It was just to make absolutely sure that we're already using the same version that upstream Julia is using.

mkitti commented 2 years ago

I'm not sure I understand the Travis CI failure here? It stalled.

mkitti commented 2 years ago

@isuruf, Also where are we with this at the moment? I have verified that the objconv we are using is the exact same objconv that Julia uses to build openblas. That build seems to work.

Since Julia is the main dependency that needs this at all, could we proceed and see we can use this package to at least build the core parts of Julia?

ngam commented 2 years ago

I'm not sure I understand the Travis CI failure here? It stalled.

It could be related to this https://github.com/conda-forge/openblas-feedstock/pull/146#discussion_r959074394. Don't overthink it; TravisCI is pretty sensitive and problematic.

mkitti commented 2 years ago

I'm on my Mac Studio at the moment. How do I download this to test it?

isuruf commented 2 years ago

We can give this a try.

ngam commented 2 years ago

I'm on my Mac Studio at the moment. How do I download this to test it?

I can upload it to my channel if you want too, but in general follow something like this https://conda-forge.org/docs/maintainer/knowledge_base.html#testing-the-packages or my personal workflow:

  1. download the artifacts from here: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=561711&view=artifacts&pathAsName=false&type=publishedArtifacts
  2. upload to your personal conda channel, first conda install anaconda-client, then anaconda upload DOWNLOADED_PATH
  3. mix and match by adding your channel to the env, -c yourchannel
  4. if you want to do this as part of an automated build, but for testing only!, then change the channel sources as I have shown you in julia-feedstock, look for the yaml file under .ci_support and add your channel above conda-forge in the channel sources section
ngam commented 2 years ago

Also, I already tested this particular artifact https://github.com/conda-forge/openblas-feedstock/pull/146#issuecomment-1232861684

Tag me in whatever PR you want me to weigh in

github-actions[bot] commented 2 years ago

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

Thus the PR was passing and merged! Have a great day!

mkitti commented 2 years ago

Great. Thank you @isuruf .

rgommers commented 2 years ago

FYI: this new build seems to have yielded a couple of issues on both x86_64 and arm64 in SciPy: https://github.com/scipy/scipy/issues/16949. That's with the regular 32-bit openblas builds, so it's probably the upgrade to gfortran 11 here.

mkitti commented 2 years ago

@rgommers @isuruf @ngam To make absolutely sure that the issue is bumping fortran compiler version rather than the other changes here, is there a way to obtain a build from before the merge commit but with the change in fortran?

ngam commented 2 years ago

That build doesn't exist; we can create it pretty easily though. Let'm spin a PR for this.

I now remember the last time I tried this. objconv doesn't seem to support arm64 binaries. I'm not sure the artifact produced would work.

@isuruf could you please double-check on the naming and suffix changes @mkitti made? Could we at least make sure these don't change the current setup? I will try to figure out a local test for this, but I need a little bit of thinking... (any advice btw? replace good ol openblas and see if things like scipy work? assuming the different suffixes conflict...)

lol https://github.com/conda-forge/openblas-feedstock/pull/146#issuecomment-1230887885 I definitely made that comment thinking of a previous scipy--openblas issue. Sorry folks!

rgommers commented 2 years ago

Thanks for following up, and for opening a new PR for testing

@isuruf could you please double-check on the naming and suffix changes @mkitti made?

The contents of this PR look good to me, and if there was any issue around suffix or similar, we'd see much more breakage I'd think.