-
Notifications
You must be signed in to change notification settings - Fork 7
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
Special handling of __setstate__
to support alternative pickle mechanisms.
#30094
Changes from 6 commits
2c62f59
20a4139
53e8160
10b0796
1d7ac2f
6bc10a7
846bfbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -394,6 +394,11 @@ class cpp_function : public function { | |
// it alive. | ||
auto *rec = unique_rec.get(); | ||
|
||
// Note the "@__setstate__" manipulation below. | ||
rec->is_constructor = rec->name != nullptr | ||
&& (std::strcmp(rec->name, "__init__") == 0 | ||
|| std::strcmp(rec->name, "__setstate__") == 0); | ||
|
||
// Keep track of strdup'ed strings, and clean them up as long as the function's capsule | ||
// has not taken ownership yet (when `unique_rec.release()` is called). | ||
// Note: This cannot easily be fixed by a `unique_ptr` with custom deleter, because the | ||
|
@@ -403,7 +408,14 @@ class cpp_function : public function { | |
strdup_guard guarded_strdup; | ||
|
||
/* Create copies of all referenced C-style strings */ | ||
rec->name = guarded_strdup(rec->name ? rec->name : ""); | ||
if (rec->name == nullptr) { | ||
rec->name = guarded_strdup(""); | ||
} else if (std::strcmp(rec->name, "@__setstate__") == 0) { | ||
// See google/pywrapcc#30094 for background. | ||
rec->name = guarded_strdup(rec->name + 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider typing the string There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
} else { | ||
rec->name = guarded_strdup(rec->name); | ||
} | ||
if (rec->doc) { | ||
rec->doc = guarded_strdup(rec->doc); | ||
} | ||
|
@@ -418,9 +430,6 @@ class cpp_function : public function { | |
} | ||
} | ||
|
||
rec->is_constructor = (std::strcmp(rec->name, "__init__") == 0) | ||
|| (std::strcmp(rec->name, "__setstate__") == 0); | ||
|
||
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES) && !defined(PYBIND11_DISABLE_NEW_STYLE_INIT_WARNING) | ||
if (rec->is_constructor && !rec->is_new_style_constructor) { | ||
const auto class_name | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,11 +8,14 @@ | |
BSD-style license that can be found in the LICENSE file. | ||
*/ | ||
|
||
#include <pybind11/stl.h> | ||
|
||
#include "pybind11_tests.h" | ||
|
||
#include <memory> | ||
#include <stdexcept> | ||
#include <utility> | ||
#include <vector> | ||
|
||
namespace exercise_trampoline { | ||
|
||
|
@@ -60,6 +63,47 @@ void wrap(py::module m) { | |
|
||
} // namespace exercise_trampoline | ||
|
||
namespace exercise_getinitargs_getstate_setstate { | ||
|
||
// Mechanism similar to what was established with Boost.Python: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may want to actually describe the protocol here rather than just linking to the origin. My stab at this based on your PR description is that: Test binding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please take another look. I changed the comment here significantly. For the description of the protocol I'm pointing to https://www.boost.org/doc/libs/1_31_0/libs/python/doc/v2/pickle.html now (the page is actually from 2002). The main point is that |
||
// https://www.boost.org/doc/libs/1_58_0/libs/python/doc/v2/pickle.html | ||
// This mechanism was also adopted for PyCLIF: | ||
// https://github.com/google/clif/blob/7d388e1de7db5beeb3d7429c18a2776d8188f44f/clif/testing/python/pickle_compatibility.clif | ||
|
||
class StoreTwoWithState { | ||
public: | ||
StoreTwoWithState(int v0, int v1) : values_{v0, v1}, state_{"blank"} {} | ||
const std::vector<int> &GetInitArgs() const { return values_; } | ||
const std::string &GetState() const { return state_; } | ||
void SetState(const std::string &state) { state_ = state; } | ||
|
||
private: | ||
std::vector<int> values_; | ||
std::string state_; | ||
}; | ||
|
||
void wrap(py::module m) { | ||
py::class_<StoreTwoWithState>(std::move(m), "StoreTwoWithState") | ||
.def(py::init<int, int>()) | ||
.def("__getinitargs__", | ||
[](const StoreTwoWithState &self) { return py::tuple(py::cast(self.GetInitArgs())); }) | ||
.def("__getstate__", &StoreTwoWithState::GetState) | ||
.def( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit confused that both reduce_ex and setstate are provided in the same python class. I thought this would completely bypass setstate during unpickling? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this to the comment further up:
The BTW: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. This is to make a class that supports all different versions of the pickle protocol. My Python experience only started around 2010, so I do lack that part of background. |
||
"__reduce_ex__", | ||
[](py::handle self, int /*protocol*/) { | ||
return py::make_tuple(self.attr("__class__"), | ||
self.attr("__getinitargs__")(), | ||
self.attr("__getstate__")()); | ||
}, | ||
py::arg("protocol") = -1) | ||
.def( | ||
"@__setstate__", // See google/pywrapcc#30094 for background. | ||
[](StoreTwoWithState *self, const std::string &state) { self->SetState(state); }, | ||
py::arg("state")); | ||
} | ||
|
||
} // namespace exercise_getinitargs_getstate_setstate | ||
|
||
TEST_SUBMODULE(pickling, m) { | ||
m.def("simple_callable", []() { return 20220426; }); | ||
|
||
|
@@ -191,4 +235,5 @@ TEST_SUBMODULE(pickling, m) { | |
#endif | ||
|
||
exercise_trampoline::wrap(m); | ||
exercise_getinitargs_getstate_setstate::wrap(m); | ||
} |
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.
Is the
@
name following some precedence? If not I would suggest using a more descriptive name, e.g., introducing a namespace and some sort of binding protocol version tags, like,pybind:2:__setstate__
?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.
After turning this over in my mind I settled on
__setstate__[non-constructor]
.The ideas behind it are:
typing
annotation (good or bad?).