Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI short benchmarking with frame-omni-bencher #4405

Closed
wants to merge 8 commits into from
Closed

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented May 7, 2024

TODO

Follow-up TODO:

Possible follow-ups

  • remove benchmarking code from polkadot / polkadot-parachain binaries
  • simplify gitlab short-benchmarks with matrix-like syntax

@bkontur bkontur added the R0-silent Changes should not be mentioned in any release notes label May 14, 2024
@kianenigma kianenigma mentioned this pull request May 20, 2024
1 task
@bkontur
Copy link
Contributor Author

bkontur commented May 22, 2024

@ggwpez
Currently, we can use frame-omni-bencher either with pre-generated chain-spec with runtime code included or with pre-compiled runtime wasm blob which has GenesisBuilder::get_preset defined.

I am thinking about another possibility that could be maybe very useful,
what if we add support for frame-omni-bencher to run with both combined:

  • pre-compiled runtime wasm blob which has GenesisBuilder::get_preset, which could generate default genesis spec
  • and then merge/override default genesis spec with provided json chain-spec as argument (without runtime code attribute)
./target/release/frame-omni-bencher v1 benchmark pallet \ 
    --runtime ./target/release/wbuild/bridge-hub-rococo-runtime/bridge_hub_rococo_runtime.compact.compressed.wasm
    --chain some-chain-spec-with-overrides.json
    --all --steps 2 --repeat 1

This would add flexibility, e.g. when you want to tune benchmarking with different genesis, you just change json and not need to compile wasm again and so on.

I checked the code, and it should be that hard, maybe just to add new GenesisBuilder type e.g. RuntimeWithSpecOverride or something like that.

@ggwpez
Copy link
Member

ggwpez commented May 22, 2024

This would add flexibility, e.g. when you want to tune benchmarking with different genesis, you just change json and not need to compile wasm again and so on.

Yea it could be useful. But should probably rename the argument from --chain to --genesis-spec since i want to deprecate that. Its just not a good name in general 🙈

@bkontur bkontur changed the title Add frame-omni-bencher to the CI artifacts and release binary CI short benchmarking with frame-omni-bencher May 29, 2024
@bkontur
Copy link
Contributor Author

bkontur commented May 29, 2024

sync with: #4617
cc: @alvicsam

github-merge-queue bot pushed a commit that referenced this pull request May 29, 2024
marking it as release-able, attaching the same version number that is
attached to other binaries such as `polkadot` and `polkadot-parachain`.

I have more thoughts about the version number, though. The chain-spec
builder is mainly a user of the `sp-genesis-builder` api. So the
versioning should be such that it helps users know give a version of
`sp-genesis-builder` in their runtime, which version of
`chain-spec-builder` should they use?

With this, we can possibly alter the version number to always match
`sp-genesis-builder`.

Fixes #4352

- [x] Add to release artifacts ~~similar to
#4405 done here:
#4557

---------

Co-authored-by: Branislav Kontur <bkontur@gmail.com>
EgorPopelyaev pushed a commit that referenced this pull request May 30, 2024
marking it as release-able, attaching the same version number that is
attached to other binaries such as `polkadot` and `polkadot-parachain`.

I have more thoughts about the version number, though. The chain-spec
builder is mainly a user of the `sp-genesis-builder` api. So the
versioning should be such that it helps users know give a version of
`sp-genesis-builder` in their runtime, which version of
`chain-spec-builder` should they use?

With this, we can possibly alter the version number to always match
`sp-genesis-builder`.

Fixes #4352

- [x] Add to release artifacts ~~similar to
#4405 done here:
#4557

---------

Co-authored-by: Branislav Kontur <bkontur@gmail.com>
EgorPopelyaev pushed a commit that referenced this pull request May 30, 2024
marking it as release-able, attaching the same version number that is
attached to other binaries such as `polkadot` and `polkadot-parachain`.

I have more thoughts about the version number, though. The chain-spec
builder is mainly a user of the `sp-genesis-builder` api. So the
versioning should be such that it helps users know give a version of
`sp-genesis-builder` in their runtime, which version of
`chain-spec-builder` should they use?

With this, we can possibly alter the version number to always match
`sp-genesis-builder`.

Fixes #4352

- [x] Add to release artifacts ~~similar to
#4405 done here:
#4557

---------

Co-authored-by: Branislav Kontur <bkontur@gmail.com>
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
marking it as release-able, attaching the same version number that is
attached to other binaries such as `polkadot` and `polkadot-parachain`.

I have more thoughts about the version number, though. The chain-spec
builder is mainly a user of the `sp-genesis-builder` api. So the
versioning should be such that it helps users know give a version of
`sp-genesis-builder` in their runtime, which version of
`chain-spec-builder` should they use?

With this, we can possibly alter the version number to always match
`sp-genesis-builder`.

Fixes paritytech#4352

- [x] Add to release artifacts ~~similar to
paritytech#4405 done here:
paritytech#4557

---------

Co-authored-by: Branislav Kontur <bkontur@gmail.com>
@serban300 serban300 mentioned this pull request Jul 23, 2024
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
marking it as release-able, attaching the same version number that is
attached to other binaries such as `polkadot` and `polkadot-parachain`.

I have more thoughts about the version number, though. The chain-spec
builder is mainly a user of the `sp-genesis-builder` api. So the
versioning should be such that it helps users know give a version of
`sp-genesis-builder` in their runtime, which version of
`chain-spec-builder` should they use?

With this, we can possibly alter the version number to always match
`sp-genesis-builder`.

Fixes paritytech#4352

- [x] Add to release artifacts ~~similar to
paritytech#4405 done here:
paritytech#4557

---------

Co-authored-by: Branislav Kontur <bkontur@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Aug 7, 2024
- Part of paritytech/ci_cd#1006
- Closes: paritytech/ci_cd#1010
- Related: #4405

- Possibly affecting how frame-omni-bencher works on different runtimes:
#5083

Currently works in parallel with gitlab short benchmarks. 
Triggered only by adding `GHA-migration` label to assure smooth
transition (kind of feature-flag).
Later when tested on random PRs we'll remove the gitlab and turn on by
default these tests

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
gui1117 pushed a commit to gui1117/polkadot-sdk that referenced this pull request Aug 15, 2024
- Part of paritytech/ci_cd#1006
- Closes: paritytech/ci_cd#1010
- Related: paritytech#4405

- Possibly affecting how frame-omni-bencher works on different runtimes:
paritytech#5083

Currently works in parallel with gitlab short benchmarks. 
Triggered only by adding `GHA-migration` label to assure smooth
transition (kind of feature-flag).
Later when tested on random PRs we'll remove the gitlab and turn on by
default these tests

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Aug 28, 2024
- Part of https://github.com/paritytech/ci_cd/issues/1006
- Closes: https://github.com/paritytech/ci_cd/issues/1010
- Related: paritytech#4405

- Possibly affecting how frame-omni-bencher works on different runtimes:
paritytech#5083

Currently works in parallel with gitlab short benchmarks. 
Triggered only by adding `GHA-migration` label to assure smooth
transition (kind of feature-flag).
Later when tested on random PRs we'll remove the gitlab and turn on by
default these tests

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@bkontur bkontur force-pushed the bko-bencher branch 2 times, most recently from e1ff127 to 6135102 Compare September 1, 2024 19:33
@bkontur bkontur force-pushed the bko-bencher branch 2 times, most recently from 4670223 to 425fd15 Compare September 1, 2024 21:01
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 3/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7209519

bkontur and others added 8 commits September 9, 2024 16:12
clippy

clippy

clippy

Fix
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix changes_to_storage

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Better error messages

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

genesis-preset flag

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Bench runner fix genesis storage

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

clippy
@bkontur
Copy link
Contributor Author

bkontur commented Sep 13, 2024

closing in favor of:
#5327
#5655
#5706

@bkontur bkontur closed this Sep 13, 2024
@bkontur bkontur deleted the bko-bencher branch September 13, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants