garage icon indicating copy to clipboard operation
garage copied to clipboard

Fixing #2248

Open irisliucy opened this issue 5 years ago • 5 comments

Fix #2248 by disabling validate_args.

irisliucy avatar Mar 08 '21 21:03 irisliucy

@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.

avnishn avatar Mar 09 '21 00:03 avnishn

@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.

irisliucy avatar Mar 09 '21 00:03 irisliucy

@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.

avnishn avatar Mar 09 '21 20:03 avnishn

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 avatar Mar 12 '21 19:03 ryanjulian

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.

ryanjulian avatar Mar 12 '21 19:03 ryanjulian