AllenCellModeling / aicspylibczi

Python module utilizing libCZI for reading Zeiss CZI files.
https://allencellmodeling.github.io/aicspylibczi
GNU General Public License v3.0
36 stars 8 forks source link

add arm64 macos build #102

Closed ianhi closed 2 years ago

ianhi commented 2 years ago

hopefully fixes: https://github.com/AllenCellModeling/aicspylibczi/issues/101

ianhi commented 2 years ago

oops i may have put in the wrong place

ianhi commented 2 years ago

ah ya beat me to it

psobolewskiPhD commented 2 years ago

Y'all lemme know (how) and I can test anything M1 related!

ianhi commented 2 years ago

I don't think this should affect any of the tests here so the only way to check is to run the deply script and try to use the generated wheel

evamaxfield commented 2 years ago

I don't think this should affect any of the tests here so the only way to check is to run the deply script and try to use the generated wheel

Agree. I leave this PR approve and deploy to @toloudis

codecov-commenter commented 2 years ago

Codecov Report

Merging #102 (c963e50) into main (0c9a19d) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #102   +/-   ##
=======================================
  Coverage   98.64%   98.64%           
=======================================
  Files           3        3           
  Lines         148      148           
=======================================
  Hits          146      146           
  Misses          2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0c9a19d...c963e50. Read the comment docs.

toloudis commented 2 years ago

I think we can test prior to merging by changing the conditions that the action runs under (and remove the publish step), then watch actions and see if anything fails. if the build succeeds, then we can reset the conditions and merge and move forward. Otherwise I can just merge and go for it blind instead of half blind :)

toloudis commented 2 years ago

i'll just do it, let's build it and see.

psobolewskiPhD commented 2 years ago

Should just show up here right? https://pypi.org/project/aicspylibczi/#files Edit: getting sleepy!! Edit2: Ill check tomorrow!

ianhi commented 2 years ago

Should just show up here right?

@psobolewskiPHD I think you can only get it from: https://github.com/AllenCellModeling/aicspylibczi/suites/4994059858/artifacts/146261793

evamaxfield commented 2 years ago

Whenever we push a new version, likely just a patch release, it should also be pushed to pypi

toloudis commented 2 years ago

yes - it won't go to pypi until we do a version bump and trigger the release publish process

psobolewskiPhD commented 2 years ago

Bad news, unless I screwed something up:

pip install /Users/piotrsobolewski/Downloads/artifact/aicspylibczi-3.0.4-cp3-cp39-macosx_11_0_arm64.whl
Processing /Users/piotrsobolewski/Downloads/artifact/aicspylibczi-3.0.4-cp39-cp39-macosx_11_0_arm64.whl
Requirement already satisfied: numpy>=1.14.1 in /Users/piotrsobolewski/Dev/miniforge3/envs/napari-env/lib/python3.9/site-packages (from aicspylibczi==3.0.4) (1.22.0)
Installing collected packages: aicspylibczi
Successfully installed aicspylibczi-3.0.4

But:

> lipo -info _aicspylibczi.cpython-39-darwin.so
Non-fat file: _aicspylibczi.cpython-39-darwin.so is architecture: x86_64

Edit: so it looks like wheels are packaged, but the wrong compiled? library is included.

evamaxfield commented 2 years ago

Sorry, why is this bad news? It looks like that install worked!

psobolewskiPhD commented 2 years ago

The install worked, but the library in the wheel is x86-64: Non-fat file: _aicspylibczi.cpython-39-darwin.so is architecture: x86_64 It should be arm64: artifact/aicspylibczi-3.0.4-cp3-cp39-macosx_11_0_arm64.whl So the wheel/bundling is making a "thing" for arm64, but it's not actually compiling an arm64 version of the library and placing it inside the bundle. Edit: this means that in a native arm64 environment this wheel will not work and will throw an architecture error.

evamaxfield commented 2 years ago

Here are the docs from cibuildwheel themselves if you find any insights in them... i can't see what went wrong: https://cibuildwheel.readthedocs.io/en/stable/faq/#apple-silicon

evamaxfield commented 2 years ago

And, looking at their direct example. It's the code we had before this PR :confused:

EDIT: correction we had auto universal2 instead of x86_64 universal2 I guess thats another configuration we could try????

psobolewskiPhD commented 2 years ago

I can't say I understand how any of this works. I don't get what cibuildwheel does, relative to the setup.py/cfg files. I assume the issue is that when the _aics bit is compiled it's not cross compiled, but just x86. I've fixed other things to compile by set the cmake flag CMAKE_OSX_ARCHITECTURES to arm64—but this library compiles just fine if i clone the repo. I think it must be something related to cmake because: https://github.com/AllenCellModeling/aicspylibczi/issues/101#issuecomment-1017252716 Trying to compile via pip also fails.