improve caching #1005

jrhizor · 2020-11-17T19:09:23Z

This PR separates caching into separate steps that correspond to different keys. I go into detail on this in comments on the PR.

I tried different methods from https://dev.to/dtinth/caching-docker-builds-in-github-actions-which-approach-is-the-fastest-a-research-18ei. In our case, we have so many large images that some methods (like the save/load) simply take up more disk space than is allowed on Github Actions. What I have here looks to me like the best balance, especially for split builds.

This doesn't speed up the CI build as much as I would like. The Build step is cut almost in half, but there's more overhead and it doesn't catch every case. The biggest benefit is that local builds are sped up significantly. Once the builds are split we'll have a much faster iteration cycle on CI too (lower overhead and fewer edge cases to target fixing).

Latest successful build: https://github.com/airbytehq/airbyte/runs/1426297335 / https://scans.gradle.com/s/4vlk24ao7da3m

jrhizor · 2020-11-19T19:04:42Z

.github/workflows/gradle.yml

-          key: gradle-${{ hashFiles('**/*.gradle') }}
+          path: |
+            /tmp/docker-registry
+          key: docker-${{ runner.os }}-${{ hashFiles('Dockerfile') }}-${{ github.sha }}
          restore-keys: |


docker can (and should) use restore-keys since some of the layers will match even if the contents change

jrhizor · 2020-11-19T19:05:15Z

.github/workflows/gradle.yml

-          key: gradle-${{ hashFiles('**/*.gradle') }}
+          path: |
+            /tmp/docker-registry
+          key: docker-${{ runner.os }}-${{ hashFiles('Dockerfile') }}-${{ github.sha }}


I'm using Dockerfile hashes even though the contents of the image might change, because this is only used to speed up builds, not to decide if it should skip a Docker build.

is github.sha intended? wouldn't this basically be discarded on every commit?

