-
-
Notifications
You must be signed in to change notification settings - Fork 9
Add support for pickling ASCIIDtype and StringDType instances #33
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
Changes from all commits
5c96050
98873db
a790e5e
457fc76
73c8f64
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 |
---|---|---|
|
@@ -226,6 +226,82 @@ static PyMemberDef ASCIIDType_members[] = { | |
{NULL}, | ||
}; | ||
|
||
static int PICKLE_VERSION = 1; | ||
|
||
static PyObject * | ||
asciidtype__reduce__(ASCIIDTypeObject *self) | ||
{ | ||
PyObject *ret, *mod, *obj, *state; | ||
|
||
ret = PyTuple_New(3); | ||
if (ret == NULL) { | ||
return NULL; | ||
} | ||
|
||
mod = PyImport_ImportModule("asciidtype"); | ||
if (mod == NULL) { | ||
Py_DECREF(ret); | ||
return NULL; | ||
} | ||
|
||
obj = PyObject_GetAttrString(mod, "ASCIIDType"); | ||
Py_DECREF(mod); | ||
if (obj == NULL) { | ||
Py_DECREF(ret); | ||
return NULL; | ||
} | ||
|
||
PyTuple_SET_ITEM(ret, 0, obj); | ||
|
||
PyTuple_SET_ITEM(ret, 1, Py_BuildValue("(l)", self->size)); | ||
|
||
state = PyTuple_New(1); | ||
|
||
PyTuple_SET_ITEM(state, 0, PyLong_FromLong(PICKLE_VERSION)); | ||
|
||
PyTuple_SET_ITEM(ret, 2, state); | ||
|
||
return ret; | ||
} | ||
|
||
static PyObject * | ||
asciidtype__setstate__(ASCIIDTypeObject *NPY_UNUSED(self), PyObject *args) | ||
{ | ||
if (PyTuple_GET_SIZE(args) != 1 || | ||
!(PyLong_Check(PyTuple_GET_ITEM(args, 0)))) { | ||
PyErr_BadInternalCall(); | ||
return NULL; | ||
} | ||
|
||
long version = PyLong_AsLong(PyTuple_GET_ITEM(args, 0)); | ||
|
||
if (version != PICKLE_VERSION) { | ||
PyErr_Format(PyExc_ValueError, | ||
"Pickle version mismatch. Got version %d but expected " | ||
"version %d.", | ||
version, PICKLE_VERSION); | ||
return NULL; | ||
} | ||
|
||
Py_RETURN_NONE; | ||
} | ||
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. Not convinced we need it, and I would try to avoid it :). 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. Are you saying we don't need to save a pickle version? I thought that was generally useful for forward compatibility, just in case we ever want to change what gets pickled in the future. 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 saying there is no point in 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. But then how do I check the pickle version when I load the pickle? 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. Yeah, if you want a version, then it is not possible. But, you don't need a version if you assume your API is stable, and if you have a loader function, you can keep the old one around also? |
||
|
||
static PyMethodDef ASCIIDType_methods[] = { | ||
{ | ||
"__reduce__", | ||
(PyCFunction)asciidtype__reduce__, | ||
METH_NOARGS, | ||
"Reduction method for an ASCIIDType object", | ||
}, | ||
{ | ||
"__setstate__", | ||
(PyCFunction)asciidtype__setstate__, | ||
METH_O, | ||
"Unpickle an ASCIIDType object", | ||
}, | ||
{NULL}, | ||
}; | ||
|
||
/* | ||
* This is the basic things that you need to create a Python Type/Class in C. | ||
* However, there is a slight difference here because we create a | ||
|
@@ -242,6 +318,7 @@ PyArray_DTypeMeta ASCIIDType = { | |
.tp_repr = (reprfunc)asciidtype_repr, | ||
.tp_str = (reprfunc)asciidtype_repr, | ||
.tp_members = ASCIIDType_members, | ||
.tp_methods = ASCIIDType_methods, | ||
}}, | ||
/* rest, filled in during DTypeMeta initialization */ | ||
}; | ||
|
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:
is good enough? presumably we can consider
ASCIIDType(length)
to be stable API? And you made the type pickable using the copyreg, so no need for the reconstruct function? Plus, if we have a reconstruction function, then we certainly can assume stable API?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.
Ah, you didn't do the copyreg for the ASCIIDtype, I think. But maybe that is nicer? For the StringDType, this seems true even more so? We can assume
StringDType()
will always give the same thing, so no need for__setstate__
logic?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 using
copyreg
at all in either dtype. It is a good point though that we don't actually need to save the dtype itself at all though, thanks for the hint.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.
Ah, I misread... somehow I thought one of the reconstructs was to make the class work. So I guess you need the reconstruct helper until we fix the class pickling.
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.
That's exactly right. I originally had it as you suggested, but that didn't work because
ASCIIDType
is an instance ofDTypeMeta
, so python checks the type, sees it's in thecopyreg
, and dutifully calls the pickler numpy registers for the type.