Improve Variable interface by goldsborough · Pull Request #5127 · pytorch/pytorch

goldsborough · 2018-02-07T23:11:01Z

This PR is a large change that revamps the autograd::Variable interface to provide a clean API that encapsulates implementation details and reduces code required to interact with Variables.

A large part of this PR are small changes to update use-sites to use the new API. I would recommend beginning by taking a look at variable.h and variable.cpp, as well as variable_version.h, which have large changes.

I've also attempted to improve documentation of the Variable. Please let me know if either the API or the documentation does not make sense, or if either could be improved.

@zdevito @apaszke @colesbury

torch/csrc/utils/variadic.h

apaszke

I'm not a huge heavyweight OOP fan, so I'm a bit ambivalent about this PR. Some changes just pack the same code in slightly nicer methods, while I'd consider other to be quite unnecessary (e.g. struct -> class that doesn't really fit the conventions of our codebase) or dangerous (removing make_variable). I'd also like to have @colesbury look over them to make sure I haven't missed anything, since this is a critical part of this system.

torch/csrc/autograd/python_variable.cpp

torch/csrc/autograd/variable.h

torch/csrc/autograd/variable.cpp

colesbury

This looks generally reasonable to me. I'm most concerned about:

The Variable() constructor which takes data. I think a static factory method might be better
Movement of some functions to the variable.cpp. We don't compile with LTO and many of these are called often.

torch/csrc/autograd/saved_variable.h

torch/csrc/autograd/variable.h

torch/csrc/autograd/functions/special.cpp

torch/csrc/autograd/python_variable.cpp

torch/csrc/autograd/variable.cpp

goldsborough · 2018-02-08T00:54:58Z

Thanks for the comments. I think I fixed all of them. Let me know about make_variable vs. Variable::<something>.

tools/autograd/templates/VariableType.cpp

torch/csrc/autograd/python_variable.cpp

torch/csrc/autograd/variable.h

aten/src/ATen/TensorImpl.h

zdevito

I have a few comments, not yet a full review.

torch/csrc/autograd/functions/basic_ops.cpp

torch/csrc/autograd/saved_variable.h

torch/csrc/autograd/variable.h

torch/csrc/jit/graph_executor.cpp

torch/csrc/autograd/python_variable.cpp

goldsborough · 2018-02-08T23:54:46Z

Summary of updates and who this should be interesting for:

Made Variable constructors into free functions @apaszke @ezyang
Implemented as_variable_ref to downcast Tensor to Variable, with a safe mode if DEBUG is defined @ezyang
Changed grad_fn_ptr() to grad_fn_unsafe() @apaszke
Made the set_type hack less hacky by (1) Making the type_ field protected in TensorImpl (@zdevito says this is fine) and makes sense anyway and (2) prefixing set_type with temporary_hack_, @ezyang @apaszke
Slightly unrelated to other things: removed the ir.h include from tracer_state.h, so that future modifications to the JIT will not rebuild variable and its descendants (70 files or so) @zdevito

tools/autograd/templates/VariableType.h

torch/csrc/autograd/saved_variable.h

torch/csrc/autograd/saved_variable.cpp

torch/csrc/autograd/variable.h

torch/csrc/autograd/variable.cpp

goldsborough · 2018-02-12T20:43:34Z

Achievement unlocked: make clang segfault

ezyang

Let's merge this before it bitrots further.

@goldsborough I think we need another rebase

goldsborough · 2018-02-13T03:49:23Z

🙏

1) No longer compact namespaces (revert from pytorch#5127) 2) Don't break on return type for long function declarations

1) No longer compact namespaces (revert from #5127) 2) Don't break on return type for long function declarations

1) No longer compact namespaces (revert from pytorch#5127) 2) Don't break on return type for long function declarations

onnxbot-worker-3 mentioned this pull request Feb 7, 2018

[auto] pytorch-pr-5127 onnxbot/onnx-fb-universe#594

Open

goldsborough commented Feb 7, 2018

View reviewed changes

torch/csrc/utils/variadic.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

apaszke suggested changes Feb 7, 2018

View reviewed changes

colesbury reviewed Feb 8, 2018

View reviewed changes