Added Build&Deploy for `mac-os` #229

Abelarm · 2022-10-12T18:13:16Z

Resolving the #139

I have added the build_deploy command for mac-os integrated with the windows one. Added the mac-os (darwin) to the platform to which apply static linking.

TODO before merging:

Test on mac-os
Un-comment the part of the code that trigger only on fix_deploy
Un-comment the part of the code that deploy the whl

codecov · 2022-10-12T18:17:32Z

Codecov Report

Merging #229 (88db146) into develop (a5e0dae) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #229   +/-   ##
========================================
  Coverage    45.76%   45.76%           
========================================
  Files           84       84           
  Lines         9096     9096           
========================================
  Hits          4163     4163           
  Misses        4933     4933

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

phoebe-p · 2022-10-13T03:04:27Z

Thank you @Abelarm, I promised @dalonsoa yesterday that I would figure out how to make the wheels for macOS but it seems you have saved me some work! I can test the wheel on my computer.

Abelarm · 2022-10-13T06:34:55Z

Thank you @Abelarm, I promised @dalonsoa yesterday that I would figure out how to make the wheels for macOS but it seems you have saved me some work! I can test the wheel on my computer.

Great!! If you want to test the static linking would be great 🚀

phoebe-p · 2022-10-14T04:26:21Z

Ok, I have tested the relevant wheel downloaded from here in a new Python 3.10 virtual environment, but it does not work, which is not surprising because the builds are only for x86 architectures and I have an M1 chip. I think this tool can help us? I will give it a go.

Edit: I don't have access to an older Mac machine, maybe @dalonsoa does? Either way, if we have to use the cibuildwheel tool to make the wheels for M1/Apple Silicon architectures, I suppose we will have to test them again anyway.

Abelarm · 2022-10-14T09:18:24Z

Thank you @phoebe-p! Did you try to install/run in a Rosetta environment? Something like THIS

If you decide to use the other build method is probably best to use the same one for all the platforms.

dalonsoa · 2022-10-17T19:29:03Z

I'm afraid this is not correct. While you can build wheels for linux this way, PyPI only accept wheels created with manylinux to maximise the compatibility with Linux distros and versions. So, Linux builds need its own action, as it was before.

As for MacOS, it seems building things in MacOS is not working, so I have no wheel to test, I'm afraid.

dalonsoa

See my other comment.

phoebe-p · 2022-10-17T23:00:59Z

@dalonsoa the macOS builds in my (separate) branch are failing for some reason I don't understand, but @Abelarm's are working, just only building on push so they are on his fork here. Would be great if you could test it, since I don't have a Rosetta environment set up. I really would like to get this working on M1 chips too because I will not be the only person who wants to use Solcore on a new Mac - so I will keep trying on that front.

dalonsoa · 2022-10-18T03:56:40Z

Very true - sorry for the confusion @Abelarm !

So, the MacOS wheels do work in my laptop totally fine and I'm able to run the PDD solver without compiling anything. However, I do have a fortran compiler installed. As you have pointed out, we should statically link everything so a compiler is not needed. That is taken care for linux in the manilinux action, and it was accounted for in Windows. I guess that removing the conditional statement that you mention should do the trick and include the libraries also in MacOS, so let's try that.

Otherwise, I'm happy this works - at least for non M1 chips!

dalonsoa · 2022-10-18T04:06:27Z

@phoebe-p , it seems that cibuildwheel could be the only tool we need to do this, including creating manylinux wheels! If you manage to make it work for M1, we might want to replace the current CI for that one - it will be way simpler!

phoebe-p · 2022-10-18T04:14:18Z

I actually had the MacOS x86 build using cibuildwheel working here before I broke it again by trying to add the ARM build in the next commit, so there are some MacOS wheels you could try there. I will see if I can get a version working building everything using just cibuildwheel since as you say the workflow will be a lot more compact (just a bit slow testing this because the build takes a while each time in GitHub Actions). If I understood the discussion above directly, I should modify the extra_link_args argument in setup.py to make sure everything is statically linked.

dalonsoa · 2022-10-18T05:48:04Z

Indeed, it does work. Funny enough, it triggers the safety features of MacOS and I need to manually authorise one by one a bunch of fortran libraries bundled with the .so PDD library. So, I guess it does statically link stuff, after all.

