GNOME Bugzilla – Bug 428732
pickling GEnums
Last modified: 2008-04-02 20:18:29 UTC
Please consider adding a __reduce__ method to GEnums, that objects containing GEnums may be pickled. Yes, we actually do this. Comes in handy when serializing objects for drag-n-drop purposes. Other information: See attached patch for a possible implementation.
Created attachment 86193 [details] [review] patch to add __reduce__ method to GEnum
Comment on attachment 86193 [details] [review] patch to add __reduce__ method to GEnum Looks good. Do you think you can add a test for it?
Yow. Apparently, my patch is not ready for prime time. My attempt at a test for it shows me that the solution is not general. Apparently, there are GEnum derivatives that *don't* let you create new instances by calling the class with the integer valued argument. Failures are for a coule different reasons: 'value out of range', '__enum_values__ badly formed'. Apparently, our code never tried to pickle (or rather, unpickle) any of these. I can't really justify putting much more effort into this right now. Do what you want with this bug. If you want to fix my patch, or fix whatever is broken that's making my patch fail, great, thanks. If you want to no-change this bug, I can live with that.
Created attachment 86197 [details] Attempt at a simple test.
Created attachment 86198 [details] More detailed test that shows exactly what fails and how
(In reply to comment #3) > Yow. > > Apparently, my patch is not ready for prime time. That's fine. As long as the short comings are known. It's more important that this patch is useful for you than for it to be able to pickle and unpickle all enums available in gtk. However, if you want it to go in it should be possible to extend the picke/__reduce__ protocol for GEnums to be able to handle all possible enums available in gtk for example. > My attempt at a test for it shows me that the solution is not general. > Apparently, there are GEnum derivatives that *don't* let you create new > instances by calling the class with the integer valued argument. Failures are > for a coule different reasons: 'value out of range', '__enum_values__ badly > formed'. I was actually thinking of a test more in the line of what you're actually using the code for, something which can be integrated into the pygobject testsuite, so it cannot import gtk.
Created attachment 86199 [details] Show problem with patch/pickle By the way, you don't need my patch or the pickle module to show the strangely behaving GEnums...
> I was actually thinking of a test more in the line of what you're actually > using the code for, something which can be integrated into the pygobject > testsuite, so it cannot import gtk. > Yeah, but what we're using the code for is pickling gtk objects (composite objects containing GEnums).
(In reply to comment #8) > > I was actually thinking of a test more in the line of what you're actually > > using the code for, something which can be integrated into the pygobject > > testsuite, so it cannot import gtk. > > > > Yeah, but what we're using the code for is pickling gtk objects (composite > objects containing GEnums). > Indeed, a quick search of the gobject module shows that there are no GEnum-derived classes in gobject, nor any named GEnum instances. Just trying to pickle "gobject.GEnum(some_int)" fails because instances of the GEnum base class (not a derivative) don't have a __dict__, and my patch expects a __dict__. So, as is, we *have* to import some module that derives from GEnum to test it. (Or create the derivative in the test. But that's rather contrived.)
(In reply to comment #9) [..] > So, as is, we *have* to import some module that derives from GEnum to test it. > (Or create the derivative in the test. But that's rather contrived.) The latter, that should be done in testhelper.c, there are other tests which does the same, to test PyGObject/GObject-in-C interoperability/compatibility. Yeah I know, it's a bit of work.
Can you test with latest SVN? I recently committed a (different) fix for PyGObject enum pickling problem and it seems to work fine now.
(In reply to comment #11) > Can you test with latest SVN? I recently committed a (different) fix for > PyGObject enum pickling problem and it seems to work fine now. > No, I can't test with the latest SVN. My SVN client does not seem to want to talk to the SVN server: $ svn ls http://svn.gnome.org/svn svn: PROPFIND request failed on '/svn' svn: PROPFIND of '/svn': could not connect to server (http://svn.gnome.org) $ svn co http://svn.gnome.org/svn/pygobject/trunk pygobject svn: PROPFIND request failed on '/svn/pygobject/trunk' svn: PROPFIND of '/svn/pygobject/trunk': could not connect to server (http://svn.gnome.org) I can get there with a browser. Don't know why svn isn't working. Maybe version mismatch? I'm using the 1.4.2 svn client. Anyway, I'm attaching a script to test pickling/unpickling of GEnums. It probably wouldn't be appropriate to put in the pygobject test suite, since it uses pygtk. But it does test what I really wanted this for -- pickling some of the GEnums in pygtk. Could *you* run it again the latest SVN? If it works, I'm content. Thanks.
Created attachment 108136 [details] A simple test of pickling/unpickling GEnums
Note that your test pickles using old protocol. The fix I commited works only for the latest protocol, i.e. if you dumps(..., -1). However, there are still problems: >>> gtk.JUSTIFY_LEFT == pickle.loads (pickle.dumps (gtk.JUSTIFY_LEFT, -1)) True >>> gtk.ANCHOR_CENTER == pickle.loads (pickle.dumps (gtk.ANCHOR_CENTER, -1)) Traceback (most recent call last):
+ Trace 193551
return Unpickler(file).load()
dispatch[key](self)
obj = cls.__new__(cls, *args)
I.e. it works for some enums, yet not for others... Strange. I will look into this. Anyway, Johan, should we only fix pickling with latest protocol or make the older ones (depending on __reduce__) work too? As I see, there are no reviewed/accepted patches here.
The problem is that some enums contain numerically equal values (e.g. gtk.ANCHOR_N == gtk.ANCHOR_NORTH). They are counted in GEnumClass::n_values, but not in __enum_values__ dictionary. Therefore, n_values != len (__enum_values__) and pyg_enum_new() bails out just in case. It was probably not noticed before because pyg_enum_new() was never called before the fix :-/ I'm not sure what's best to do here...
Re: Pickle protocol: For what it's worth, I do not see the requirement to use the latest pickle protocol as an unreasonable restriction. Of course, I know you need to consider what's best for the user community in general. But my guess is that by now it would be hard to conceive of a user needing to pickle GEnum who had a legitimate reason not to be able to use the latest pickle protocol (which is not all that new any more). Re: badly formed __enum_values__: Is the assertion that n_values == len(__enum_values__) necessary or desirable? Clearly, there are cases where the assertion failure is legitimate. Are there any *real* errors that this assertion failure might catch? Could it be removed altogether?
About protocol. Now you can't pickle enums at all (using whatever protocol) and we don't have an abundance of bug reports about this. So, I guess it is not a e widely needed feature. About assertion, it looks more like sanity checks to me. Since it is not really sane (it makes false claims that n_values describes number of numerically _different_ values, and this is not true), I think this can be removed. Alternatively, we can just soften assertion: instead of if (!PyDict_Check(values) || PyDict_Size(values) != eclass->n_values) have if (!PyDict_Check(values) || PyDict_Size(values) > eclass->n_values) so that requirement on dictionary size is less strict and correct. Since I don't see how to create a dict of all values (writing a multidict just for this seems like a far-fetched overkill) and GEnumClass doesn't contain information about number of different values, I guess that's about the strictest assertion we can leave in the code.
Sounds reasonable that any pickle version should be supported. Yes, softing up that check a bit probably seems like a good idea. (In retrospect, using a dictionary to store the values might not have been that good)
Created attachment 108442 [details] [review] patch fixing everything (it seems) A patch based on Phil's original one, together with two fixes for the problems he reported. Turns out these problems were common for pickling using whatever protocol. However, to make pickling with old protocol work, we still need __reduce__, otherwise vanilla implementation tries to instantiate gobject.GEnum and that instantiation fails. Phil: I didn't quite understand why you added two trailing None's in your patch. Judging by documentation for Python, this is not needed (and actually shouldn't work ;-). Now pickling should work with whatever protocol and whatever enum, tested with attachment 108136 [details]. However, I didn't add unit tests, as they would require PyGTK. Johan: any suggestions?
There's already C code to help testing, you could register a simple enum there if you want (there's already one registered for threading tests), but I'm okay if you don't want to do that. pyg_enum_reduce(PyGEnum *self, PyObject *args) would probably be slightly cleaner if you just do: pyg_enum_reduce(PyObject *self, PyObject *args) since you can remove 3 (!) casting.
> Phil: I didn't quite understand why you added two trailing None's in your > patch. Judging by documentation for Python, this is not needed (and actually > shouldn't work ;-). Not needed: true. Shouldn't work: incorrect. They are optional. From http://docs.python.org/lib/node320.html (3rd paragraph): "When a tuple is returned, it must be between two and five elements long. Optional elements can either be omitted, or None can be provided as their value." Thanks, both Paul and Johan, for your attention to this issue. I look forward to its release. Meanwhile, I'm going to replace the patch in my local build with the one you've just attached.
Created attachment 108500 [details] [review] fixes patch + test The same patch with reduce() argument type changed to avoid casts, as Johan suggested. I also added a unit test, though cannot run it here ('runtests.py' exclude the file for some reason, but there are more local problems). However, the test passes when run manually under interpreter. Phil: indeed, I just consulted documentation of Python 2.3, there only 2 or 3 items in the tuple.
Sending ChangeLog Sending gobject/pygenum.c Sending tests/test_enum.py Transmitting file data ... Committed revision 756.