serialize torch.Size object #20952
serialize torch.Size object #20952
Conversation
pytorchbot
added
module: internals
|
@pytorchbot retest this please |
|
@pytorchbot rebase this please |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
colesbury
left a comment
colesbury
left a comment
There was a problem hiding this comment.
Thanks for fixing this. See inline comment for requested changes
| static PyObject *THPSize_reduce(THPSize *self) | ||
| { | ||
| HANDLE_TH_ERRORS | ||
| auto ret = THPObjectPtr{PyTuple_New(2)}; |
There was a problem hiding this comment.
Almost every call here can fail and should be checked. For example, PyTuple_New can fail with due to out-of-memory. PyImport_ImportModule can fail for lots of reasons.
In general you need to write something like:
auto ret = THPObjectPtr{PyTuple_New(2)};
if (!ret) return NULL;
or you can write the following because of the HANDLE_TH_ERRORS macro:
auto ret = THPObjectPtr{PyTuple_New(2)};
if (!ret) throw python_error();
This is easy to mess up. I think it might be worth writing the __reduce__ function in Python and binding it in C++, although that's not trivial either. (Up to you).
|
@colesbury I updated the PR as suggested, would you mind taking another look when you have time? Thanks! |
torch/csrc/Size.cpp
Outdated
| THPObjectPtr t(PyTuple_New(PyTuple_Size((PyObject*)self))); | ||
| if (!t) throw python_error(); | ||
| for (Py_ssize_t i = 0; i < PyTuple_Size((PyObject*)self); ++i) { | ||
| PyTuple_SET_ITEM(t.get(), i, PyTuple_GET_ITEM(self, i)); |
There was a problem hiding this comment.
The reference counting doesn't match here: PyTuple_GET_ITEM returns a borrowed reference and PyTuple_SET_ITEM steals a reference.
You need to increment the reference count before PyTuple_SET_ITEM
torch/csrc/Size.cpp
Outdated
| auto module = THPObjectPtr(PyImport_ImportModule("torch")); | ||
| if (!module) throw python_error(); | ||
|
|
||
| THPObjectPtr obj(PyObject_GetAttrString(module, "Size")); |
There was a problem hiding this comment.
I think obj is just THPSizeType. You'll need to cast to a PyObject* and increment the reference count.
torch/csrc/Size.cpp
Outdated
| for (Py_ssize_t i = 0; i < PyTuple_Size((PyObject*)self); ++i) { | ||
| PyTuple_SET_ITEM(t.get(), i, PyTuple_GET_ITEM(self, i)); | ||
| } | ||
| PyTuple_SET_ITEM(ret.get(), 1, Py_BuildValue("(O)", t.release())); |
There was a problem hiding this comment.
I think this should be t.get() instead of t.release(), otherwise it leaks t. (Py_BuildValue has normal reference counting semantics -- it increments the reference count of the value).
I think you should also check the return value of Py_BuildValue here before calling PyTuple_SET_ITEM.
ailzhang
force-pushed
the
fix_size_pickle
branch
from
dfb76ea to
d0c3a54
Compare
|
Hi @colesbury , I updated the PR following the suggested changes, would you mind taking another look when you have time? Thanks! |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
colesbury
left a comment
colesbury
left a comment
There was a problem hiding this comment.
Looks good. See inline comment
torch/csrc/Size.cpp
Outdated
| auto ret = THPObjectPtr{PyTuple_New(2)}; | ||
| if (!ret) throw python_error(); | ||
|
|
||
| auto module = THPObjectPtr(PyImport_ImportModule("torch")); |
There was a problem hiding this comment.
These two lines are not used. I think they can be deleted
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
6 participants
fixes #20823