How to use `--store-durations` in Github Actions?

[michamos]

Let me first thank you for this great tool, which makes it super easy to decrease testing time in Github Actions.

I have some interrogations about how the new feature to combine --store-durations with --groups is supposed to be used to update test timings while running the splitted test suite during CI. I couldn't find any documentation, but I assume the idea is to do as suggested by @sondrelg in #11 (comment) and basically use the Github actions/cache to cache the .test_durations file.

I see two problems with that approach, both due to the fact that, as far as I understand, loading the cache is done at the beginning of the job, storing the cache at the end.

1. If there are several concurrent runs of the tests for different groups, they will read the same value of the file at the beginning if available (from the previous run), but they will try to overwrite the cache when they finish. The result is that the slowest group will have its durations persisted, because faster groups will have their durations overwritten almost immediately by slower ones. I believe this will cause all test durations from faster groups to be overestimated (as they will have no durations, so average test duration of the slowest group will be taken), so on average the other groups with unestimated tests will still finish faster, and it's not clear to me that over several runs, the durations will converge to the accurate values, as the slowest group will probably remain the slowest.
2. I don't think there's any guarantee that a job for a given group can't start after the job for a different group in the same run finishes. However, if that happens, the first job will have already updated the durations in the cache before the second one starts, causing a potentially different split into groups, which might cause some tests to run twice or not to run at all.

Unless I misunderstood how this whole thing works, I think the more robust approach would be to store the group durations as artifacts, and then have an additional job in the workflow which depends on the group runs that consolidates all the separate artifacts into one duration file and caches that. That would solve problem 1. as all group durations will be taken into account, and problem 2. as the caching will happen only after all test runs finish. The missing piece to implement such a strategy is a tool that can combine the test durations from the different groups, and a way to annotate in the .test_durations file whether a duration has been updated in the current run in order for the tool to know which durations need to be put into the combined file.

[sondrelg]

the more robust approach would be to store the group durations as artifacts, and then have an additional job in the workflow which depends on the group runs that consolidates all the separate artifacts into one duration file

That's exactly what I ended up doing 😄

Here's a functional example (what I use)

  test:
    ...
    steps:
      ...
      - run: pip install awscli

      - name: Download test-durations
        run: aws s3 cp s3://<bucket>/.test_durations .test_durations
        env:
          AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
          AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}

      - name: Run tests
        run: pytest --splits 8 --group ${{ matrix.group }} --store-durations

      - name: Upload partial durations
        uses: actions/upload-artifact@v1
        with:
          name: split-${{ matrix.group }}
          path: .test_durations

  upload-timings:
    needs: test
    steps:
      - uses: actions/checkout@v2
      - run: pip install awscli

      - name: Download artifacts
        uses: actions/download-artifact@v2

      # This will generate a single .test_durations file in the root dir
      - name: Combine test-durations
        run: python .github/scripts/combine_dict.py 8

      - name: Upload test-durations
        run: |
          aws s3 cp .test_durations s3://<bucket>/.test_durations
        env:
          AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
          AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}

The combine-dict script is just a simple dict merge:

if __name__ == '__main__':
    import json
    import sys

    splits = int(sys.argv[1])

    x = {}

    for split in range(1, splits + 1):
        with open(f'split-{split}/.test_durations') as f:
            x.update(json.load(f))

    with open('.test_durations', 'w') as f:
        json.dump(x, f)

Maybe you could try it out and add some documentation if you get it to work? 🙂

[sondrelg]

Just as a follow-up, I ended up using S3 since that was already available to me, but if you manage to get this to work with the cache itself that would be even better 👍

[michamos]

Thanks @sondrelg, good to know my suggestion seems to work and you did all the hard work already 🙂
It's not clear to me how you distinguish when combining the durations between durations of tests that have been run in the current group, and hence have been updated, from durations of tests that the group didn't select and were already present in the .test_durations file from the start. In other words, if there's already a .test_durations file present in the cache, won't you end up simply keeping the .test_durations file of the last group?

[sondrelg]

Yeah you're probably right 🤔 I hadn't considered that tbh.

Could we maybe pass the --store-durations arg to get it to output partial durations and upload those instead?

[mbkroese]

I also recognised the problem that you are flagging here.
In my case the groups run on different nodes, and they get their own CI_NODE_INDEX assigned automatically by Gitlab.
After the durations are computed I rename .test_durations for node i to .test_durations.i, and store that as artifact.
Then a subsequent job simply goes over all the json files and averages the values for the same key.

The downside of this approach is that the old values get a large weight.

Maybe a solution could be to change the meaning of the --store-durations flag a bit.
We can allow the following values: all (default), group.
So if users use --store-durations=group then a .test_durations file would be created with just the durations of that group.
If users just do --store-durations then we can interpret that as equivalent to --store-durations=all.

I would actually really like a feature like this, and when this gets introduced also immediately add the combine-durations functionality described here: #11 (comment)

[sondrelg]

Could the same thing maybe be achieved by specifying which file to read from, and which to write from @mbkroese?

