Simulation: Params from File #4435

alexanderbez · 2019-05-29T13:43:15Z

Cleanup simulation parameter output (uses JSON now)
Move distribution parameter generation to simulation package
Add simulation godoc (doc.go)

closes: #4428

/cc @npinto

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry: clog add [section] [stanza] [message]
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2019-05-29T14:07:48Z

Codecov Report

Merging #4435 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4435   +/-   ##
=======================================
  Coverage   54.58%   54.58%           
=======================================
  Files         248      248           
  Lines       15967    15967           
=======================================
  Hits         8716     8716           
  Misses       6617     6617           
  Partials      634      634

npinto · 2019-05-30T00:11:57Z

Should we also consider adding new parameters to control some hardcoded values like e.g.:

cosmos-sdk/simapp/sim_test.go

Line 312 in d099d84

{100, banksim.SimulateMsgSend(app.accountKeeper, app.bankKeeper)},

What do you think?

alexanderbez · 2019-05-31T21:59:58Z

Good idea @npinto! Yes, I'll include this too.

simapp/doc.go

simapp/test_util.go

x/simulation/util.go

alessio

Few comments - changes are good, docs is great, as such deserves better go doc compliant formatting

fedekunze

minor comments only

simapp/sim_test.go

simapp/test_util.go

x/simulation/util.go

simapp/sim_test.go

fedekunze

ACK

alexanderbez · 2019-06-05T17:33:04Z

@alessio I fixed the godoc. It's pretty now. Ty

simapp/doc.go

alessio

pending tiny corrections - else this looks good

Co-Authored-By: Alessio Treglia <quadrispro@ubuntu.com>

npinto · 2019-06-05T18:59:51Z

What do you think about allowing pausing/resuming of these simulations (w.r.t. eg blocks) ? That is: (1) having a way to dump the full state of the simulator at the end of a given number of blocks, and (2) taking this state and resuming the simulation for another given number of blocks.

This could allow "in-flight" fine tuning of parameters and also much better optimizations of simulations (e.g. "early stopping" of non promising simulations).

alexanderbez · 2019-06-05T19:19:33Z

@npinto good idea but out of scope of this PR! Let's open up another issue for that. I'd like to keep the PRs as minimal and focused as possible 👍

npinto · 2019-06-05T19:36:34Z

@alexanderbez sounds good, I'll open another feature request for this. Thanks!

npinto · 2019-06-05T23:06:51Z

Should we consider printing out a json representation of the event statistics? like e.g.:

Simulation stopped early as all validators have been unbonded; nobody left to propose a block!
Event statistics:
                                         /no-operation/failure => 1582
                                       auth/deduct_fee/failure => 15
                                            auth/deduct_fee/ok => 159
                                        bank/multisend/failure => 22
                                             bank/multisend/ok => 217
                                             bank/send/failure => 242
                                                  bank/send/ok => 2379
                                           beginblock/evidence => 151
                                     beginblock/signing/missed => 565
                                     beginblock/signing/signed => 2768
                            distr/set_withdraw_address/failure => 1312
                       distr/withdraw_delegator_reward/failure => 1190
                            distr/withdraw_delegator_reward/ok => 140
                   distr/withdraw_validator_commission/failure => 299
                        distr/withdraw_validator_commission/ok => 994
                               endblock/validatorupdates/added => 106
                              endblock/validatorupdates/kicked => 113
                             endblock/validatorupdates/updated => 842
                                           gov/deposit/failure => 802
                                                gov/deposit/ok => 1741
                                   gov/submit_proposal/failure => 60
                                        gov/submit_proposal/ok => 300
                                       slashing/unjail/failure => 2483
                              staking/begin_redelegate/failure => 1944
                                   staking/begin_redelegate/ok => 148
                              staking/create_validator/failure => 2018
                                   staking/create_validator/ok => 106
                                      staking/delegate/failure => 1479
                                           staking/delegate/ok => 2910
                                staking/edit_validator/failure => 60
                                     staking/edit_validator/ok => 63
GoLevelDB Stats
Compactions
 Level |   Tables   |    Size(MB)   |    Time(sec)  |    Read(MB)   |   Write(MB)
-------+------------+---------------+---------------+---------------+---------------
   0   |          0 |       0.00000 |       0.06395 |       0.00000 |       5.99821
   1   |          1 |       0.84860 |       0.02618 |       5.99821 |       0.84860

GoLevelDB cached block size 1273211
--- PASS: TestFullAppSimulation (30.89s)
PASS
ok  	github.com/cosmos/cosmos-sdk/simapp	30.936s

It might then be easier to parse them out if they need to be analyzed/optimized.

fedekunze · 2019-06-06T10:09:29Z

Should we consider printing out a json representation of the event statistics?

