Skip to content
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

Improve docker build performance #233

Merged
merged 3 commits into from
Apr 13, 2023
Merged

Improve docker build performance #233

merged 3 commits into from
Apr 13, 2023

Conversation

chowder
Copy link

@chowder chowder commented Apr 2, 2023

Description

  1. Use GitHub Actions cache to cache docker blobs so that images are incrementally updated instead of re-building from scratch.
  2. Add piwheels as an additional index to reduce the need to build wheels from source when building images for ARM.

Unfortunately it doesn't address the fact that we still need to build pandas from source (since piwheels don't provide wheels for Python 3.11), and that takes up majority of the build time.

See: #232

How Has This Been Tested?

The following two GH Actions runs on my fork are successful, with the second one taking only 15s for a no-op rebuild:

https://github.com/chowder/Twitch-Channel-Points-Miner-v2/actions/runs/4588562374
https://github.com/chowder/Twitch-Channel-Points-Miner-v2/actions/runs/4589448613

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md)
  • My changes generate no new warnings
  • Any dependent changes have been updated in requirements.txt

@chowder
Copy link
Author

chowder commented Apr 2, 2023

For hermeticity we should also be pinning the dependency versions in requirements.txt

@rdavydov
Copy link
Owner

rdavydov commented Apr 3, 2023

2. Add piwheels as an additional index to reduce the need to build wheels from source when building images for ARM.

Unfortunately it doesn't address the fact that we still need to build pandas from source (since piwheels don't provide wheels for Python 3.11), and that takes up majority of the build time.

pandas and numpy are the only ones that are building from sources, so this part is not really necessary.

Regarding caching - awesome, thanks!

@rdavydov rdavydov self-requested a review April 3, 2023 16:46
@rdavydov rdavydov self-assigned this Apr 3, 2023
@rdavydov rdavydov added the 🧱 enhancement New feature or request label Apr 3, 2023
@rdavydov
Copy link
Owner

rdavydov commented Apr 4, 2023

@chowder ^

Dockerfile Outdated
@@ -26,7 +26,7 @@ RUN DEBIAN_FRONTEND=noninteractive apt-get install -qq -y --fix-missing --no-ins
&& if [ "${BUILDX_QEMU_ENV}" = "true" ] && [ "$(getconf LONG_BIT)" = "32" ]; then \
pip install -U cryptography==3.3.2; \
fi \
&& pip install -r requirements.txt \
&& pip install -r requirements.txt --extra-index-url=https://www.piwheels.org/simple \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chowder Won't it affect other platforms: AMD64 and ARM64? https://www.piwheels.org/simple/pandas/ only has ARMv6 and ARMv7 packages.

@rdavydov
Copy link
Owner

OK, let's test this.

@rdavydov rdavydov merged commit 4dd1f7a into rdavydov:master Apr 13, 2023
@silentguy256
Copy link

@chowder Hm, is it possible that you change from "push: true" to "push: false" was only for debugging? Because currently I don't see the container even though it was build successfully

@rdavydov from that he said in my FR it should not have affected the other platforms because he used --extra-index-url instead of --index-url

@rdavydov
Copy link
Owner

@silentguy256 There is no need in piwheels.

pandas and numpy are the only ones that are building from sources, so this part is not really necessary.

And now we have caching, so let's just leave it without piwheels.

Hm, is it possible that you change from "push: true" to "push: false" was only for debugging? Because currently I don't see the container even though it was build successfully

Whoops, that's right. Need to change that and test caching.

@rdavydov
Copy link
Owner

@silentguy256 Caching works, ~20s! Containers are there now. Thanks for the heads up!

lyw1217 pushed a commit to lyw1217/Twitch-Channel-Points-Miner-v2 that referenced this pull request Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧱 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants