Stream: git-wasmtime

Topic: wasmtime / issue #4421 Adds a github action to support x6...

Wasmtime GitHub notifications bot (Jul 09 2022 at 03:24):

jlb6740 commented on issue #4421:

This patch is a replacement for #3740

Results currently look like this: https://github.com/jlb6740/wasmtime/pull/3#issuecomment-1179394254

Wasmtime GitHub notifications bot (Jul 11 2022 at 17:05):

alexcrichton commented on issue #4421:

This is looking quite nice! Would it be possible to add bytecodealliance/wasmtime-sightglass-benchmarking/.github/workflows/main.yml to this repository as well? I think it'd be ideal if we could have everything in this repository if we can.

Instead of a curl to send a repository dispatch to the private repository I'd imagine that we could instead push to a branch? For example the commit that wants to be benchmarked could be checked out as part of this workflow and then pushed to a special branch name in the private repository. The "actually run the benchmark" perf run would then trigger on this special branch name and do everything it's already doing today.

Review-wise I think it'd be best to be able to edit the workflow publicly in this repository rather than juggling hidden state elsewhere.

Wasmtime GitHub notifications bot (Jul 12 2022 at 21:40):

jlb6740 commented on issue #4421:

This is looking quite nice! Would it be possible to add bytecodealliance/wasmtime-sightglass-benchmarking/.github/workflows/main.yml to this repository as well? I think it'd be ideal if we could have everything in this repository if we can.

I would definitely like that as well, but how is it possible?

Instead of a curl to send a repository dispatch to the private repository I'd imagine that we could instead push to a branch? For example the commit that wants to be benchmarked could be checked out as part of this workflow and then pushed to a special branch name in the private repository. The "actually run the benchmark" perf run would then trigger on this special branch name and do everything it's already doing today.

@alexcrichton Would you like to see this refactor before merging this patch? Currently I am not sure how what you describe would work. The yml file that executes commands to download the correct sightglass, mainline, and patch, I would think needs to be associated with the private repo. That self-hosted runner executes the run steps in the github action yml so that needs to be part of the private repo's github actions. By putting logic here to push a patch to the private server I don't see yet what that really accomplishes. It introduces a patch on the private server but for what purpose? Currently the private server (that gets it's run steps from the yml) downloads the patch. The patch is never duplicated on any other branch, it just lives at its home of its original feature branch. Also, I am not sure how that would work when multiple requests at the same time? In short, I don't yet see a way to accomplish the interesting steps in this repository alone. My thoughts are that while I completely agree from a review standpoint that it would be best to have all logic in one place, the private repository is that place and can be checked and has it's own review process. I don't think we have to sacrifice correctness. Just treat it like any other API .. when there is an issue just have to know which side to make the changes. From the wasmtime perspective, the curl statement sends a request to the private repository where the private repository is now a black box. If we want to scrutinize or update the logic the black box is doing, we just have to do it there.

Review-wise I think it'd be best to be able to edit the workflow publicly in this repository rather than juggling hidden state elsewhere.

Again, I do agree.

Wasmtime GitHub notifications bot (Jul 13 2022 at 15:18):

alexcrichton commented on issue #4421:

I would envision adding the workflow to this repository as:

• The on: condition is only for branches that are named perf-test-*. This means it won't run for the main bytecodealliance repo
• The workflow added to this repo would run on a custom runner. The custom runner is only added to the private repo. This is ok since it won't actually run in the public repo.
• Otherwise the workflow in the private repo is basically the same, it checks out the branch whenever a push happens to perf-test-* and it could post results back to the PR when done.

Otherwise from this action added here the main difference would be instead of a curl a git push is done to a perf-test-* branch in the private repository.

Would you like to see this refactor before merging this patch?

Personally I would say yes. Having everything in one repo seems like a foundational shift relative to what you've currently got working which probably won't happen if we don't do it to start out with.

Also, I am not sure how that would work when multiple requests at the same time?

One idea would be for PR number N to push to perf-test-N so that way multiple PRs don't conflict.

the private repository is that place and can be checked and has it's own review process

The major downside, infrastructurally, is that you can't change atomically change the perf process in a PR to wasmtime. That means that the Wasmtime configuration to schedule a perf run must be kept in sync with the private repo's configuration. By having all the configuration in one place we can change everything at once in a single PR. Needing two PRs is not only cumbersome but runs the risk of breakage since the repos could get out of sync with each other.

Wasmtime GitHub notifications bot (Aug 19 2022 at 17:51):

jlb6740 commented on issue #4421:

@fitzgen @cfallin Hi guys .. had a good chat with @alexcrichton today and flushed out some concerns about the refactoring suggestion made. Its good to try the refactoring now. Since this is an anticipated patch, if there are any unanticipated roadblocks with the refactor we will look to merge this as is and address the refactor concern later, but we think the refactoring should work as expected. The idea is to have as much logic here as possible. Will start on that next week and hopefully have this ready for review the week after when @alexcrichton is back. I do anticipate that once the actions is updated there will be new comments and concerns to address before merger which is fine, so do anticipate that.

Wasmtime GitHub notifications bot (Aug 19 2022 at 17:56):

fitzgen commented on issue #4421:

SGTM, really excited to get this up and running soon!

Wasmtime GitHub notifications bot (Sep 14 2022 at 15:55):

jlb6740 commented on issue #4421:

@alexcrichton Note, this latest patch is intended to address previous comments.

Wasmtime GitHub notifications bot (Sep 15 2022 at 22:00):

jlb6740 commented on issue #4421:

Looks great to me, thanks again for your patience!

I've got some minor nits below but it's fine to defer any or all of them to later if you'd prefer. Do you need me to configure any tokens or such in the repo before/after merging to have this work?

@alexcrichton No thanks for your patience! I think the refactoring works as imagined and is definitely an improved workflow as it combines everything in one patch and reduces logic. @alexcrichton @fitzgen @abrown I think this is ready to merge and iterate on. The only question I have is are the tokens workable. I believe they are, but of course those can be updated too.

Wasmtime GitHub notifications bot (Sep 15 2022 at 23:24):

jlb6740 commented on issue #4421:

Example output is here: https://github.com/jlb6740/wasmtime/pull/10
Note, the comment trigger /bench_x64 needs to be posted via the "review" comment box. It is not triggered by simply leaving a comment here in the "issue".

Last updated: Nov 22 2024 at 17:03 UTC