-
Notifications
You must be signed in to change notification settings - Fork 814
pybind11 pickle support for experimental vocab, vectors, regex_tokenizer #1085
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1085 +/- ##
=======================================
Coverage 77.54% 77.54%
=======================================
Files 45 45
Lines 3086 3086
=======================================
Hits 2393 2393
Misses 693 693 Continue to review full report at Codecov.
|
| .def("get_stoi", &Vocab::get_stoi) | ||
| .def("get_itos", &Vocab::get_itos); | ||
| .def("get_itos", &Vocab::get_itos) | ||
| .def(py::pickle( |
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.
Maybe we can use the existing _set_vocab_states etc. functionality
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.
Similar to def_pickle func of torchbind, I tried _set_vectors_states and _get_vectors_from_states in py::pickle func of pybind11. However, the intrusive_ptr holder used by _set_vectors_states and _get_vectors_from_states are not supported in pybind11 pickle mechanism.
error: static assertion failed: pybind11::init(): init function must return a compatible pointer, holder, or value
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 think you could change
_set_vectors_states(const c10::intrusive_ptr<Vectors> &self)
to
_set_vectors_states(const Vectors &self)
and then change the callsite of _set_vectors_states(self) to _set_vectors_states(*self).
You could even use move semantics
Add pybind11 pickle support for
No need to call
to_ivaluebefore saving.