Yes, it is intended to store the cache entries with different keys. However, restore-keys allows it to read from different keys. It makes it easier to benchmark re-running builds for Docker with almost no downside (except maybe more cache evictions since it isn't overwriting keys as much).

jrhizor · 2020-11-19T19:05:45Z

.github/workflows/gradle.yml

-          path: ~/.npm
-          key: node-${{ hashFiles('**/package-lock.json') }}
+          path: |
+            ~/.cache/pip


Depending on how you use pip, more might be stored here. Right now this is super lightweight. It also allows restore-keys because this can be used by all venvs.

jrhizor · 2020-11-19T19:06:41Z

.github/workflows/gradle.yml

+        with:
+          path: |
+            ~/.npm
+            **/node_modules


It's somewhat debatable that we should have node_modules in a place that uses restore-keys, but it seems like a much smaller issue than python venvs or gradle caching.

isn't ~/.npm effectively the cache where node_modules is first populated from? does it make a difference to include node_modules directly?

You're right, it isn't recommended. I'll remove.

jrhizor · 2020-11-19T19:08:10Z

.github/workflows/gradle.yml

+        uses: actions/cache@v2
+        with:
+          path: |
+            ~/.gradle/caches


The Github Action cache examples prefer to cache the specific directories for gradle instead of the entire ~/.gradle.

This intentionally does not use restore-key.

add comment saying this "This intentionally does not use restore-key."?

jrhizor · 2020-11-19T19:08:47Z

.github/workflows/gradle.yml

+      - name: Check documentation
+        if: success() && github.ref == 'refs/heads/master'
+        run: ./tools/site/link_checker.sh check_docs
+
      - name: Write Integration Test Credentials


I moved this block down to make sure that ./gradlew build doesn't accidentally include a step that depends on credentials.

jrhizor · 2020-11-19T19:08:56Z

airbyte-integrations/connectors/source-mssql/build.gradle

@@ -20,17 +20,15 @@ dependencies {

    // on rc version due to docker pull bug in current stable version.
    // issue: https://github.com/airbytehq/airbyte/issues/493
-    testCompile "org.testcontainers:mssqlserver:1.15.0-rc2"
+    testImplementation "org.testcontainers:mssqlserver:1.15.0-rc2"


compile is deprecated

jrhizor · 2020-11-19T19:10:20Z

tools/bin/build_image.sh

+    --iidfile "$ID_FILE"
+else
+  # run build with local docker registery for CI
+  docker pull localhost:5000/"$TAG" || true


Storing in a local repository with a custom data dir (which is also started by Github Actions) seems like the best balance between build speed and cache speed.

sherifnada · 2020-11-19T19:56:30Z

.github/workflows/gradle.yml

-          key: gradle-${{ hashFiles('**/*.gradle') }}
+          path: |
+            /tmp/docker-registry
+          key: docker-${{ runner.os }}-${{ hashFiles('Dockerfile') }}-${{ github.sha }}


is github.sha intended? wouldn't this basically be discarded on every commit?

sherifnada · 2020-11-19T20:00:21Z

tools/bin/build_image.sh

+    --iidfile "$ID_FILE"
+else
+  # run build with local docker registery for CI
+  docker pull localhost:5000/"$TAG" || true


perhaps a silly question but shouldn't this also include the image name?

here "$TAG" is the tagged image name.

renamed so it's more clear

sherifnada · 2020-11-19T20:03:57Z

.github/workflows/gradle.yml

+        with:
+          path: |
+            ~/.npm
+            **/node_modules


isn't ~/.npm effectively the cache where node_modules is first populated from? does it make a difference to include node_modules directly?

cgardens · 2020-11-19T20:31:39Z

.github/workflows/gradle.yml

@@ -43,6 +64,19 @@ jobs:
        with:
          python-version: '3.7'

+      - name: Start local Docker registry


how does this interact with the docker cache? the idea is we get access to the Docker Caching above? i'm not sure i full understand how these two work together.

This uses /tmp/docker-registry as the data directory for a local registry. When build_image.sh pushes data to the local registry, this is what's being cached in Docker Caching (which creates a post-step at the end which saves the contents).

cgardens · 2020-11-19T20:32:38Z

buildSrc/src/main/groovy/airbyte-python.gradle

@@ -45,11 +45,17 @@ class AirbytePythonPlugin implements Plugin<Project> {
        project.task('installReqs', type: PythonTask) {
            module = "pip"
            command = "install -r requirements.txt"
+            inputs.file('requirements.txt')
+            outputs.file('build/installedreqs.txt')
+            outputs.cacheIf { true }


what's the implication of this? it just always caches its output?

This tells it that it's cacheable, but it still needs to re-run if the inputs change.

cgardens · 2020-11-19T20:34:22Z

In general I would be in favor of your putting your inline github comments into the code as actual comments.

jrhizor added 30 commits November 17, 2020 11:07

python caching

8ee8b7e

improve gradle caching

0935798

store pip cache dir

8ba3b92

debugging info

76001e7

split build steps and temporarily remove testing infra from build

4130412

fix formatting error

1629280

fix another syntax error

aabe54d

split repo caches

af74819

switch pip dir

f175856

use git hash instead of rehashing everything

215f47b

syntax

2b58a96

list directories

81a1341

list after cache population

af805ec

list .docker

244f379

specify custom output cache directory for docker

f74b1b9

only build bigquery and use buildx on ci

3b61123

add build command

486540f

use docker driver instead of docker-container

b20b554

use correct key

49f0034

use local registry

7d0b938

allow push failure

103ad7e

make sure to tag first

ebd2a5f

move part of node caching and restore rest of actual build

99ce830

cleanup

25eb722

Merge branch 'master' into jrhizor/python-caching

2ed835a

clean up mssql gradle file

c505225

toggle on inline cache

0a5f588

try save and load method instead

9da8693

use separate tar for each image

d5424ab

limit to 20

defe36b

jrhizor added 6 commits November 18, 2020 19:27

always succeed even though disk space runs out

4fa24f7

try to just pull the public image to see if that's faster

c57b12b

use cachefrom

379b63f

revert back to local registry version

10996a3

Merge branch 'master' into jrhizor/python-caching

a9062d4

see how buildkit compares

fa1d783

jrhizor changed the title ~~python caching~~ improve caching Nov 19, 2020

jrhizor commented Nov 19, 2020

View reviewed changes

jrhizor requested review from michel-tricot, cgardens and sherifnada November 19, 2020 19:12

jrhizor marked this pull request as ready for review November 19, 2020 19:12

sherifnada approved these changes Nov 19, 2020

View reviewed changes

address review comments

2b79bfb

cgardens approved these changes Nov 19, 2020

View reviewed changes

add comments

1206351

jrhizor merged commit 05404bd into master Nov 19, 2020

jrhizor deleted the jrhizor/python-caching branch November 19, 2020 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve caching #1005

improve caching #1005

jrhizor commented Nov 17, 2020 •

edited

Loading

jrhizor Nov 19, 2020

jrhizor Nov 19, 2020

sherifnada Nov 19, 2020

jrhizor Nov 19, 2020

jrhizor Nov 19, 2020

jrhizor Nov 19, 2020

sherifnada Nov 19, 2020

jrhizor Nov 19, 2020

jrhizor Nov 19, 2020

cgardens Nov 19, 2020

jrhizor Nov 19, 2020

jrhizor Nov 19, 2020

jrhizor Nov 19, 2020

sherifnada Nov 19, 2020

sherifnada Nov 19, 2020

jrhizor Nov 19, 2020

jrhizor Nov 19, 2020

sherifnada Nov 19, 2020

cgardens Nov 19, 2020

jrhizor Nov 19, 2020

cgardens Nov 19, 2020

jrhizor Nov 19, 2020

cgardens commented Nov 19, 2020

improve caching #1005

improve caching #1005

Conversation

jrhizor commented Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens commented Nov 19, 2020

jrhizor commented Nov 17, 2020 •

edited

Loading