-
-
Notifications
You must be signed in to change notification settings - Fork 984
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
Fixes for PyTorch 1.7 release #2683
Conversation
Thanks for helping, @fehiepsi 🎉 |
@martinjankowiak can you please take a look at your failing @neerajprad @fehiepsi any idea how to fix multi-chain mcmc in the baseball example? |
@neerajprad I'm seeing weird unexpected |
I'll take a look at this, @fritzo. |
pyro/infer/mcmc/api.py
Outdated
# at https://github.com/pytorch/pytorch/issues/10375 | ||
# This also resolves "RuntimeError: Cowardly refusing to serialize non-leaf tensor which | ||
# requires_grad", which happens with `jit_compile` under PyTorch 1.7 | ||
args = [arg.clone().detach() if torch.is_tensor(arg) else arg for arg in args] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neerajprad Could you double-check if this is a good solution? This resolves the issues for:
- cuda + jit/nojit
- cpu + jit
There is no issue with cpu + nojit
so should we filter it out at if self.num_chains > 1
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we remove the num_chains
restriction and simply detach
(instead of cloning)? That should be very cheap and we can do that as a sanity measure anyways. I think @fritzo has correctly identified that the jit is incorrectly propagating up requires_grad
, so this seems like a bigger problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It seems that .detach()
solves the issue. Removing num_chains
helps for single-chain too (without detach, baseball failed with num_chains=1???). I think the problem comes from this line of Binomial.log_prob
. If I change k * self.logits
to (k + 0) * self.logits
, the problem goes away without having to detach
... That observation agrees with: the jit is incorrectly propagating up requires_grad
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @fehiepsi. I wonder if the problem is due to the cached logits. I have seen this previously for transforms where the cached value has its requires_grad
set during backward, but in that case we get an error saying that JIT cannot insert a constant with a requires_grad
attribute set. That doesn't explain why adding 0 helps take care of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I remember that the issue still happens when I changed self.logits
to self.probs
.
Thanks again for fixing the baseball example @fehiepsi! I think this is ready to merge. |
See release notes
Tasks
replace
.expand(...)
->.expand(...).clone()
if the result must support.__setitem__()
update to use
torch.fft
; see Deprecate old fft functions pytorch/pytorch#44876 (comment)fix
examples/sparse_regression.py
To reproduce, run
pdb -cc examples/sparse_regression.py --num-steps=2 --num-data=50 --num-dimensions 20
fix multi-chain mcmc, failing in the baseball example
The baseball example is failing. to reproduce:
Tested
Ran tests locally against torch==1.7. (Note CI still uses PyTorch 1.6 since that is the oldest supported)
pytest -vx --stage unit
pytest -vx --stage integration
make test-tutorials
pytest -vx --stage test_examples
(currently failing)