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

Sometimes psets don't compare correctly with sets or frozensets #27

Closed
itamarst opened this Issue Feb 26, 2015 · 12 comments

Comments

Projects
None yet
2 participants
@itamarst
Contributor

itamarst commented Feb 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@tobgu

tobgu Feb 26, 2015

Owner

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.

Owner

tobgu commented Feb 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@itamarst

itamarst Feb 26, 2015

Contributor

So the issue is versions of Python < 2.7.8 basically?

Contributor

itamarst commented Feb 26, 2015

So the issue is versions of Python < 2.7.8 basically?

@itamarst

This comment has been minimized.

Show comment
Hide comment
@itamarst

itamarst Feb 26, 2015

Contributor

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.
Contributor

itamarst commented Feb 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@itamarst

itamarst Feb 26, 2015

Contributor

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.

Contributor

itamarst commented Feb 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@tobgu

tobgu Feb 26, 2015

Owner

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!

Owner

tobgu commented Feb 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@itamarst

itamarst Feb 26, 2015

Contributor

I'll think about it :(

Contributor

itamarst commented Feb 26, 2015

I'll think about it :(

@itamarst

This comment has been minimized.

Show comment
Hide comment
@itamarst

itamarst Feb 26, 2015

Contributor

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?

Contributor

itamarst commented Feb 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@tobgu

tobgu Feb 26, 2015

Owner

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

Owner

tobgu commented Feb 26, 2015

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

@itamarst

This comment has been minimized.

Show comment
Hide comment
@itamarst

itamarst Feb 27, 2015

Contributor
  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__
Contributor

itamarst commented Feb 27, 2015

  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

This comment has been minimized.

Show comment
Hide comment
@tobgu

tobgu Feb 27, 2015

Owner

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.

Owner

tobgu commented Feb 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@itamarst

itamarst Feb 27, 2015

Contributor

Ugh, missed that when skimming the patch, sorry.

Contributor

itamarst commented Feb 27, 2015

Ugh, missed that when skimming the patch, sorry.

itamarst added a commit to ClusterHQ/flocker that referenced this issue Mar 2, 2015

tomprince pushed a commit to ClusterHQ/flocker that referenced this issue Mar 19, 2015

@tobgu

This comment has been minimized.

Show comment
Hide comment
@tobgu

tobgu Apr 3, 2015

Owner

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.

Owner

tobgu commented Apr 3, 2015

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.

@tobgu tobgu closed this Apr 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment