After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 428732 - pickling GEnums
pickling GEnums
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: gobject
2.12.x
Other All
: Normal minor
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2007-04-11 18:56 UTC by Phil Dumont
Modified: 2008-04-02 20:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to add __reduce__ method to GEnum (1.01 KB, patch)
2007-04-11 18:57 UTC, Phil Dumont
none Details | Review
Attempt at a simple test. (224 bytes, application/x-python)
2007-04-11 20:43 UTC, Phil Dumont
  Details
More detailed test that shows exactly what fails and how (637 bytes, application/x-python)
2007-04-11 20:44 UTC, Phil Dumont
  Details
Show problem with patch/pickle (299 bytes, application/x-python)
2007-04-11 20:54 UTC, Phil Dumont
  Details
A simple test of pickling/unpickling GEnums (361 bytes, application/x-python)
2008-03-27 20:25 UTC, Phil Dumont
  Details
patch fixing everything (it seems) (2.44 KB, patch)
2008-04-01 21:57 UTC, Paul Pogonyshev
none Details | Review
fixes patch + test (3.21 KB, patch)
2008-04-02 20:06 UTC, Paul Pogonyshev
committed Details | Review

Description Phil Dumont 2007-04-11 18:56:36 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.
Comment 1 Phil Dumont 2007-04-11 18:57:44 UTC
Created attachment 86193 [details] [review]
patch to add __reduce__ method to GEnum
Comment 2 Johan (not receiving bugmail) Dahlin 2007-04-11 18:59:38 UTC
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?
Comment 3 Phil Dumont 2007-04-11 20:40:43 UTC
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.
Comment 4 Phil Dumont 2007-04-11 20:43:46 UTC
Created attachment 86197 [details]
Attempt at a simple test.
Comment 5 Phil Dumont 2007-04-11 20:44:34 UTC
Created attachment 86198 [details]
More detailed test that shows exactly what fails and how
Comment 6 Johan (not receiving bugmail) Dahlin 2007-04-11 20:50:11 UTC
(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.
Comment 7 Phil Dumont 2007-04-11 20:54:30 UTC
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...
Comment 8 Phil Dumont 2007-04-11 20:57:01 UTC
> 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).
Comment 9 Phil Dumont 2007-04-11 21:03:14 UTC
(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.)
Comment 10 Johan (not receiving bugmail) Dahlin 2007-04-11 21:06:01 UTC
(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.
Comment 11 Paul Pogonyshev 2008-03-26 19:05:35 UTC
Can you test with latest SVN?  I recently committed a (different) fix for PyGObject enum pickling problem and it seems to work fine now.
Comment 12 Phil Dumont 2008-03-27 20:23:02 UTC
(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.
Comment 13 Phil Dumont 2008-03-27 20:25:50 UTC
Created attachment 108136 [details]
A simple test of pickling/unpickling GEnums
Comment 14 Paul Pogonyshev 2008-03-28 18:18:48 UTC
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):
  • File "<stdin>", line 1 in <module>
  • File "/usr/python2.5/lib/python2.5/pickle.py", line 1374 in loads
    return Unpickler(file).load()
  • File "/usr/python2.5/lib/python2.5/pickle.py", line 858 in load
    dispatch[key](self)
  • File "/usr/python2.5/lib/python2.5/pickle.py", line 1083 in load_newobj
    obj = cls.__new__(cls, *args)
TypeError: __enum_values__ badly formed

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.
Comment 15 Paul Pogonyshev 2008-03-28 18:34:03 UTC
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...
Comment 16 Phil Dumont 2008-03-31 14:40:29 UTC
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?
Comment 17 Paul Pogonyshev 2008-04-01 20:05:50 UTC
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.
Comment 18 Johan (not receiving bugmail) Dahlin 2008-04-01 20:11:20 UTC
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)
Comment 19 Paul Pogonyshev 2008-04-01 21:57:01 UTC
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?
Comment 20 Johan (not receiving bugmail) Dahlin 2008-04-01 22:22:59 UTC
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.
Comment 21 Phil Dumont 2008-04-02 12:37:59 UTC
> 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.
Comment 22 Paul Pogonyshev 2008-04-02 20:06:18 UTC
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.
Comment 23 Paul Pogonyshev 2008-04-02 20:18:29 UTC
Sending        ChangeLog
Sending        gobject/pygenum.c
Sending        tests/test_enum.py
Transmitting file data ...
Committed revision 756.