Improve CI for wasmer_wamr feature #116

mattyg · 2024-08-30T17:54:00Z

Some additional cleanup I missed in #112

I discovered that windows runners in the holochain repo were failing to build these crates with the wasmer_wamr feature, then realized I hadn't updated the windows ci here to also test against that feature.

I am still unable to build with the wasmer_wamr feature on windows, but I have made some progress. For now I've commented out those CI jobs. See #117

I'm also not sure if this CI setup for running benchmarking makes sense. Do we want to run benchmarking on every platform and feature? If we're running cargo bench on a fresh runner every time, does it even have a prior run to compare to? Should it be comparing to develop for each PR?

…well, move into common workflow file

…windows runner

mattyg · 2024-08-30T17:54:40Z

.circleci/config.yml

This is not being used anymore and appears years outdated.

mattyg · 2024-08-30T17:55:12Z

.github/workflows/test.yml

        os: ["ubuntu-latest", "macos-latest"]
+        wasmer-feature: ["wasmer_sys", "wasmer_wamr"]


Separate ci jobs for each wasmer feature for ease of review.

mattyg · 2024-08-30T17:55:59Z

.github/workflows/test.yml

-      - run: nix develop --command ${{ matrix.script }}
+      - run: nix develop --command ./${{ matrix.script }}-${{ matrix.wasmer-feature }}.sh
+
+  test-windows:


~~Test for both wasmer features on windows~~

Move test-windows ci job into same file to make it more obvious that both jobs need to be modified together

mattyg · 2024-08-30T17:56:15Z

README.md

@@ -103,7 +103,7 @@ very reasonable overhead for the host to build a completely fresh instance.
 Calling a function with `holochain_wasmer_host::guest::call()` takes several `us`
 for small input/output values and some `ms` for ~1mb of input/output data.

-To see benchmarks on your system run `nix-shell --run ./bench.sh`.
+To see benchmarks on your system run `nix-shell --run ./scripts/bench.sh`.


Move all scripts into scripts dir

mattyg · 2024-08-30T18:23:32Z

.github/workflows/test.yml

@@ -6,13 +6,18 @@ on:
      - develop
  pull_request: {}

+concurrency: 


cancel superseded ci jobs

…explanatory comment

neonphog · 2024-09-06T17:27:04Z

I'm also not sure if this CI setup for running benchmarking makes sense. Do we want to run benchmarking on every platform and feature? If we're running cargo bench on a fresh runner every time, does it even have a prior run to compare to? Should it be comparing to develop for each PR?

Indeed we need to figure out our benchmarking strategy in general. Whether we're just visually inspecting, or setting some threshold failure value... whether we care about comparing changes, or just looking at spot absolute results.

But that doesn't have to be solved for this PR : )

neonphog

Nice, LGTM

jost-s

Good question about the benchmarking. There's no comparison to previous results taking place right now. I tend to think we only need benchmarks on one platform, being Linux.

jost-s · 2024-09-10T23:48:39Z

.github/workflows/test.yml

    runs-on: ${{ matrix.os }}
    steps:
+      - name: increase swap space
+        if: matrix.os == 'ubuntu-latest'
+        uses: actionhippie/swap-space@v1


In holochain there's an action for ubuntu to free up disk space - does that achieve what you need the swap space for?

No, the bench runs on CI were crashing when run in series due to running out of memory.

ThetaSinner · 2024-09-11T16:52:08Z

does it even have a prior run to compare to?

I was confused by this. The benchmarks are actually set up to do a number of iterations in a known amount of time. So the performance benchmark is how many iterations they can complete in that time. So if performance regresses then we'd catch it. That's not so useful for performance being improved and then harmed again, unless those tests are actively maintained.

ThetaSinner · 2024-09-11T16:53:43Z

.github/workflows/test.yml

@@ -6,15 +6,31 @@ on:
      - develop
  pull_request: {}

+concurrency: 
+  group: ${{ github.head_ref }}


This should include the pipeline, otherwise it's concurrency across all CI workflows that use the head ref - e.g. https://github.com/holochain/holochain/blob/develop/.github/workflows/test.yml#L10

That uses github.workflow to keep workflow concurrency separate

That or you can just use github.ref_name. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#github-context

Yes, but I think probably both? I think either head_ref or ref_name will conflict if you put them into two different workflows

updated -- I just copied the settings from the holochain workflow.

ThetaSinner · 2024-09-11T16:59:46Z

scripts/bench.sh

@@ -0,0 +1,4 @@
+#! /usr/bin/env bash
+
+./bench-wasmer_sys.sh && \


Any reason to && rather than just sequential commands?

I guess so that if the first command fails you see the failure output as the last text in the terminal rather than having it get buried by the output of the subsequent command. I don't have a strong preference either way.

ThetaSinner

Looks great! Really appreciate CI improvements being made on this repo, it's not had that much attention recently :)

mattyg · 2024-09-11T21:18:35Z

does it even have a prior run to compare to?

I was confused by this. The benchmarks are actually set up to do a number of iterations in a known amount of time. So the performance benchmark is how many iterations they can complete in that time. So if performance regresses then we'd catch it. That's not so useful for performance being improved and then harmed again, unless those tests are actively maintained.

