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

Added randomized simulation parameters generation #389

Merged
merged 17 commits into from
Feb 5, 2021
Merged

Added randomized simulation parameters generation #389

merged 17 commits into from
Feb 5, 2021

Conversation

leobragaz
Copy link
Contributor

@leobragaz leobragaz commented Jan 26, 2021

After I integrated wasmd inside desmos I found out that the simulation tests fails due to the fact that the RandomizedParams function of module.go returns nil.

--- FAIL: TestAppImportExport (128.47s)
panic: UnmarshalJSON cannot decode empty bytes [recovered]
	panic: UnmarshalJSON cannot decode empty bytes

In order to solve this I added the simulation folder with the params generation and the randomized genesis that will be called inside the tests. This could be useful for those projects that run sims.

Closes #392.

@leobragaz leobragaz requested a review from alpe as a code owner January 26, 2021 15:45
@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #389 (e975efd) into master (4141cc3) will decrease coverage by 0.06%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #389      +/-   ##
==========================================
- Coverage   55.42%   55.35%   -0.07%     
==========================================
  Files          39       39              
  Lines        4018     4021       +3     
==========================================
- Hits         2227     2226       -1     
- Misses       1612     1615       +3     
- Partials      179      180       +1     
Impacted Files Coverage Δ
x/wasm/module.go 49.09% <33.33%> (+0.03%) ⬆️
app/app.go 87.74% <66.66%> (-0.35%) ⬇️
x/wasm/internal/keeper/keeper.go 88.36% <0.00%> (-0.56%) ⬇️

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but I don't understand the simulation system.

I'd love @alpe to take a quick look before merging.

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Thank you very much for adding these simulation data generators! 🌻
I have run them locally but ended up with failures. Please see my comments to fix the return types.

x/wasm/simulation/params.go Outdated Show resolved Hide resolved
x/wasm/simulation/params.go Outdated Show resolved Hide resolved
x/wasm/simulation/params.go Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Member

Since this is not used by wasmd internally, I would ask you to submit working test cases for simulation to wasmd (so we could at least manually validate it is correct). We won't focus much more energy on this PR, but happy to merge if it gets working, and then include it in the next release.

We are making a 0.15.0 with the new cosmos-sdk v0.41.0.
Happy to make a 0.15.1 point release with sims when it is working

@leobragaz
Copy link
Contributor Author

Thanks @alpe! Can I ask which tests did you run to end up with failures?

@leobragaz leobragaz requested a review from alpe January 28, 2021 11:32
@alpe alpe added the enhancement New feature or request label Jan 29, 2021
@alpe
Copy link
Contributor

alpe commented Jan 29, 2021

Thanks @alpe! Can I ask which tests did you run to end up with failures?

In case somebody is following this PR. We talked on discord to move this forward. https://discord.com/channels/737637324434833438/737640672680607764/804352247026417724

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Thanks again for the updates. 🤗 Only some minor comments on the code although I could not make it work on my machine. It fails with a panic and no helpful errors.

ERROR OUTPUT

exit status 1
panic: halting simulations
goroutine 36 [running]:
main.worker(0x1, 0xc000074a20, 0xc00028e000)
	/Users/alex/go/pkg/mod/github.com/cosmos/tools/cmd/runsim@v1.0.0/main.go:216 +0x7a5
main.main.func2(0xc0002a2010, 0xc000074a20, 0xc00028e000, 0x1)
	/Users/alex/go/pkg/mod/github.com/cosmos/tools/cmd/runsim@v1.0.0/main.go:160 +0x6b
created by main.main
	/Users/alex/go/pkg/mod/github.com/cosmos/tools/cmd/runsim@v1.0.0/main.go:158 +0xc1a
make: *** [test-sim-import-export] Error 2

Does it work for you?

app/app.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
x/wasm/simulation/params.go Show resolved Hide resolved
contrib/devtools/Makefile Show resolved Hide resolved
@leobragaz
Copy link
Contributor Author

leobragaz commented Feb 2, 2021

Thanks again for the updates. 🤗 Only some minor comments on the code although I could not make it work on my machine. It fails with a panic and no helpful errors.

ERROR OUTPUT

exit status 1
panic: halting simulations
goroutine 36 [running]:
main.worker(0x1, 0xc000074a20, 0xc00028e000)
	/Users/alex/go/pkg/mod/github.com/cosmos/tools/cmd/runsim@v1.0.0/main.go:216 +0x7a5
main.main.func2(0xc0002a2010, 0xc000074a20, 0xc00028e000, 0x1)
	/Users/alex/go/pkg/mod/github.com/cosmos/tools/cmd/runsim@v1.0.0/main.go:160 +0x6b
created by main.main
	/Users/alex/go/pkg/mod/github.com/cosmos/tools/cmd/runsim@v1.0.0/main.go:158 +0xc1a
make: *** [test-sim-import-export] Error 2

Does it work for you?

@alpe
Above those cryptic lines you should also see this line:

Seed 7: FAILED
To reproduce run: go test ./app -run TestAppImportExport -Enabled=true -NumBlocks=50 -Genesis= -Verbose=true -Commit=true -Seed=7 -Period=5 -ExportParamsPath /var/folders/xs/g0l3kxkx5msct63qbx9shlsr0000gn/T/sim-logs-833631974/sim_params-7.json -ExportStatePath /var/folders/xs/g0l3kxkx5msct63qbx9shlsr0000gn/T/sim-logs-833631974/sim_state-7.json -v -timeout 24h
ERROR OUTPUT 

If you're using GoLand (or IntelliJ) it's pretty easy to get a clearer error log by editing the configuration of the TestSimAppImportExport by doing these steps:

  1. Run the test from within the sim_test.go file
  2. Edit the configuration and paste this -Enabled=true -NumBlocks=50 -Genesis= -Verbose=true -Commit=true -Seed=7 -Period=5 -ExportParamsPath /var/folders/xs/g0l3kxkx5msct63qbx9shlsr0000gn/T/sim-logs-833631974/sim_params-7.json -ExportStatePath /var/folders/xs/g0l3kxkx5msct63qbx9shlsr0000gn/T/sim-logs-833631974/sim_state-7.json
    inside Program arguments and -v -timeout 24h inside Go tool arguments like this
  3. Re run the configured test

This will print a much meaningful error showing an error on import referring to the capability module.
And that was why I've removed that lines in the first place, but since it will make no difference because it will fail anyway for the same bug (since wasmd is using capability) I'll leave it there where it was before deleting it.

Since it's weird to have a failing test to make sure that params generation works, I've added an extra TestFullAppSimulation that exited with code 0.
For the capability bug I think I will discuss with my team since we will also run into that bug and open an issue into the Cosmos repo.

added me into contributors list
@alpe alpe merged commit d0befd9 into CosmWasm:master Feb 5, 2021
@alpe
Copy link
Contributor

alpe commented Feb 5, 2021

@bragaz thank you very much for this big effort to integrate simulations. Well done 👏
I will 👀 for the SDK bug fix and discuss how we can integrate them into our release process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Parameter change via proposal
4 participants