Skip to content

Implement step-wise caching for batch spec execution#540

Merged
mrnugget merged 14 commits into
mainfrom
mrn+kr/step-wise-caching
May 26, 2021
Merged

Implement step-wise caching for batch spec execution#540
mrnugget merged 14 commits into
mainfrom
mrn+kr/step-wise-caching

Conversation

@mrnugget

@mrnugget mrnugget commented May 18, 2021

Copy link
Copy Markdown
Contributor

This adds step-wise caching for the execution of batch specs.

In short, given a batch spec that contains the following

on:
  - repository: github.com/sourcegraph/automation-testing
  - repository: github.com/sourcegraph-testing/mkcert

steps:
  - run: echo "this is step 1" >> caching.txt
    container: alpine:3
  - run: echo "this is step 2" >> README.md
    container: alpine:3
  - run: echo "this is step 3" >> README.md
    container: alpine:3
    outputs:
      myOutput:
        value: "what is up"
  - run: echo "this is step 4" >> caching.txt
    if: ${{ eq repository.name "github.com/sourcegraph/automation-testing" }}
    container: alpine:3

this PR would cause src to cache the results _per step and per repository.

In practice, this means that if we would change the batch spec from the above so it would look like this

steps:
  - run: echo "this is step 0" >> caching.txt
    container: alpine:3
  - run: echo "this is step 1" >> README.md
    container: alpine:3
    #                         vvvv LOOK HERE vvv
  - run: echo "this is step 2 WITH A CHANGE" >> README.md
    container: alpine:3
    outputs:
      myOutput:
        value: "what is up"
  - run: echo "this is step 3" >> caching.txt
    if: ${{ eq repository.name "github.com/sourcegraph/automation-testing" }}
    container: alpine:3

and we'd then re-execute the batch spec, the steps 0 and 1 would not need to be re-executed. Only step 2 would need to be re-executed for both repositories and step 3 would only need to be re-executed for both repositories.

Base automatically changed from mrn/refactor-statuses to main May 19, 2021 08:11
@mrnugget mrnugget force-pushed the mrn+kr/step-wise-caching branch from bc5d5e5 to e54bfd6 Compare May 19, 2021 13:25
@mrnugget mrnugget changed the title WIP: Step-wise caching. not really working WIP: Implement step-wise caching so that changes to single steps to not require rerunning all steps May 19, 2021
@mrnugget mrnugget changed the title WIP: Implement step-wise caching so that changes to single steps to not require rerunning all steps Implement step-wise caching for batch spec execution May 25, 2021
@mrnugget mrnugget marked this pull request as ready for review May 25, 2021 14:22
@mrnugget mrnugget requested a review from a team May 25, 2021 14:22

@LawnGnome LawnGnome left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

Comment thread internal/batches/executor/coordinator.go Outdated
Comment thread internal/batches/executor/coordinator_test.go Outdated
Comment thread internal/batches/executor/execution_cache.go Outdated
Comment thread internal/batches/executor/execution_cache.go
Comment thread internal/batches/executor/executor.go
Comment thread internal/batches/executor/run_steps.go
+++ new-file.txt
@@ -0,0 +1,2 @@
+check this out. this is a new file.
+written on a computer. what a blast.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💥

@LawnGnome LawnGnome left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(I swear I hit approved.)

@courier-new courier-new left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is SO COOL to see coming together!! Most of my comments here are just learning questions. 🙂

Comment thread internal/batches/executor/changeset_specs_test.go
Comment thread internal/batches/executor/coordinator.go Outdated
Comment thread internal/batches/executor/coordinator_test.go
Comment thread internal/batches/executor/coordinator_test.go Outdated
Comment thread internal/batches/executor/coordinator_test.go Outdated
Comment thread internal/batches/executor/execution_cache.go
Comment thread internal/batches/executor/execution_cache.go
Comment thread internal/batches/executor/execution_cache.go
Comment thread internal/batches/executor/execution_cache.go
Comment thread internal/batches/executor/run_steps.go Outdated
@mrnugget mrnugget merged commit e5f28d4 into main May 26, 2021
@mrnugget mrnugget deleted the mrn+kr/step-wise-caching branch May 26, 2021 09:53
mrnugget added a commit that referenced this pull request May 26, 2021
This is a tiny follow-up to #540 and removes the TODO that Adam
confirmed in his review.
mrnugget added a commit that referenced this pull request May 27, 2021
This is a tiny follow-up to #540 and removes the TODO that Adam
confirmed in his review.
scjohns pushed a commit that referenced this pull request Apr 24, 2023
This adds step-wise caching for the execution of batch specs.

In short, given a batch spec that contains the following

```yaml
on:
  - repository: github.com/sourcegraph/automation-testing
  - repository: github.com/sourcegraph-testing/mkcert

steps:
  - run: echo "this is step 1" >> caching.txt
    container: alpine:3
  - run: echo "this is step 2" >> README.md
    container: alpine:3
  - run: echo "this is step 3" >> README.md
    container: alpine:3
    outputs:
      myOutput:
        value: "what is up"
  - run: echo "this is step 4" >> caching.txt
    if: ${{ eq repository.name "github.com/sourcegraph/automation-testing" }}
    container: alpine:3
```

this PR would cause `src` to cache the results _per step and per repository.

In practice, this means that if we would change the batch spec from the above so it would look like this

```yaml
steps:
  - run: echo "this is step 0" >> caching.txt
    container: alpine:3
  - run: echo "this is step 1" >> README.md
    container: alpine:3
    #                         vvvv LOOK HERE vvv
  - run: echo "this is step 2 WITH A CHANGE" >> README.md
    container: alpine:3
    outputs:
      myOutput:
        value: "what is up"
  - run: echo "this is step 3" >> caching.txt
    if: ${{ eq repository.name "github.com/sourcegraph/automation-testing" }}
    container: alpine:3
```

and we'd then re-execute the batch spec, **the steps 0 and 1 would not need to be re-executed**. Only step 2 would need to be re-executed for both repositories and step 3 would only need to be re-executed for both repositories.
scjohns pushed a commit that referenced this pull request Apr 24, 2023
This is a tiny follow-up to #540 and removes the TODO that Adam
confirmed in his review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants