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

Improve frame-omni-bencher docs #6392

Open
iulianbarbu opened this issue Nov 6, 2024 · 7 comments · May be fixed by #6642
Open

Improve frame-omni-bencher docs #6392

iulianbarbu opened this issue Nov 6, 2024 · 7 comments · May be fixed by #6642
Assignees
Labels
C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. T12-benchmarks This PR/Issue is related to benchmarking and weights.

Comments

@iulianbarbu
Copy link
Contributor

iulianbarbu commented Nov 6, 2024

Straight-forward changes:

  • the command from the usage example should be backed by integration tests, as we did in staging-chain-spec-builder, and exported to the docs by using docify::export_content
  • the README.md should be generated from a README.docify.md as we did with staging-chain-spec-builder again, by running a cargo command with --features generate-readme.
  • the README.md should be include_stred into a lib.rs (even if the crate is intended to be a binary create) - currently the README's content is duplicated as public docs for command.rs::Command struct, which isn't displayed/used anywhere. docs.rs will show something like below (which I assume is taken from crates.io):

Screenshot from 2024-11-06 16-48-19

Not a must, but good practice related, as we did with staging-chain-spec-builder:

  • existing integration tests could use cmd_lib::run_cmd or other cmd_lib alternative, based on the assertion logic needs.
  • we might extend the usage examples with some more representative commands, backed by the above integration tests, if really useful (I am not very familiar with frame-omni-bencher usage, but we'll want to clarify that at some point).
@iulianbarbu iulianbarbu added T12-benchmarks This PR/Issue is related to benchmarking and weights. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. labels Nov 6, 2024
@rainbow-promise
Copy link

Let me take a crack at it @iulianbarbu

@iulianbarbu
Copy link
Contributor Author

Thanks @rainbow-promise , go for it. Assigned this to you.

@iulianbarbu
Copy link
Contributor Author

hey @rainbow-promise ! How is it going? Please let us know if you encounter blockers with this, and no worries if you haven't got a chance to start it, just thought about checking in.

@rainbow-promise
Copy link

Hey @iulianbarbu, currently working on this #6321

Should have a PR by the week and thanks for checking in 😁

@iulianbarbu
Copy link
Contributor Author

Hey @iulianbarbu, currently working on this #6321

Should have a PR by the week and thanks for checking in 😁

Amazing, take your time. Looking forward to the PR when it's ready. 😄

@rainbow-promise
Copy link

Hello @iulianbarbu

Wondering if I need to build a custom pallet and runtime similar to:

docs/sdk/reference_docs/chain_spec_runtime/tests/chain_spec_builder_tests.rs

Or I could create a command that builds the westend runtime and bench a specific pallet?

@rainbow-promise rainbow-promise linked a pull request Nov 25, 2024 that will close this issue
@iulianbarbu
Copy link
Contributor Author

iulianbarbu commented Nov 27, 2024

Hey @rainbow-promise ! First of all sorry for the late reply and thanks for opening the draft PR for this!

Wondering if I need to build a custom pallet and runtime similar to:
docs/sdk/reference_docs/chain_spec_runtime/tests/chain_spec_builder_tests.rs
Or I could create a command that builds the westend runtime and bench a specific pallet?

We can use the runtimes WASM_BINARY_PATH, but we'd need to add a [dev-dependency] for the runtime we'd use for benching its pallets. I am also thinking that substrate-test-runtime is a better choice than westend IMO. This is an example of how we're using the runtime and the path to it: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/bin/utils/chain-spec-builder/tests/test.rs#L248, and this is how we make its building happen by default: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/bin/utils/chain-spec-builder/Cargo.toml#L42. We can still bench against balances pallet though.

@iulianbarbu iulianbarbu moved this to Milestone 1 in Polkadot Omni Node Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. T12-benchmarks This PR/Issue is related to benchmarking and weights.
Projects
Status: Milestone 1
Development

Successfully merging a pull request may close this issue.

2 participants