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

Fixing #2248 #2249

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixing #2248 #2249

wants to merge 1 commit into from

Conversation

irisliucy
Copy link
Contributor

Fix #2248 by disabling validate_args.

@irisliucy irisliucy requested a review from a team as a code owner March 8, 2021 21:15
@irisliucy irisliucy requested review from krzentner, a team, zequnyu and nicolengsy and removed request for a team March 8, 2021 21:15
@mergify mergify bot requested a review from a team March 8, 2021 21:16
@irisliucy irisliucy requested review from a team and removed request for nicolengsy and a team March 8, 2021 21:16
@mergify mergify bot requested review from a team and gitanshu and removed request for a team March 8, 2021 21:16
@avnishn
Copy link
Member

avnishn commented Mar 9, 2021

@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
Copy link
Contributor Author

@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 irisliucy force-pushed the fix-torch-tanh-normal-expand branch from f6585d4 to 73dd20e Compare March 9, 2021 01:11
@avnishn
Copy link
Member

avnishn commented Mar 9, 2021

@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
Copy link
Member

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
Copy link
Member

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.

@krzentner krzentner removed their request for review September 3, 2021 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_tanh_normal_expand is failing with torch 1.8
3 participants