Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Change weight template outputs to (x * constants::WEIGH_PER_NANOS) #11233

Closed

Conversation

hzy1919
Copy link
Contributor

@hzy1919 hzy1919 commented Apr 18, 2022

Change the weight template currently outputs from (x as Weight) to (x * constants::WEIGHT_PER_NANOS) which is semantically more clear. Related #11215 .

polkadot address: 15wPGN5vysDW1HYWfZRjQQhv2ktpfLMMuiJNMRKcodwPfM14

…T_PER_NANOS) which is semantically more clear
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Apr 18, 2022

User @hzy1919, please sign the CLA here.

@ggwpez
Copy link
Member

ggwpez commented Apr 19, 2022

Thanks @hzy1919!
Looks like a format cargo +nightly fmt is needed. There is also a conflict in one of the files, so please merge latest master into your branch and push again.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Lets see if the benchbot works here.

utils/frame/benchmarking-cli/src/shared/mod.rs Outdated Show resolved Hide resolved
@ggwpez
Copy link
Member

ggwpez commented Apr 19, 2022

/bench runtime pallet pallet_balances

@parity-benchapp
Copy link

parity-benchapp bot commented Apr 19, 2022

Error running benchmark: change-weight-template-output

stdoutFrom https://github.com/paritytech/substrate * branch master -> FETCH_HEAD 62fa7d2..0a7e5ea master -> origin/master

@ggwpez ggwpez added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 19, 2022
@hzy1919
Copy link
Contributor Author

hzy1919 commented Apr 19, 2022

Thanks @hzy1919! Looks like a format cargo +nightly fmt is needed. There is also a conflict in one of the files, so please merge latest master into your branch and push again.

ok, I will handle it .

hzy1919 and others added 4 commits April 19, 2022 20:24
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member

ggwpez commented Apr 19, 2022

I will push the weights of a single pallet, to see that it all works. Then we should be good.
Generating the weights takes a while though…

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
hzy1919 and others added 4 commits April 27, 2022 21:23
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
@hzy1919
Copy link
Contributor Author

hzy1919 commented May 4, 2022

hi @ggwpez ,the CI workflows seems failed a few days ago, I am not sure how to solve it , Can you provide some help?

@ggwpez
Copy link
Member

ggwpez commented May 4, 2022

The polkadot/cumulus error is unrelated i think. You can try to merge master into your branch and then push (not force push).

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

I think we are nearly done here. Great work so far @hzy1919!

utils/frame/benchmarking-cli/src/shared/mod.rs Outdated Show resolved Hide resolved
utils/frame/benchmarking-cli/src/shared/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

I will push some weights again.

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

shawntabrizi commented May 11, 2022

I'm sorry, but end of the day, I don't think I really want this.

I don't see the advantage here. We are strictly adding more complexity and characters to each file.

Also making assumptions which may not always be true, for example that there are no sub-nanosecond weights.

What is more simple than a raw number?

@ggwpez
Copy link
Member

ggwpez commented May 12, 2022

As @shawntabrizi reminded me; We will have new weights soon #10918 so it probably makes not much sense now.
Sorry @hzy1919 that we cannot merge it in the end. I think your effort is worth a tip. Maybe someone tips you if you put your address in the MR description like explained here: https://github.com/paritytech/substrate-tip-bot#pull-request-body

@ggwpez ggwpez closed this May 12, 2022
@shawntabrizi
Copy link
Member

@hzy1919 if you still want a tip for the work you did, please include your polkadot address.

@hzy1919
Copy link
Contributor Author

hzy1919 commented May 14, 2022

Well,it doesn't matter. I'm glad to see a better solution for it . @ggwpez @shawntabrizi Thanks for your patient help ! And i hope i can make more contributions later. 😄

@shawntabrizi
Copy link
Member

/tip small

@substrate-tip-bot
Copy link

@shawntabrizi A small tip was successfully submitted for hzy1919 (15wPGN5vysDW1HYWfZRjQQhv2ktpfLMMuiJNMRKcodwPfM14 on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Development

Successfully merging this pull request may close these issues.

3 participants