-
Notifications
You must be signed in to change notification settings - Fork 14
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
Arm64 support #372
Arm64 support #372
Conversation
ca46d54
to
23138b7
Compare
c7d6c02
to
a72ac82
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.
@unkcpz thanks for all your thankless work on this! I've tested it on a M2 Mac machine and it works great.
I mostly just have some clarifying questions, nothing big. I would only suggest we wait till the mamba-bash-completion package becomes available, which should hopefully not be long.
Just as a side note, I found out that I won't be able to run my app on Arm. My app relies on ORCA for ab initio calculations. While ORCA team provides binaries for Mac Arm64, what we actually need here are Linux/arm64 binaries so that they can run inside our Ubuntu container. I didn't quite realized this before. It is a bit unfortunate, I'll see if the ORCA folks would be willing to release a new binary (though our use case seems to be quite niche).
tests/conftest.py
Outdated
@@ -16,7 +16,7 @@ def is_responsive(url): | |||
return False | |||
|
|||
|
|||
@pytest.fixture(scope="session", params=["lab", "base-with-services"]) | |||
@pytest.fixture(scope="session", params=["lab", "base-with-services", "full-stack"]) |
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.
Why did you add the full-stack
variant here? What does this bring in terms of testing?
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.
If you look at stack/full-stack/Dockerfile you'll find I have re-install erlang
even though it is already installed in the base-with-services
. The reason is that we use the multiple stage and packages installed in the preliminary stage will not transfer to the final image. This test is will fail if erlang
not installed explicitly in full-stack
.
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.
Right, fair enough. I guess my question then is whether there is enough reason to test the "lab" image separately, or whether just testing base-with-services
and full-stack
is enough (which would also allow us to get rid of bunch of extra skipping logic in some tests. But that can be left for another PR.
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.
whether just testing base-with-services and full-stack is enough
I am second on this. I didn't see the point to test lab
and full-stack
. Can you open an issue and assign it to me and you? Should be a quick fix I assume.
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.
I tried to remove the test of base-with-services
in #385. The lab is worth testing because we want to be sure strating with docker-compose is working.
d7794a1
to
7d345de
Compare
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci fix gha test
Remove jsonschema check for CI workflow
for more information, see https://pre-commit.ci
Hey @unkcpz are there any remaining blockers here in terms of all dependencies? Is the QeApp actually running on this Arm image? If yes, maybe @mbercx and @cpignedoli can do final round of testing and we're good to go? |
Is not, but nothing can be done here. What I need to do is update the qe installed from conda-forge to 7.2 which starts to have arm64 supported.
Maybe @cpignedoli can test on Empa apps? I have apple M1 laptop as well so I can give it a test for QeApp. I plan to release a new version with the "daemon stop" fix. Let's see how fast I can make a QE update for the app. |
Running a test on my M1 chip laptop with the QE from conda-forge, and all is good aiidalab/aiidalab-qe#404 🎊 |
@unkcpz I will test everything hopefully by tonight. |
I had no problems with my tests so from my side, despite I did not go through the files, I am OK with merging. |
Not in the amd64 machine. Is it the @cpignedoli could you open an issue for it with one screenshot? Thanks! |
The goal of this change is twofold: - Improve CI runtimes (currently the overall workflow takes ~1h40min!) - Reduce complexity I ended up reverting things back to where they were roughly around PR #372, while (hopefully) keeping the current modularity. See more details in #439 The workflow now takes ~20 minutes. Major changes: - Don't upload images as artifacts, upload to ghcr.io straightaway. (this however does not support PRs from forks) - The arm64 image is build on Ubuntu GHA runner with the help of QEMU virtualization. - Build and test on amd64 first to catch issues within first 6 minutes - Only test full-stack for arm64
doit build --target base --target full-stack
mamba-bash-completion
package for arm arch