Interestingly, the .so library created in @Abelarm 's action has exactly the same size but triggers no complain from MacOS, so I'm not sure if somehow it does not include the fortran dependencies or it does include them but are signed or something and MacOS is happy with them...

phoebe-p · 2022-10-18T10:52:06Z

Ok, sorry to derail this PR with discussion about the M1 wheels/use of cibuildwheel, I can open a new issue to discuss this if that is preferable... After trying many, many different things today, I do not think that this tool will work for us at the moment. As far as I understand it, the runners used by cibuildwheel are not really ARM64 architecture, they are just supposed to build a wheel which is compatible with the chosen architecture (see discussion here). This causes issues when e.g. installing gfortran, because it will not install the ARM64 version, and then the compilation fails... It is a mess. I also could not get the Windows builds to work with this tool, so for now there does not seem to be any reason to use cibuildwheel (it does work for 'normal' macOS and Ubuntu, but that should be easy anyway, and then there is the issue with the static linking requiring manual authorisation which @dalonsoa mentions in the previous comment).

On the bright side, since the last time I tried (#209), compiling the PDD locally on my own system is now as easy as before (i.e. it just works with homebrew gcc/gfortran 12.2.0), so it is also less urgent that we provide wheels to M1 macOS users. Of course, I could simply build the wheels for different Python versions on M1 myself for the time being, but I don't think there is a way of integrating that with the automated release to PyPI which we are trying to build here.

Abelarm · 2022-10-19T17:31:45Z

Guys sorry If i didn't participate actively in the discussion (which was extremely interesting) but I had really some rough days at works. But now I should be able to follow it more actively.

So if I got it correctly, at time of writing, we cannot build solcore for mac-os M1 or M2 (not even using cibuildwheel) so we should fall back to my initial approach. Regarding the static linking we should remove the condition so it should work for "bare-metal" mac-os

…dos and macos, added static linking to all platforms

…x build

Abelarm · 2022-10-19T17:56:53Z

@dalonsoa @phoebe-p

Ok code changes applied, the sad thing is that we still need to keep the ugly if since for some reason if removed the manylinux build fails:

/opt/rh/devtoolset-10/root/usr/libexec/gcc/x86_64-redhat-linux/10/ld: /opt/rh/devtoolset-10/root/usr/lib/gcc/x86_64-redhat-linux/10/crtbeginT.o: relocation R_X86_64_32 against hidden symbol `__TMC_END__' can not be used when making a shared object
      collect2: error: ld returned 1 exit status
      error: Command "/opt/rh/devtoolset-10/root/usr/bin/gfortran -Wall -g -Wall -g -shared build/temp.linux-x86_64-cpython-37/build/src.linux-x86_64-3.7/solcore/poisson_drift_diffusion/ddModelmodule.o build/temp.linux-x86_64-cpython-37/build/src.linux-x86_64-3.7/build/src.linux-x86_64-3.7/solcore/poisson_drift_diffusion/fortranobject.o build/temp.linux-x86_64-cpython-37/solcore/poisson_drift_diffusion/DDmodel-current.o build/temp.linux-x86_64-cpython-37/build/src.linux-x86_64-3.7/solcore/poisson_drift_diffusion/ddModel-f2pywrappers2.o -L/opt/rh/devtoolset-10/root/usr/lib/gcc/x86_64-redhat-linux/10 -L/opt/rh/devtoolset-10/root/usr/lib/gcc/x86_64-redhat-linux/10 -lgfortran -o build/lib.linux-x86_64-cpython-37/solcore/poisson_drift_diffusion/ddModel.cpython-37m-x86_64-linux-gnu.so -static -static-libgfortran -static-libgcc" failed with exit status 1

really not an expert on gfortran and static linking so 🤷‍♂️

tcaduser · 2022-10-19T18:47:43Z

@dalonsoa @phoebe-p

Ok code changes applied, the sad thing is that we still need to keep the ugly if since for some reason if removed the manylinux build fails:

/opt/rh/devtoolset-10/root/usr/libexec/gcc/x86_64-redhat-linux/10/ld: /opt/rh/devtoolset-10/root/usr/lib/gcc/x86_64-redhat-linux/10/crtbeginT.o: relocation R_X86_64_32 against hidden symbol `__TMC_END__' can not be used when making a shared object
      collect2: error: ld returned 1 exit status
      error: Command "/opt/rh/devtoolset-10/root/usr/bin/gfortran -Wall -g -Wall -g -shared build/temp.linux-x86_64-cpython-37/build/src.linux-x86_64-3.7/solcore/poisson_drift_diffusion/ddModelmodule.o build/temp.linux-x86_64-cpython-37/build/src.linux-x86_64-3.7/build/src.linux-x86_64-3.7/solcore/poisson_drift_diffusion/fortranobject.o build/temp.linux-x86_64-cpython-37/solcore/poisson_drift_diffusion/DDmodel-current.o build/temp.linux-x86_64-cpython-37/build/src.linux-x86_64-3.7/solcore/poisson_drift_diffusion/ddModel-f2pywrappers2.o -L/opt/rh/devtoolset-10/root/usr/lib/gcc/x86_64-redhat-linux/10 -L/opt/rh/devtoolset-10/root/usr/lib/gcc/x86_64-redhat-linux/10 -lgfortran -o build/lib.linux-x86_64-cpython-37/solcore/poisson_drift_diffusion/ddModel.cpython-37m-x86_64-linux-gnu.so -static -static-libgfortran -static-libgcc" failed with exit status 1

really not an expert on gfortran and static linking so 🤷‍♂️

@Abelarm

FWIW, Linux is pretty strict about statically linking non-relocatable archives into a shared library. Unfortunately it appears that the devtoolset developers provide an archive with non-sharable code.

If you dynamically link against libgfortran, the auditwheel tool for Linux is supposed to be able to collect these external libraries and package them into your wheel:
https://github.com/pypa/auditwheel

That being said, I would think libgfortran is available on most Linux systems.

Similarly for macOS, the delocate package is supposed to package external dependencies into your wheel:
https://github.com/matthew-brett/delocate.

dalonsoa

A couple of minor changes, but otherwise it looks good to me!

.github/workflows/build_deploy_wheels.yml

dalonsoa · 2022-10-20T05:37:42Z

On the bright side, since the last time I tried (#209), compiling the PDD locally on my own system is now as easy as before (i.e. it just works with homebrew gcc/gfortran 12.2.0), so it is also less urgent that we provide wheels to M1 macOS users. Of course, I could simply build the wheels for different Python versions on M1 myself for the time being, but I don't think there is a way of integrating that with the automated release to PyPI which we are trying to build here.

@phoebe-p , after this is merge we need to update the documentation to reflect this changes in the installation process, so maybe when we do that, you can mention the instructions about compiling things for M1.

dalonsoa · 2022-10-20T05:40:01Z

@Abelarm , I tested this already for MacOS and it was working, so you can tick that. Do not remove the upload artefact part, as I think it is useful to have the binary wheels available for dev versions.

Abelarm · 2022-10-20T16:33:56Z

@dalonsoa @phoebe-p
Ok code changes applied, the sad thing is that we still need to keep the ugly if since for some reason if removed the manylinux build fails:
/opt/rh/devtoolset-10/root/usr/libexec/gcc/x86_64-redhat-linux/10/ld: /opt/rh/devtoolset-10/root/usr/lib/gcc/x86_64-redhat-linux/10/crtbeginT.o: relocation R_X86_64_32 against hidden symbol `__TMC_END__' can not be used when making a shared object
      collect2: error: ld returned 1 exit status
      error: Command "/opt/rh/devtoolset-10/root/usr/bin/gfortran -Wall -g -Wall -g -shared build/temp.linux-x86_64-cpython-37/build/src.linux-x86_64-3.7/solcore/poisson_drift_diffusion/ddModelmodule.o build/temp.linux-x86_64-cpython-37/build/src.linux-x86_64-3.7/build/src.linux-x86_64-3.7/solcore/poisson_drift_diffusion/fortranobject.o build/temp.linux-x86_64-cpython-37/solcore/poisson_drift_diffusion/DDmodel-current.o build/temp.linux-x86_64-cpython-37/build/src.linux-x86_64-3.7/solcore/poisson_drift_diffusion/ddModel-f2pywrappers2.o -L/opt/rh/devtoolset-10/root/usr/lib/gcc/x86_64-redhat-linux/10 -L/opt/rh/devtoolset-10/root/usr/lib/gcc/x86_64-redhat-linux/10 -lgfortran -o build/lib.linux-x86_64-cpython-37/solcore/poisson_drift_diffusion/ddModel.cpython-37m-x86_64-linux-gnu.so -static -static-libgfortran -static-libgcc" failed with exit status 1
really not an expert on gfortran and static linking so man_shrugging
@Abelarm

FWIW, Linux is pretty strict about statically linking non-relocatable archives into a shared library. Unfortunately it appears that the devtoolset developers provide an archive with non-sharable code.

If you dynamically link against libgfortran, the auditwheel tool for Linux is supposed to be able to collect these external libraries and package them into your wheel: https://github.com/pypa/auditwheel

That being said, I would think libgfortran is available on most Linux systems.

Similarly for macOS, the delocate package is supposed to package external dependencies into your wheel: https://github.com/matthew-brett/delocate.

Thank you @tcaduser for the really detailed answer, really great!!
So if we want to statically linking also the linux one we should use an external tool to prevent the error for happening, but agreeing with what you said probably not worth the effort since libgfortran is available on most Linux systems.

Regarding the PR @dalonsoa I think everything is tidy and clean now, I ma not sure we should trigger the whole build for each push, probably leave it as it was before (triggering only on a specific branch) would be better :)

tcaduser · 2022-10-20T17:08:54Z

@Abelarm

I'm glad to help. I have been going through the process of package packaging lately, and it is tricky.

You may want to use something like auditwheel, since libgfortran is versioned.

However it may not matter if all systems have it by the same name.

On Ubuntu 22.04:
$ find /usr/lib/ -iname 'libgfortran*'

/usr/lib/x86_64-linux-gnu/libgfortran.so.5.0.0
/usr/lib/x86_64-linux-gnu/libgfortran.so.5

On manylinux2014:

# find /lib64/ -iname 'libgfortran*'
/lib64/libgfortran.so.5
/lib64/libgfortran.so.5.0.0

I have not tried Solcore yet, and I am very interested trying the drift diffusion simulation.

tcaduser · 2022-10-20T17:26:15Z

@Abelarm

This a page specifically referencing gfortran as a contender for auditwheel:
https://realpython.com/python-wheels/#bundling-shared-libraries

Abelarm · 2022-10-20T18:18:23Z

Whoa a lot to learn here... 😃

I think it's already possible to try the drift diffusion simulation for all the systems:

linux you could try even without the libgfortran
mac-os probably need the libgfortran (not sure)
windows tested without the libgfortran and it works

@dalonsoa correct me if I am wrong

dalonsoa · 2022-10-20T21:09:07Z

Regarding the PR @dalonsoa I think everything is tidy and clean now, I ma not sure we should trigger the whole build for each push, probably leave it as it was before (triggering only on a specific branch) would be better :)

OK, let's trigger it only for push events to main and develop. I really want to keep this being run regularly - and easy to test - to make sure things don't break along the way. After that modification, we can merge and release a new version!!

Removed too many blank lines Inverting the order to see it runs Rolling back

Abelarm added 4 commits October 12, 2022 19:20

First commit to test an unified build_deploy for all the OS

cebb5b5

Fixed indentation error

93f4c79

Adding OS dependecies install

6f3e981

changed command orders

0ca9b85

dalonsoa requested changes Oct 17, 2022

View reviewed changes

Reverted the build process for linux, new build applying only for win…

6c54c55

…dos and macos, added static linking to all platforms

Abelarm force-pushed the test_unified_build_deploy branch from 5337174 to 6c54c55 Compare October 19, 2022 17:38

Abelarm added 3 commits October 19, 2022 19:40

cannot use - inside github action

8b7ba5c

wrong name for action

718803d

Need to keep the if statement because removing it breaks the manylinu…

2b0ef81

…x build

Adding back the upload to PyPI and artifact

01b736c

Abelarm force-pushed the test_unified_build_deploy branch from 2ce3f18 to 01b736c Compare October 19, 2022 18:09

Merge branch 'develop' into test_unified_build_deploy

1f4f56d

dalonsoa approved these changes Oct 20, 2022

View reviewed changes

.github/workflows/build_deploy_wheels.yml Outdated Show resolved Hide resolved

.github/workflows/build_deploy_wheels.yml Outdated Show resolved Hide resolved

.github/workflows/build_deploy_wheels.yml Outdated Show resolved Hide resolved

dalonsoa mentioned this pull request Oct 20, 2022

Implement pytest as test suite run in the CI qpv-research-group/sunglass#2

Closed

Abelarm force-pushed the test_unified_build_deploy branch 2 times, most recently from 9891caa to 4b7692e Compare October 20, 2022 15:36

Abelarm added 2 commits October 21, 2022 00:01

Resolved comments raised by @dalonsoa

a2c87c5

Removed too many blank lines Inverting the order to see it runs Rolling back

Added push only to specific branches

88db146

Abelarm force-pushed the test_unified_build_deploy branch from c4a4cee to 88db146 Compare October 20, 2022 22:02

dalonsoa merged commit 3173f25 into qpv-research-group:develop Oct 21, 2022

dalonsoa mentioned this pull request Oct 27, 2022

Create binary wheels for Solcore #139

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Build&Deploy for `mac-os` #229

Added Build&Deploy for `mac-os` #229

Abelarm commented Oct 12, 2022 •

edited

codecov bot commented Oct 12, 2022 •

edited

phoebe-p commented Oct 13, 2022

Abelarm commented Oct 13, 2022

phoebe-p commented Oct 14, 2022 •

edited

Abelarm commented Oct 14, 2022

dalonsoa commented Oct 17, 2022

dalonsoa left a comment

phoebe-p commented Oct 17, 2022

dalonsoa commented Oct 18, 2022

dalonsoa commented Oct 18, 2022

phoebe-p commented Oct 18, 2022 •

edited

dalonsoa commented Oct 18, 2022

phoebe-p commented Oct 18, 2022 •

edited

Abelarm commented Oct 19, 2022

Abelarm commented Oct 19, 2022

tcaduser commented Oct 19, 2022

dalonsoa left a comment

dalonsoa commented Oct 20, 2022

dalonsoa commented Oct 20, 2022

Abelarm commented Oct 20, 2022

tcaduser commented Oct 20, 2022

tcaduser commented Oct 20, 2022

Abelarm commented Oct 20, 2022 •

edited

dalonsoa commented Oct 20, 2022

Added Build&Deploy for mac-os #229

Added Build&Deploy for mac-os #229

Conversation

Abelarm commented Oct 12, 2022 • edited

codecov bot commented Oct 12, 2022 • edited

Codecov Report

phoebe-p commented Oct 13, 2022

Abelarm commented Oct 13, 2022

phoebe-p commented Oct 14, 2022 • edited

Abelarm commented Oct 14, 2022

dalonsoa commented Oct 17, 2022

dalonsoa left a comment

Choose a reason for hiding this comment

phoebe-p commented Oct 17, 2022

dalonsoa commented Oct 18, 2022

dalonsoa commented Oct 18, 2022

phoebe-p commented Oct 18, 2022 • edited

dalonsoa commented Oct 18, 2022

phoebe-p commented Oct 18, 2022 • edited

Abelarm commented Oct 19, 2022

Abelarm commented Oct 19, 2022

tcaduser commented Oct 19, 2022

dalonsoa left a comment

Choose a reason for hiding this comment

dalonsoa commented Oct 20, 2022

dalonsoa commented Oct 20, 2022

Abelarm commented Oct 20, 2022

tcaduser commented Oct 20, 2022

tcaduser commented Oct 20, 2022

Abelarm commented Oct 20, 2022 • edited

dalonsoa commented Oct 20, 2022

Added Build&Deploy for `mac-os` #229

Added Build&Deploy for `mac-os` #229

Abelarm commented Oct 12, 2022 •

edited

codecov bot commented Oct 12, 2022 •

edited

phoebe-p commented Oct 14, 2022 •

edited

phoebe-p commented Oct 18, 2022 •

edited

phoebe-p commented Oct 18, 2022 •

edited

Abelarm commented Oct 20, 2022 •

edited