Stream: git-wasmtime

Topic: wasmtime / PR #8612 CI: shard testing crates across multi...

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 01:40):

fitzgen requested alexcrichton for a review on PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 01:40):

fitzgen requested wasmtime-default-reviewers for a review on PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 01:40):

fitzgen opened PR #8612 from fitzgen:ci-shard-tests to bytecodealliance:main:

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 01:52):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 02:03):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 02:19):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 02:22):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 02:33):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 02:48):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 02:49):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 03:14):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 03:15):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 03:26):

fitzgen commented on PR #8612:

Successful default PR run in ~8 minutes now: https://github.com/bytecodealliance/wasmtime/actions/runs/9073195225

Going to do a full run in a second.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 03:27):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 04:29):

fitzgen commented on PR #8612:

Unfortunately, it looks like cargo nextest's CI partitioning isn't exposed/supported by cargo miri.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 04:29):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 05:14):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 05:14):

fitzgen requested abrown for a review on PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 05:14):

fitzgen requested wasmtime-core-reviewers for a review on PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 05:14):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 05:21):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 06:01):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 07:20):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 07:38):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 07:43):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 08:01):

fitzgen commented on PR #8612:

Successful full test run in 14 minutes: https://github.com/bytecodealliance/wasmtime/actions/runs/9075621159?pr=8612

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 10:44):

github-actions[bot] commented on PR #8612:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 16:14):

alexcrichton commented on PR #8612:

Thanks for working on this! I'm personally a bit worried about the complexity creep here. Our CI is already pretty nontrivial and adding this into the mix I feel like takes it up to another level of complexity which makes it harder for future contributors/modifications. I can't argue with the results of course, but the merge queue currently reports that the average time-to-merge is 19 minutes so this whould shave 5 minutes off merge time. Given that we generally have an empty queue and we don't have many time-pressured PRs I'm not sure whether the 25% reduction in CI will be worth the complexity increase here.

I'll admit though I'm pretty biased as I'm still sort of used to the 3-hour-cycle-time from rust-lang/rust so my perception of what's appropriate for CI is likely pretty skewed. I'm curious how others feel as well as to the tradeoff of CI complexity here vs the time reduction.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 16:17):

abrown submitted PR review:

I think this is great though I didn't take enough time to understand the sharding in ci/build-test-matrix.js; walking through a short example in the doc comment might help because it all looks a bit complex. But overall speeding up CI is great; it looks like this would bring it down from 20+ minutes to ~14 minutes. @alexcrichton, any other comments?

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 16:27):

fitzgen commented on PR #8612:

Yeah I can definitely add more docs/comments if we want to continue down this route.

I think the default PR check time dropping from ~15 minutes to ~8 minutes is the real benefit here; more so than the 20 to 14 for full CI, at least IMO.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 16:29):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 16:29):

fitzgen created PR review comment:

I think this job is worth splitting out regardless if we want the rest of this PR. This is an extra 4-5 minutes at the end of a full ci/run-tests.sh on main.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 20:54):

alexcrichton submitted PR review:

Ok I've added some thoughts below but if you could add some more documentation specifically to sharding I think that'd be good.

I'll also point out that this is quite a lot of sharding, more than I've ever seen done before on github actions, so while I have no reason to think things will go wrong there's always the possibility we uncover something strange in the future. Most projects I don't think approach anywhere near this concurrency. The micro_checks job is also very heavily sharded where commands that might take ~5 seconds now take ~1 minute due to the overhead of a runner, downloading rustc, downloading deps, etc. That's not going to break the bank with our concurrency limit though so I don't have any reason to not do this.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 20:54):

alexcrichton submitted PR review:

Ok I've added some thoughts below but if you could add some more documentation specifically to sharding I think that'd be good.

I'll also point out that this is quite a lot of sharding, more than I've ever seen done before on github actions, so while I have no reason to think things will go wrong there's always the possibility we uncover something strange in the future. Most projects I don't think approach anywhere near this concurrency. The micro_checks job is also very heavily sharded where commands that might take ~5 seconds now take ~1 minute due to the overhead of a runner, downloading rustc, downloading deps, etc. That's not going to break the bank with our concurrency limit though so I don't have any reason to not do this.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 20:54):

alexcrichton created PR review comment:

I think it might be best to simplify this and replace it with --all-features. Most of the above features are on-by-default but it's also not an exhaustive list of off-by-default features that should be tested for crates. For example wasmtime-environ should have the compile feature enabled when testing, which it is today due to feature unioning but if it's split to its own job then it's probably best to slap --all-features and call it a day here.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 20:54):

alexcrichton created PR review comment:

Technically-speaking rather than using a regex for this it's probably best to build a map based on the output of cargo metadata and lookup things bad on id to project out the package name from the package metadata in the json blob. (probably more robust than regex-matching)

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 20:54):

alexcrichton created PR review comment:

AFAIK the extra_features bits here are exclusively for wasi-nn and ONNX stuff so I feel like we're carrying quite a lot of stuff in CI configuration just for that. At that point it might be best to just split out that build/test entirely into its own job? That'd help remove all the special casing around extra_features here

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 20:54):

alexcrichton created PR review comment:

This I think can be removed as it's only used by the one below.

view this post on Zulip Wasmtime GitHub notifications bot (May 14 2024 at 20:54):

alexcrichton created PR review comment:

This logic may not be quite so true any more given the more matrix entries we have, could the item.isa item be required here to basically only run the isa-based builders on runtest modifications? (e.g. not msrv)

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2024 at 17:23):

fitzgen commented on PR #8612:

I'll also point out that this is quite a lot of sharding, more than I've ever seen done before on github actions, so while I have no reason to think things will go wrong there's always the possibility we uncover something strange in the future. Most projects I don't think approach anywhere near this concurrency. The micro_checks job is also very heavily sharded where commands that might take ~5 seconds now take ~1 minute due to the overhead of a runner, downloading rustc, downloading deps, etc. That's not going to break the bank with our concurrency limit though so I don't have any reason to not do this.

Yeah I actually wanted to group that CI job into buckets of ~10 checks each or something but I couldn't figure out an easy way to do that ¯\\\_(ツ)\_/¯

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2024 at 23:23):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2024 at 23:24):

fitzgen commented on PR #8612:

Okay I think I figured out how to avoid having a billion check jobs

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2024 at 23:24):

fitzgen requested alexcrichton for a review on PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2024 at 23:30):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2024 at 23:32):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2024 at 23:35):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2024 at 23:40):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 00:08):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 00:08):

fitzgen requested wasmtime-fuzz-reviewers for a review on PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 00:09):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 00:20):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 00:27):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 00:38):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 00:52):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 01:05):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 01:06):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 01:12):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 03:44):

github-actions[bot] commented on PR #8612:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 14:58):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 14:58):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 14:58):

alexcrichton created PR review comment:

Do you think it's worth decreasing the size of the matrix here? This feels pretty intensive for something that isn't that platform-specific per-se. Maybe only doing linux/windows here or just linux?

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 15:58):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 15:58):

fitzgen created PR review comment:

I was just copying what existed before via tagging along on the main tests, but I'm fine cutting this down to linux and windows.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 16:07):

fitzgen updated PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 16:08):

fitzgen has enabled auto merge for PR #8612.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2024 at 16:38):

fitzgen merged PR #8612.

Last updated: Dec 23 2024 at 12:05 UTC