First it runs a bunch of iterations to get a performance range, then that range is compared to saved results from the prior run if they are found in target/criterion/ ( see https://bheisler.github.io/criterion.rs/book/analysis.html#comparison ).

I'm thinking the ideal setup would have develop benchmark results saved, and then run the PR benchmark to compare against it.

mattyg added 7 commits August 29, 2024 14:18

log

c67bc39

ci: windows tests run both wasmer_wamr & wasmer_sys feature flags as …

368a8e2

…well, move into common workflow file

chore: delete unused circle ci workflow

2eb5669

fix: bench with wasmer_wamr feature

202eb30

refactor: split up testing scripts by wasmer feature

ef93147

ci: split up ci jobs by wasmer feature, test both wasmer features in …

1cb1ba7

…windows runner

doc: clarifying comment

f39f46c

mattyg changed the title ~~Fix/integration wasmer wamr~~ Some cleanup on wasmer/wamr integration Aug 30, 2024

mattyg commented Aug 30, 2024

View reviewed changes

.circleci/config.yml Outdated

Copy link

Member Author

mattyg Aug 30, 2024

Choose a reason for hiding this comment

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

This is not being used anymore and appears years outdated.

mattyg commented Aug 30, 2024

View reviewed changes

mattyg changed the title ~~Some cleanup on wasmer/wamr integration~~ WIP Some cleanup on wasmer/wamr integration Aug 30, 2024

mattyg marked this pull request as ready for review August 30, 2024 18:00

mattyg added 6 commits August 30, 2024 11:02

ci: yaml syntax

ac0cb24

fix: scripts path changed

7da05db

fix: rm windows target

f1f6960

fix: include error_as_host feature, only run static tests once

e8e1d7b

ci: cancel previous superceeded jobs

e4994bb

fix: remove feature

3a2c0e9

mattyg commented Aug 30, 2024

View reviewed changes

mattyg added 9 commits August 30, 2024 12:18

fix: missing error_as_host feature

63d9dd5

wip: switch to hc-wasmer fork with win32 fix

44a8af5

wip: only test win32 wasmer_wamr

4fddfc3

wip: try to get windows building in ci

bd0cec3

build: bump hc-wasmer

3494a1a

wip: try mingw instead

57315e0

wip: install newer cmake

5ee2e5d

wip: try msys

9052f8a

wip: try msys2

5d4b7ef

mattyg added 10 commits August 30, 2024 20:45

wip

9094d37

wip: try ucrt

a8af8f9

wip

6dfcc86

wip

73e1aba

wip

aa46052

fix: bump wamr

c5cc0f7

ci: pause work on attempting windows build with wasmer_wamr feature, …

8d497c7

…explanatory comment

fix: typo

782990b

fix: invalid flag

2ae1eda

chore: cleanup

4d8277b

mattyg changed the title ~~WIP Some cleanup on wasmer/wamr integration~~ Improve CI for wasmer_wamr feature Sep 1, 2024

mattyg added 3 commits September 1, 2024 08:32

ci: try to resolve ubuntu ci being cancelled - maybe its out of memory?

4c1a0c8

fix: syntax

69ba4a7

fix: moar swap space

94c8ab2

mattyg requested review from neonphog, maackle, ThetaSinner and jost-s September 5, 2024 21:26

neonphog approved these changes Sep 6, 2024

View reviewed changes

jost-s approved these changes Sep 10, 2024

View reviewed changes

ci: only run bench on ubuntu

e0f1780

ThetaSinner reviewed Sep 11, 2024

View reviewed changes

ThetaSinner approved these changes Sep 11, 2024

View reviewed changes

fix: workflow concurrency group should be only for that pr

cd985e0

mattyg merged commit 280f3ac into develop Sep 11, 2024
8 checks passed

mattyg deleted the fix/integration-wasmer-wamr branch September 11, 2024 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CI for wasmer_wamr feature #116

Improve CI for wasmer_wamr feature #116

mattyg commented Aug 30, 2024 •

edited

Loading

mattyg Aug 30, 2024

mattyg Aug 30, 2024

mattyg Aug 30, 2024 •

edited

Loading

mattyg Aug 30, 2024

mattyg Aug 30, 2024

neonphog commented Sep 6, 2024

neonphog left a comment

jost-s left a comment

jost-s Sep 10, 2024

mattyg Sep 11, 2024

ThetaSinner commented Sep 11, 2024

ThetaSinner Sep 11, 2024

jost-s Sep 11, 2024

ThetaSinner Sep 11, 2024

mattyg Sep 11, 2024

ThetaSinner Sep 11, 2024

mattyg Sep 11, 2024

ThetaSinner left a comment

mattyg commented Sep 11, 2024

		os: ["ubuntu-latest", "macos-latest"]
		wasmer-feature: ["wasmer_sys", "wasmer_wamr"]

		@@ -0,0 +1,4 @@
		#! /usr/bin/env bash

		./bench-wasmer_sys.sh && \

Improve CI for wasmer_wamr feature #116

Improve CI for wasmer_wamr feature #116

Conversation

mattyg commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyg Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neonphog commented Sep 6, 2024

neonphog left a comment

Choose a reason for hiding this comment

jost-s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThetaSinner commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThetaSinner left a comment

Choose a reason for hiding this comment

mattyg commented Sep 11, 2024

mattyg commented Aug 30, 2024 •

edited

Loading

mattyg Aug 30, 2024 •

edited

Loading