Sometimes psets don't compare correctly with sets or frozensets

[itamarst]

My branch adding a bunch of pyrsistent 0.9 usage is failing consistently on our build machines - but not on my local desktop. Sample failure:

twisted.trial.unittest.FailTest: not equal:
a = [frozenset([<Application(name=u'mysql-clusterhq', image=<object object at 0x7ffacf447ba0>, ports=frozenset([]), volume=None, links=frozenset([]), environment=None, memory_limit=None, cpu_shares=None, restart_policy=<RestartNever()>)>,
            <Application(name=u'site-clusterhq.com', image=<object object at 0x7ffacf447bb0>, ports=frozenset([]), volume=None, links=frozenset([]), environment=None, memory_limit=None, cpu_shares=None, restart_policy=<RestartNever()>)>]),
 u'example.com']
b = [pset([<Application(name=u'site-clusterhq.com', image=<object object at 0x7ffacf447bb0>, ports=frozenset([]), volume=None, links=frozenset([]), environment=None, memory_limit=None, cpu_shares=None, restart_policy=<RestartNever()>)>, <Application(name=u'mysql-clusterhq', image=<object object at 0x7ffacf447ba0>, ports=frozenset([]), volume=None, links=frozenset([]), environment=None, memory_limit=None, cpu_shares=None, restart_policy=<RestartNever()>)>]),
 u'example.com']

Those two seem like they ought to be equal, and on my computer they are. And yet. (Application is a class using the characteristic library; eventually we'll probably switch everything to pyrsistent).

Full set of failures:
http://build.clusterhq.com/builders/flocker-ubuntu-14.04/builds/1272/steps/trial/logs/problems

[tobgu]

Suprising at first. Seems like the comparison fails when you put the set/frozenset to the left of the equals sign because the set/frozenset does not delegate the comparison to the pset as it should. Browsed the C-code for the python set and found that the rich compare behavior has been changed (fixed) in recent releases of 2.7.

The bug is described here:
http://bugs.python.org/issue8743

A fix was released in Python 2.7.8 (I guess you run this or 2.7.9 locally) as seen here https://hg.python.org/cpython/raw-file/v2.7.8/Misc/NEWS.

I will have a hard time implementing a workaround for it unless I make the PSet inherit from set. I don't want to do that. I would recommend an upgrade of python. :-)

Thanks for reporting. I'll make a note about it for all poor 2.6 users.

[itamarst]

So the issue is versions of Python < 2.7.8 basically?

[itamarst]

I don't think it's realistic to require minimum of 2.7.8:

• PyPy is still on older version, though perhaps this doesn't impact PyPy.
• LTS OSes like Ubuntu 14.04 and CentOS 7 are on 2.7.5. People are going to be using those, especially the latter, for years to come.

[itamarst]

From some comments in the code I assume specifically you don't want to inherit from collections.Set, not built-in set? I would personally trade "works on most common server OSes" for "slight increase in memory usage" if that's the issue.

[tobgu]

I don't think that inheriting from collections.Set would do it (which is what I wanted to avoid due to memory consumption). You would have to inherit from set for things to work. Doing that would cause all sorts of unwanted side effects in the PSet. The only alternative as I see it (in this late hour) is to implement some sort of support function for set comparison that makes sure that any PSet is always on the lefthand side of the comparison expression but that's ugly... Better suggestions are welcome!

[itamarst]

I'll think about it :(

[itamarst]

Looks like this is a pure Python fix. So ... copy/paste fixed code from Python 2 and use it when on old versions of Python?

[tobgu]

Sorry, don't really follow... Anyway I'll give it some thought and we'll see.

[itamarst]

1. Right now you're doing e.g. __le__ = Set.__le__.
2. The patch to fix that bug (http://bugs.python.org/issue8743) involves modifying the Set class.

Thus, to fix the problem on versions where Set doesn't have the fix, just copy/paste the code from the standard library into pyrsistent, so e.g.:

if sys.version_tuple < (2, 7, 8):
    def __le__(self, other):
        # code copy/pasted from Set.__le__ in 2.7.8 here
else:
    __le__ = Set.__le__

[tobgu]

Did you try this? Did it work? Because the patch also contained an update to the C-code in setobject.c which I thought was essential.

I'll asses the consequences and possibilities of making the PSet a subclass of frozenset. That is the most reasonable resolution to this that I can come up with at this moment.

[itamarst]

Ugh, missed that when skimming the patch, sorry.

[tobgu]

No can do on this one really. Sorry. The set implementations in python make use of a lot of internal representations in the C-code that make them pretty much impossible to use as base types for the PSet. Added a section in the compatibility section to inform about the problem and provide a couple of workarounds.