Grab Bag of CI Enhancements #284
Conversation
This first set of changes includes the following:
- Removes outdated github actions recipes that are no
longer functioning.
- Updates the benchmark flow to complete much more quickly by:
1. Don't try to run 10 processes concurrently; this seems to be
what previously caused some benchmarks to run for hours.
2. Just do 1 iteration (for PR flow); nightly runs will be added
later to get more complete results.
3. Partition the test suite to run in 8 parallel jobs.
- Build wasmtime main and compare with version 25 (chosen
arbitrarily) for test runs. I suspect this will be a discussion
point but this was somewhere to start. Matrixing in other versions
is very straightforward.
abrown
left a comment
abrown
left a comment
There was a problem hiding this comment.
Thanks for the PR! I had some comments below, but am interested to hear where you're headed — any new use cases for Sightglass?
.github/workflows/benchmarks.yml
Outdated
| # - uses: actions/checkout@v4 | ||
| # - run: rustup update | ||
| # - name: Rebuild benchmarks | ||
| # run: benchmarks/build-all.sh |
There was a problem hiding this comment.
We probably still want to retain the check that ensures we can reproduce the building of the benchmarks. Was this check failing?
There was a problem hiding this comment.
Ah, meant to revisit this so thank you for commenting. I understand its inclusion now; I was a bit thrown off by the rebuild being performed but the results being discarded previously and wasn't sure what the intention was.
No, wasn't failing from what I recall on my testing runs. It was the long poll for CI completion times after the other optimizations IIRC, though not by too much of a margin.
.github/workflows/benchmarks.yml
Outdated
| lines_per_chunk=$(ceil_div "$line_count" "$PARTITIONS") | ||
| split -d -l "$lines_per_chunk" "$tmpdir/all.suite" "${tmpdir}/all.split." | ||
| cp "$tmpdir/all.split.${{ matrix.partition }}" benchmarks/split.${{ matrix.partition }}.suite | ||
| rm -rf "$tmpdir" |
There was a problem hiding this comment.
Seems like this parallelization logic could go in a script to be able to reproduce locally?
There was a problem hiding this comment.
Yeah, I can extract this into a script. Apart from testing for CI changes, I don't think it would provide anything meaningful locally, but it could be useful for developers who need to make changes.
.github/workflows/benchmarks.yml
Outdated
| --engine=artifacts/wasmtime-v25.0.0/libengine.so \ | ||
| --processes=4 \ | ||
| --iterations-per-process=1 \ | ||
| -- "benchmarks/split.${{ matrix.partition }}.suite" |
There was a problem hiding this comment.
The run.sh script chooses slightly different parameters here. I'm not sure if it matters which parameters we use but I think making it easy to reproduce locally would be of some benefit.
There was a problem hiding this comment.
Yeah, part of the issue with CI is conflicting defaults between run-all.sh and what we want for different CI flows. These could be made configurable via more env vars, etc. on the .sh script; my reasoning for putting the invocation inline had to do with the somewhat coupled steps taken to do the parallel test execution (creating the splits in a previous job and then running the partition based on matrixing) that would probably muddy up the script as none of that job distribution logic would make sense outside of a CI context.
We also build wasmtime(s) in an earlier stage; run-all.sh is convenient but overloaded for achieving better CI performance.
For processes/iteration-per-process, I went with a combo which gives us about the same amount of data but at reduced execution time. What I plan to implement in later changes is a workflow that can run on a scheduled basis (nightly or otherwise) that will be longer-running and generate data which acts as a better reference, possibly working towards generating something publishable to gh-pages for easy reference; if results are consistent enough across runs (not optimistic on gh runners providing that), that could be extended to create something akin to https://arewefastyet.com.
There was a problem hiding this comment.
Ok, I get it. I like the idea of making it easy to reproduce results locally without having to copy around gobs of code but, the more I think about it, this could just be a usability problem with the CLI tool itself — it's probably better to just make that easier to use. So feel free to remove any scripts we don't need any more for CI!
abrown
left a comment
abrown
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks for fixing some of those broken CI bits. I'll leave this open for a day or so in case you were planning to re-organize your commits (I see a fixup! in there) but, honestly, I will squash-merge this anyways so it's up to you.
|
@abrown I'm happy enough with the changes here for now, so I would say squash-and-merge away is good with me whenever. I'm sure we'll revisit some of the details here in future changes, but this at least gets CI going again in some form. |
2 participants
This first set of changes includes the following: