search

rlworkgroup / garage

A toolkit for reproducible reinforcement learning research.
MIT License
1.84k stars 309 forks source link

Fixing #2248 #2249

Open irisliucy opened 3 years ago

irisliucy commented 3 years ago

Fix #2248 by disabling validate_args.

avnishn commented 3 years ago

@irisliucy can you explain why validating the arguments to the constructor solves our issue?

I'm referencing this article: https://praveen-palanisamy.github.io/blog/2018/04/22/pytorch-distributions-validate-args

but I'm not sure how correct it is or how this would fix our tests. Also for some reason we fail pre-commit on your changes, and this probably is because some new functions were added to the torch.distribution interface, that are not being implemented.

irisliucy commented 3 years ago

@avnishn There was an issue (https://github.com/rlworkgroup/garage/runs/2045693049?check_suite_focus=true#step:5:255) failing github action earlier and @gitanshu also filed a similar issue for torch 1.8. It failed because of validate_args argument in torch.distribution.

avnishn commented 3 years ago

@irisliucy is there a way in which we can go to the tests, and then change the offending the parameters.

Unless there is a good reason to turn off validation of parameters, I think that we shouldn't go in this direction.

ryanjulian commented 3 years ago

According to https://pytorch.org/docs/stable/distributions.html validate_args can be slow which is why it can be turned off in the first place, so this isn't clear-cut.

My big question is, why were our args failing in the first place?

ryanjulian commented 3 years ago

Ah after having looked deeper, I think we should fix the root cause. Seems like not a hard fix. @avnishn can you please suggest the root cause fix? This is your code which is broken.