Migrate pallet-nis benchmark to v2 #6293

aurexav · 2024-10-30T15:12:48Z

Refactor to use the frame crate.
Use procedural macro version construct_runtime in mock.
Expose PalletId to frame::pallet_prelude.
Part of Refactor pallets to V2 bench syntax #6202.

Polkadot address: 156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y

substrate/frame/nis/src/benchmarking.rs

…ntime_dev'

kianenigma

Can you also migrate this pallet to the umbrella crate, as a part of this migration?

Here's an example PR: #5995

prdoc/pr_6293.prdoc

aurexav · 2024-11-15T07:40:15Z

Can you also migrate this pallet to the umbrella crate, as a part of this migration?

Here's an example PR: #5995

Sure, will check that.

Co-authored-by: Giuseppe Re <giuseppe.re@parity.io>

aurexav · 2024-12-08T10:55:49Z

@kianenigma @re-gius, please review.

aurexav · 2024-12-08T10:56:38Z

/cmd prdoc --bump patch --audience runtime_dev

I updated the PR description. Can I call this?

re-gius · 2024-12-10T16:41:22Z

Please take a look at CI errors. Also comment in #6504 if you're migrating pallet-nis to the umbrella crate in this PR.

re-gius · 2024-12-11T09:48:43Z

bot bench substrate-pallet --features=runtime-benchmarks --pallet=pallet_nis

re-gius · 2024-12-11T10:05:06Z

substrate/frame/support/src/lib.rs

@@ -909,7 +909,7 @@ pub mod pallet_prelude {
 			Task, TypedGet,
 		},
 		Blake2_128, Blake2_128Concat, Blake2_256, CloneNoBound, DebugNoBound, EqNoBound, Identity,
-		PartialEqNoBound, RuntimeDebugNoBound, Twox128, Twox256, Twox64Concat,
+		PalletId, PartialEqNoBound, RuntimeDebugNoBound, Twox128, Twox256, Twox64Concat,


You're not supposed to change this pallet in the context of migration to the umbrella crate

Otherwise, I have to depend on frame-support and use frame_support::PalletId. I think this is a part of migration to the umbrella crate.

You should add frame_support::PalletId in a prelude in the umbrella crate (file mentioned in the other comment) and avoid changing this pallet.

Make sense.

But after some thoughts, I think it's also good to place the PalletId here.

I added PalletId to frame-support's root 3 years ago paritytech/substrate#8477. At that time, there wasn't a really good place for it. I think it's a good time to move it into frame_support::pallet_prelude, it is being used frequently.

In principle, I completely agree with you. The problem here is many parachains depend on frame_support, so a major bump in frame_support could cause breaking changes for external teams. This is why we decided to never touch the frame_support prelude for umbrella crate migrations. If you still strongly feel that the frame support prelude needs an update, then it's probably better to group all the changes in a single PR and have a broad discussion about that.

re-gius · 2024-12-11T10:10:43Z

substrate/frame/nis/src/lib.rs