Since we only write durations for the tests run, the output file (if you're not writing back to the same file as you read from) would contain only the new durations, right 🙂

I definitely wouldn't mind built-in combine-duration logic 👏

[mbkroese]

@sondrelg I think that could work - right now we update the data that we read before and write the result, but it could be changed to just create a new dictionary which we can write to the write location.

However, think this should be considered a breaking change, because in your scheme, if the read and write location are the same, we would overwrite our current durations with those of the group. Whereas in the current situation (where read and write location are already the same) we would just update the durations of tests we ran.

[sondrelg]

if the read and write location are the same, we would overwrite our current durations with those of the group. Whereas in the current situation (where read and write location are already the same) we would just update the durations of tests we ran.

Isn't this the same thing? Can you elaborate a little bit more on what's different in this case? 🙂

[mbkroese]

Suppose currently we have these durations:

{'a': 1, 'b': 2, 'c': 3}

And we run one group that only executes test 'a' with new duration 5, then (in the current implementation) we'll write:

{'a': 5, 'b': 2, 'c': 3}

Whereas in your proposal we'd be writing:

{ 'a': 5} 

right?

[sondrelg]

If we said pytest --input=.durations --output=.durations then we should end up with

{'a': 5, 'b': 2, 'c': 3}

But if we say pytest --input=.durations --output=.durations-${{ matrix.group }}

Then .durations should remain

{'a': 1, 'b': 2, 'c': 3}

And we would write { 'a': 5} to .durations-1 for matrix group 1.

So the current implementation would be equivalent of specifying the same file for input and output while breaking them up into two options (I think) should give us the flexibility of writing partial outputs.

Does that make sense or am I missing something? 🙂

[mbkroese]

I see what you mean now. I assumed you always just wanted to write the durations of tests we ran. But instead you want to update the durations at the write location of the tests we ran. Makes sense 👍

I think both proposed solutions are valid.

[sondrelg]

If --store-durations=group would just write group durations to the durations file, I guess that would also be equivalent for my use-case, so I don't mind how it's implemented.

@michamos or @jerry-git, do you have any pros/cons/preferences?

[michamos]

Actually I managed to make it work correctly afaict with the current implementation, by modifying @sondrelg's combining script to also take into account the previous version of the test durations to only update the changed values. I also managed to use the github cache to store the test durations across runs.

This is the (simplified) workflow I use:

 test:
    # additional config, like matrix omitted here
    steps:
      # test setup omitted here

      - name: Get durations from cache
        uses: actions/cache@v2
        with:
          path: test_durations
          # the key must never match, even when restarting workflows, as that
          # will cause durations to get out of sync between groups, the
          # combined durations will be loaded if available
          key: test-durations-split-${{ github.run_id }}-${{ github.run_number}}-${{ matrix.group }}
          restore-keys: |
            test-durations-combined-${{ github.sha }}
            test-durations-combined

      - name: Run tests
        run: pytest --splits 6 --group ${{ matrix.group }} --store-durations

      - name: Upload partial durations
        uses: actions/upload-artifact@v2
        with:
          name: split-${{ matrix.group }}
          path: .test_durations

  update_durations:
    name: Combine and update integration test durations
    runs-on: ubuntu-latest
    needs: test
    steps:
      - name: Checkout
        uses: actions/checkout@v2

      - name: Get durations from cache
        uses: actions/cache@v2
        with:
          path: .test_durations
          # key won't match during the first run for the given commit, but
          # restore-key will if there's a previous stored durations file, 
          # so cache will both be loaded and stored
          key: test-durations-combined-${{ github.sha }}
          restore-keys: test-durations-combined

      - name: Download artifacts
        uses: actions/download-artifact@v2

      - name: Combine test durations
        uses: ./.github/actions/combine-durations
        with:
          split-prefix: split-

The tricky point when using the actions/cache is that if a key matches in the cache, the cache will be loaded but not updated (docs). So we need to ensure a cache miss based on the key and use restore-keys instead for loading the cache.

For clarity, I've split combine-durations into its own action. This might become a github action into a separate repo. The action.yml contains

name: Combine durations
description: Combine pytest-split durations from multiple groups

inputs:
  durations-path:
    description: The path to the durations file (must match `--durations-path` arg to pytest)
    required: false
    default: .test_durations
  split-prefix:
    description: The path to the split durations (must match the artifacts name)
    required: true

runs:
  using: composite
  steps:
    - name: Combine durations
      shell: bash
      run: >
        python3 $GITHUB_ACTION_PATH/combine_durations.py ${{ inputs.split-prefix }} ${{ inputs.durations-path }}

and the combine_durations.py script is

import json
import sys
from pathlib import Path

split_prefix = sys.argv[1]
durations_path = Path(sys.argv[2])

split_paths = Path(".").glob(f"{split_prefix}*/{durations_path.name}")
try:
    previous_durations = json.loads(durations_path.read_text())
except FileNotFoundError:
    previous_durations = {}
new_durations = previous_durations.copy()

for path in split_paths:
    durations = json.loads(path.read_text())
    new_durations.update(
        {
            name: duration
            for (name, duration) in durations.items()
            if previous_durations.get(name) != duration
        }
    )

durations_path.parent.mkdir(parents=True, exist_ok=True)
durations_path.write_text(json.dumps(new_durations))

It would be good if (a less quick-and-dirty version of) this script became part of pytest-split as it depends on implementation details of the test durations storage format.

[michamos]

@jerry-git what do you think about adding a command to combine outputs directly to pytest-split? it could have similar logic to the script I posted in #20 (comment) and have an API that is slightly more general to accomodate different CI systems:

pytest-split-combine [--durations-path DURATIONS_PATH] SPLIT_DURATIONS_PATH...

the --durations-path would be optional and have the same meaning and default as in the pytest plugin, while the other non-optional arguments would be the paths to the spilt durations files. I believe it's better to be explicit here for a general purpose tool rather than do the globbing in the script.

[jerry-git]

Sounds great 👍

while the other non-optional arguments would be the paths to the spilt durations files

Does this mean that one would do something like
pytest-split-combine foo/split-1 foo/split-2 foo/split-3 ...?