-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support building multi-platform images and add linux/arm64
#50
Conversation
Comparing to the failure in #48 (using Line 39 in 163cba5
and makes sense: #48 (comment) The failure in this PR (using
|
7daea6d
to
3f2b6d8
Compare
3f2b6d8
to
76481e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some progress today. Will investigate more another day.
ac7f2df
to
28bcb6f
Compare
1bdd697
to
ecac840
Compare
9ca46fb
to
401506e
Compare
116ad4e
to
16a474e
Compare
16a474e
to
ff2edf1
Compare
7fa55a4
to
53d2356
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\o/ nicely done. I've read thru all the latest commits again and it looks great. One small typo fix and a bunch of non-blocking comments (though I do think some of thing should be addressed).
4780de1
to
6b305c7
Compare
This implies using linux/amd64 since that is the sole target platform.
The repo contents of RAxML 8.2.12 do not support compiling on arm64. A few immediate changes are required, so put them in a fork repo. It's also not likely that these changes will make it back to the official RAxML repo with a version release, since development efforts have moved to RAxML-NG¹. ¹ https://groups.google.com/g/raxml/c/1cRSPXcZa1o/m/SakFeA3OAgAJ
These variables, useful for multi-platform builds¹, will be used in subsequent commits. ¹ https://www.docker.com/blog/faster-multi-platform-builds-dockerfile-cross-compilation-guide/
Future commits should aim to remove these by either (1) downloading binaries dynamically based on TARGET* variables¹ or (2) building from source. Keeping the note for a package would be a last resort if neither option is feasible. ¹ https://docs.docker.com/engine/reference/builder/#automatic-platform-args-in-the-global-scope
258050a
to
cd87a3a
Compare
Planning to merge after CI passes and I successfully re-run through manual testing with the updated test image. |
Update: the push-triggered CI failed on the build job with a seemingly transient network error. It's been >3 hours and still running. PR-triggered CI took >4 hours on
I believe this is because Node.js 14 only came in My opinions:
Based on the above points, my planned approach: |
Nextclade/Nextalign v2 comes with pre-built binaries for multiple platforms. Use TARGETPLATFORM¹ to construct URLs for each OS+arch combination. Put logic in a script to reduce Dockerfile hacking and maintain readability. ¹ https://docs.docker.com/engine/reference/builder/#automatic-platform-args-in-the-global-scope
For pre-built binary downloads, previous commits used platform-specific downloads when available. Otherwise, they must either (1) use emulation or (2) be built from source on the target platform for optimal performance. For now, I'll go with (1) since this linux/arm64 support is targeted at Apple silicon users which have easy access to amd64 emulation. (2) comes with the cost of extra dev time to figure out building for each program, plus additional build time that should be considered on a case-by-case basis.
This Augur dependency has pre-built binaries for amd64, but does not for arm64. Building requires additional system deps and a special environment variable when built via `pip install`.
This is prepping for the option of building with multiple platforms in a future commit, while keeping the option to build with a single platform locally.
Previous commits enable this to work. Emulation is necessary to build arm64 on an amd64 host machine¹. ¹ https://github.com/docker/build-push-action/blob/965c6a410d446a30e95d35052c67d6eded60dad6/docs/advanced/multi-platform.md
Add extra notes to encourage platform-aware changes.
Node.js v16 is very slow when running in an arm64 Docker build on an amd64 build platform. Node.js v14 runs in the build without emulation. It uses emulation in the Docker container during run time, but the slowness there is not nearly as significant as the build time increase due to emulation. This should be reverted if/when cross-compilation is used, i.e. Node.js v16 can run natively on the build platform while installing packages for a different target platform. This reverts commit 1afd07d.
8cebb0d
to
3b53f7a
Compare
@victorlin Sounds like a good plan. From the log snippet, it seems we're running
twice? Once as part of |
Description of proposed changes
Currently, the Docker images are built for
linux/amd64
only. These changes build the Docker images for bothlinux/amd64
andlinux/arm64
.This will substantially improve the performance of the Docker runtime for Apple silicon users, as it no longer needs to use slow Intel architecture emulation. A major drawback is the increased build time since building the
linux/arm64
image on alinux/amd64
GitHub Actions runner requires emulation. However, IMO the benefit to users outweighs the increased build-time on our end, which can be improved in the future.Cross-compilation (#98) is a major build-time improvement on top of this PR to be considered separately.
Changes made outside of PR
docker/setup-qemu-action@*
to allowed third party actions under @nextstrainRelated issue(s)
arm64
to image builds #48Testing
test linux/arm64 image without emulation (all my M1 Macs have Rosetta enabled, and it isn't easy to disable.Not doing this since this feature is targeted towards other Apple silicon users which should also have Rosetta enabled.Summary of comment threads
arm64
build