+pub use weights::WeightInfo;
+
+use alloc::{vec, vec::Vec};
+use frame::{


while this is Rust-correct, it does not follow the guidelines for migrations to the umbrella crate (see here). For instance, DefensiveSaturating is used across several pallets, so it should go into the main prelude. You should edit this and add new imports into the main preludes according to the guidelines. Ideally, we'd like to have just a use frame::prelude::* statement here. While this won't be probably the case because of ad-hoc imports for this pallet, having these many ad-hoc imports kind of defeats the whole purpose of having the umbrella crate.

~~Conflicts with your previous comment?!~~

I think we should proceed with this once "migrate to umbrella" is completed? I'm uncertain about which one is being utilized most often. Additionally, I've noticed many crates are still referencing use frame::traits::Currency. Isn't Currency a common used trait? Shouldn't it be included in the prelude? There are many aspects that remain quite unclear to me at this moment.

You're part of this migration process (if you want to), meaning that as we migrate new pallets to the umbrella crate, we notice that we need to add new imports to the preludes and we do that if those imports affect more than one pallet (see guidelines). The guidelines I mentioned also say why currency-related traits are kept out of this, please take a look.

Adding new imports to the umbrella crate is the main part of this task.

Well.. That currency guidelines has been collapsed. So I missed that one.

Sorry for the delay, will resolve them today later.

fungible, nonfungible and tokens should belong to currency? I didn't expose them to prelude.

There is still a problem. There are a lot of same name traits in those scope.

For example, fungible::Inspect, nofungible::Inspect.

There are 2 ways to resolve this.

Only expose the mod. fungible, nofungible, ..

Give them an alias, pub use fungible::Inspect as FunInspect.

ok, right, then it's maybe better to revert the changes for conflicting names and keep it as it was before

I can't continue the work if the conclusion is "maybe". 😿

That's my advice, revert those changes, it's better at this point so that we can merge this

…kv2-nis

re-gius

Thanks! Just a few small changes + I will run the cmd bench bot

substrate/frame/nis/Cargo.toml

Cargo.lock

substrate/frame/nis/src/tests.rs

Co-authored-by: Giuseppe Re <giuseppe.re@parity.io>

re-gius · 2025-03-03T15:30:21Z

/cmd bench --pallet pallet_nis --clean

github-actions · 2025-03-03T15:31:06Z

Command "bench --pallet pallet_nis --clean" has started 🚀 See logs here

github-actions · 2025-03-03T17:15:15Z

Command "bench --pallet pallet_nis --clean" has finished ✅ See logs here

Subweight results:

No changes found.

Command output:

✅ Successful benchmarks of runtimes/pallets:
-- dev: ['pallet_nis']
-- rococo: ['pallet_nis']

prdoc/pr_6293.prdoc

ggwpez

Thanks!

ggwpez · 2025-03-03T20:23:51Z

substrate/frame/nis/src/benchmarking.rs

+
+		#[block]
+		{
+			Pallet::<T>::process_bid(


Can we use an extrinsic_call here instead of block?

process_bid is an internal fn.

re-gius · 2025-03-04T10:27:08Z

/tip small

substrate-tip-bot · 2025-03-04T10:27:47Z

@re-gius A referendum for a small (20 DOT) tip was successfully submitted for @aurexav (156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y on polkadot).

Referendum number: 1471.

substrate-tip-bot · 2025-03-04T10:27:50Z

The referendum has appeared on Polkassembly.

Migrate pallet-nis benchmark to v2

afa387a

aurexav requested a review from a team as a code owner October 30, 2024 15:12

aurexav mentioned this pull request Oct 30, 2024

Refactor pallets to V2 bench syntax #6202

Closed

43 tasks

re-gius added T2-pallets This PR/Issue is related to a particular pallet. T12-benchmarks This PR/Issue is related to benchmarking and weights. labels Nov 14, 2024

re-gius approved these changes Nov 14, 2024

View reviewed changes

substrate/frame/nis/src/benchmarking.rs Outdated Show resolved Hide resolved

Update from re-gius running command 'prdoc --bump patch --audience ru…

5f1eaa1

…ntime_dev'

kianenigma reviewed Nov 15, 2024

View reviewed changes

prdoc/pr_6293.prdoc Show resolved Hide resolved

kianenigma added the R0-silent Changes should not be mentioned in any release notes label Nov 15, 2024

aurexav and others added 3 commits December 8, 2024 10:37

Update substrate/frame/nis/src/benchmarking.rs

11c148c

Co-authored-by: Giuseppe Re <giuseppe.re@parity.io>

Merge branch 'master' into benchmarkv2-nis

f4fa365

Refactor to use frame crate

dbd0016

Fix compile

c63bf7d

aurexav mentioned this pull request Dec 11, 2024

[tracking] Migrate pallets to FRAME umbrella crate #6504

Open

92 tasks

aurexav added 2 commits December 11, 2024 10:30

Format

ae516e5

Fix

4371e28

re-gius reviewed Dec 11, 2024

View reviewed changes

Expose common types to prelude

fa89f0c

aurexav force-pushed the benchmarkv2-nis branch from 7a8e7a5 to fa89f0c Compare December 11, 2024 11:53

aurexav added 5 commits December 16, 2024 11:08

TBD

262e1c9

Merge commit 'cb0d8544dc8828c7b5e7f6a5fc20ce8c6ef9bbb4' into benchmar…

a16ce7d

…kv2-nis

Resolve comments

5b39275

Merge commit '7702fdd1bd869e518bf176ccf0268f83f8927f9b' into benchmar…

e7b9402

…kv2-nis

Merge remote-tracking branch 'origin/master' into benchmarkv2-nis

715b8e3

aurexav added 2 commits March 1, 2025 02:52

Merge commit '1bc6ca606438a65c927f14be3f36634ca0e58e8f' into benchmar…

26338c0

…kv2-nis

Remove unnecessary items

e525c4a

aurexav marked this pull request as ready for review March 1, 2025 16:50

re-gius reviewed Mar 3, 2025

View reviewed changes

substrate/frame/nis/Cargo.toml Outdated Show resolved Hide resolved

Cargo.lock Show resolved Hide resolved

substrate/frame/nis/src/tests.rs Outdated Show resolved Hide resolved

aurexav and others added 3 commits March 3, 2025 22:17

Update substrate/frame/nis/src/tests.rs

134e582

Co-authored-by: Giuseppe Re <giuseppe.re@parity.io>

Update Cargo.lock

9f685da

Co-authored-by: Giuseppe Re <giuseppe.re@parity.io>

Fmt

04b89a8

paritytech deleted a comment from github-actions bot Mar 3, 2025

Fix

4dceb83

github-actions bot deleted a comment from re-gius Mar 3, 2025

Update from re-gius running command 'bench --pallet pallet_nis --clean'

8cde3d9

re-gius approved these changes Mar 3, 2025

View reviewed changes

ggwpez reviewed Mar 3, 2025

View reviewed changes

prdoc/pr_6293.prdoc Outdated Show resolved Hide resolved

ggwpez approved these changes Mar 3, 2025

View reviewed changes

aurexav added 2 commits March 4, 2025 10:45

Update prdoc

c26b0fb

Merge branch 'master' into benchmarkv2-nis

f1cb64a

seemantaggarwal approved these changes Mar 4, 2025

View reviewed changes

re-gius added this pull request to the merge queue Mar 4, 2025

Merged via the queue into paritytech:master with commit c8d3339 Mar 4, 2025
248 of 254 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate pallet-nis benchmark to v2 #6293

Migrate pallet-nis benchmark to v2 #6293

aurexav commented Oct 30, 2024 •

edited

Loading

kianenigma left a comment

aurexav commented Nov 15, 2024

aurexav commented Dec 8, 2024

aurexav commented Dec 8, 2024

re-gius commented Dec 10, 2024

re-gius commented Dec 11, 2024

re-gius Dec 11, 2024

aurexav Dec 11, 2024 •

edited

Loading

re-gius Dec 11, 2024

aurexav Dec 11, 2024

re-gius Jan 3, 2025

re-gius Dec 11, 2024

aurexav Dec 11, 2024 •

edited

Loading

aurexav Dec 11, 2024

re-gius Dec 11, 2024 •

edited

Loading

aurexav Dec 11, 2024

aurexav Jan 15, 2025

aurexav Jan 18, 2025 •

edited

Loading

re-gius Jan 20, 2025

aurexav Jan 21, 2025 •

edited

Loading

re-gius Jan 23, 2025

re-gius left a comment

re-gius commented Mar 3, 2025

github-actions bot commented Mar 3, 2025

github-actions bot commented Mar 3, 2025

ggwpez left a comment

ggwpez Mar 3, 2025

aurexav Mar 4, 2025

re-gius commented Mar 4, 2025

substrate-tip-bot bot commented Mar 4, 2025

substrate-tip-bot bot commented Mar 4, 2025

Migrate pallet-nis benchmark to v2 #6293

Migrate pallet-nis benchmark to v2 #6293

Conversation

aurexav commented Oct 30, 2024 • edited Loading

kianenigma left a comment

Choose a reason for hiding this comment

aurexav commented Nov 15, 2024

aurexav commented Dec 8, 2024

aurexav commented Dec 8, 2024

re-gius commented Dec 10, 2024

re-gius commented Dec 11, 2024

Choose a reason for hiding this comment

aurexav Dec 11, 2024 • edited Loading

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

aurexav Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

re-gius Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aurexav Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aurexav Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

re-gius left a comment

Choose a reason for hiding this comment

re-gius commented Mar 3, 2025

github-actions bot commented Mar 3, 2025

github-actions bot commented Mar 3, 2025

ggwpez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

re-gius commented Mar 4, 2025

substrate-tip-bot bot commented Mar 4, 2025

substrate-tip-bot bot commented Mar 4, 2025

aurexav commented Oct 30, 2024 •

edited

Loading

aurexav Dec 11, 2024 •

edited

Loading

aurexav Dec 11, 2024 •

edited

Loading

re-gius Dec 11, 2024 •

edited

Loading

aurexav Jan 18, 2025 •

edited

Loading

aurexav Jan 21, 2025 •

edited

Loading