-
Notifications
You must be signed in to change notification settings - Fork 310
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
base: master
Are you sure you want to change the base?
Fixing #2248 #2249
Conversation
@irisliucy can you explain why validating the arguments to the constructor solves our issue? I'm referencing this article: but I'm not sure how correct it is or how this would fix our tests. |
@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 |
f6585d4
to
73dd20e
Compare
@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. |
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? |
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. |
Fix #2248 by disabling
validate_args
.