That would be nice but maybe not for the scope of this PR. I'd merge this and then improve it afterwards with the comments you suggested @npinto

alexanderbez · 2019-06-06T12:48:25Z

Yeah sure @npinto, mind opening up another issue for this?

rigelrozanski

Looks good, great documentation section.

Few comments throughout, biggest comment is the code structure of using immediately executed function variables while generating module genesis information (aka genXXXXGenesisState)

x/simulation/params.go

x/simulation/util.go

simapp/doc.go

simapp/sim_test.go

rigelrozanski · 2019-06-06T18:12:10Z

simapp/sim_test.go

+				var v uint64
+				ap.GetOrGenerate(cdc, "max_memo_characters", &v, r, func(r *rand.Rand) { v = simulation.ModuleParamSimulator["max_memo_characters"](r).(uint64) })
+				return v
+			}(r),


This code structure is quite obfuscated, I understand the thinking to compress all the code required to getOrGen a param into an immediately executed function as is done here. But I think a more intuitive/digestable structure would be to just individually get all the parameters at the top of this function (before defining authGenesis). Something like:

func genAuthGenesisState(cdc *codec.Codec, r *rand.Rand, ap simulation.AppParams, genesisState map[string]json.RawMessage) { var maxMemoChars uint64 ap.GetOrGenerate(cdc, "max_memo_characters", &maxMemoChars, r, func(r *rand.Rand) { v = simulation.ModuleParamSimulator["max_memo_characters"](r).(uint64) }) .... authGenesis := auth.NewGenesisState( nil, auth.NewParams( maxMemoChars, ...

This also applies too all modules aka genXXXXGenesisState

Yeah so I understand your concern upon first reading the code. I think the additional verbosity that results from such a suggested change isn't worth it because now that we have constants that are directly mapped to the meaning of the arguments, it's very clear what it's doing and what value it provides.

I'd rather not have to go through each of these for what will probably be a tiny (if any) gain in brevity.

So the code would actually become less verbose under the suggested change, however we would have to come up with variable names - that's pretty much the only downside. I don't mind quickly making these changes if it's just a matter of the work

rigelrozanski · 2019-06-06T18:15:31Z

simapp/sim_test.go

+				var v int
+				ap.GetOrGenerate(cdc, "op_weight_deduct_fee", &v, nil, func(_ *rand.Rand) { v = 5 })
+				return v
+			}(nil),


ditto on the defining contents of WeightedOperation before this return statement (not in immediately executed function variables) as per my previous comment on this structure for all the modules (genXXXXGenesisState)

See response :)

rigelrozanski

After implementing my suggestion, I've changed my position, I think it makes things even more obfuscated unfortunately. I've opted to keep it, but I expanded up the 2nd layer function variables what were really hidden in there (and also made the lines very long)

npinto · 2019-07-02T23:41:07Z

Yeah sure @npinto, mind opening up another issue for this?

Sure! See #4670

* Move distr params to simulation param generator * Cleanup sim params output * Print params when possible instead of entire genesis struct * Update module param simulator keys * Rename module params simulator keys * Add/fix missing module params and implement SimParams * UPdate sim test to accept params file * Allow weights to be provided from file * Create doc.go * Remove TODO * Implement MustMarshalJSONIndent * Use mustMarshalJSONIndent for non-codec * Remove TODO * Fix doc.go * Update points * Update simapp/doc.go Co-Authored-By: Alessio Treglia <quadrispro@ubuntu.com> * Update simapp/doc.go Co-Authored-By: Alessio Treglia <quadrispro@ubuntu.com> * Cleanup appStateFn * Use constants for simulation parameter keys * Update msgs.go * minor formatting

Move distr params to simulation param generator

394c981

alexanderbez added WIP C:Simulations labels May 29, 2019

alexanderbez added 4 commits May 29, 2019 10:10

Cleanup sim params output

bb3cd77

Print params when possible instead of entire genesis struct

06c9d4a

Update module param simulator keys

123446b

Rename module params simulator keys

d099d84

alexanderbez added 5 commits May 31, 2019 18:48

Add/fix missing module params and implement SimParams

23d0ade

UPdate sim test to accept params file

39812d3

Allow weights to be provided from file

0552761

Create doc.go

1b4ded5

Remove TODO

4d9a93d

alexanderbez marked this pull request as ready for review June 5, 2019 15:23

alexanderbez requested review from alessio, jackzampolin and rigelrozanski as code owners June 5, 2019 15:23

alexanderbez requested review from cwgoes, fedekunze and sabau June 5, 2019 15:23

alexanderbez added ready-for-review and removed WIP labels Jun 5, 2019

alessio reviewed Jun 5, 2019

View reviewed changes

simapp/doc.go Outdated Show resolved Hide resolved

alessio reviewed Jun 5, 2019

View reviewed changes

simapp/test_util.go Outdated Show resolved Hide resolved

alessio reviewed Jun 5, 2019

View reviewed changes

x/simulation/util.go Show resolved Hide resolved

alessio suggested changes Jun 5, 2019

View reviewed changes

fedekunze reviewed Jun 5, 2019

View reviewed changes

simapp/sim_test.go Outdated Show resolved Hide resolved

simapp/test_util.go Outdated Show resolved Hide resolved

x/simulation/util.go Show resolved Hide resolved

simapp/sim_test.go Show resolved Hide resolved

Implement MustMarshalJSONIndent

6d94be5

fedekunze approved these changes Jun 5, 2019

View reviewed changes

Fix doc.go

6706ad7

alexanderbez requested a review from alessio June 5, 2019 17:33

alessio reviewed Jun 5, 2019

View reviewed changes

simapp/doc.go Outdated Show resolved Hide resolved

alessio reviewed Jun 5, 2019

View reviewed changes

simapp/doc.go Outdated Show resolved Hide resolved

alessio reviewed Jun 5, 2019

View reviewed changes

simapp/doc.go Outdated Show resolved Hide resolved

alessio reviewed Jun 5, 2019

View reviewed changes

simapp/doc.go Outdated Show resolved Hide resolved

alessio approved these changes Jun 5, 2019

View reviewed changes

alexanderbez and others added 3 commits June 5, 2019 13:43

Update points

2bcfa5d

Update simapp/doc.go

de287a5

Co-Authored-By: Alessio Treglia <quadrispro@ubuntu.com>

Update simapp/doc.go

496308f

Co-Authored-By: Alessio Treglia <quadrispro@ubuntu.com>

fedekunze added the will-merge label Jun 5, 2019

rigelrozanski suggested changes Jun 6, 2019

View reviewed changes

alexanderbez added 2 commits June 7, 2019 10:04

Cleanup appStateFn

93e434a

Use constants for simulation parameter keys

5f9d71d

alexanderbez requested a review from rigelrozanski June 7, 2019 14:38

alexanderbez and others added 2 commits June 7, 2019 10:41

Update msgs.go

26977b7

minor formatting

267d073

rigelrozanski approved these changes Jun 8, 2019

View reviewed changes

rigelrozanski merged commit 6606007 into master Jun 8, 2019

rigelrozanski deleted the bez/4428-sim-params-file branch June 8, 2019 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simulation: Params from File #4435

Simulation: Params from File #4435

alexanderbez commented May 29, 2019 •

edited

Loading

codecov bot commented May 29, 2019 •

edited

Loading

npinto commented May 30, 2019 •

edited

Loading

alexanderbez commented May 31, 2019

alessio left a comment

fedekunze left a comment

fedekunze left a comment

alexanderbez commented Jun 5, 2019

alessio left a comment

npinto commented Jun 5, 2019

alexanderbez commented Jun 5, 2019

npinto commented Jun 5, 2019

npinto commented Jun 5, 2019

fedekunze commented Jun 6, 2019

alexanderbez commented Jun 6, 2019

rigelrozanski left a comment

rigelrozanski Jun 6, 2019

alexanderbez Jun 7, 2019

rigelrozanski Jun 7, 2019

rigelrozanski Jun 6, 2019

alexanderbez Jun 7, 2019

rigelrozanski left a comment

npinto commented Jul 2, 2019

Simulation: Params from File #4435

Simulation: Params from File #4435

Conversation

alexanderbez commented May 29, 2019 • edited Loading

codecov bot commented May 29, 2019 • edited Loading

Codecov Report

npinto commented May 30, 2019 • edited Loading

alexanderbez commented May 31, 2019

alessio left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez commented Jun 5, 2019

alessio left a comment

Choose a reason for hiding this comment

npinto commented Jun 5, 2019

alexanderbez commented Jun 5, 2019

npinto commented Jun 5, 2019

npinto commented Jun 5, 2019

fedekunze commented Jun 6, 2019

alexanderbez commented Jun 6, 2019

rigelrozanski left a comment

Choose a reason for hiding this comment

rigelrozanski Jun 6, 2019

Choose a reason for hiding this comment

alexanderbez Jun 7, 2019

Choose a reason for hiding this comment

rigelrozanski Jun 7, 2019

Choose a reason for hiding this comment

rigelrozanski Jun 6, 2019

Choose a reason for hiding this comment

alexanderbez Jun 7, 2019

Choose a reason for hiding this comment

rigelrozanski left a comment

Choose a reason for hiding this comment

npinto commented Jul 2, 2019

alexanderbez commented May 29, 2019 •

edited

Loading

codecov bot commented May 29, 2019 •

edited

Loading

npinto commented May 30, 2019 •

edited

Loading