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

Join.__slots__ : meaningless, according to Python documentation? #1268

Closed
kwmsmith opened this Issue Oct 10, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@kwmsmith
Member

kwmsmith commented Oct 10, 2015

From the fine manual:

"When inheriting from a class without __slots__, the __dict__ attribute of that class will always be accessible, so a __slots__ definition in the subclass is meaningless."

Looking at Join--which defines __slots__--it is a subclass of Expr and Node, neither of which define __slots__. From my reading, this makes Join.__slots__ "meaningless."

What is the intent of the usage of __slots__ in the classes in blaze.expr.collections? Space savings? An easy way to restrict attributes?

@cpcloud

This comment has been minimized.

Member

cpcloud commented Oct 10, 2015

"When inheriting from a class without __slots__, the __dict__ attribute of that class will always be accessible, so a __slots__ definition in the subclass is meaningless."

Unless you're using __slots__ in the superclass

What is the intent of the usage of __slots__ in the classes in blaze.expr.collections? Space savings? An easy way to restrict attributes?

Both. Another reason is simplification of initialization. Notice that many blaze expressions have no explicit constructor defined: these instances are constructed via the Node constructor, which uses slots.

@mrocklin

This comment has been minimized.

Member

mrocklin commented Oct 10, 2015

This decision came out of experience with sympy which had non-trivial logic within constructors. This ended up making a lot of transformations of the expression tree difficult because you were always running into complexity in the constructors.

With blaze.expr the goal was to have expression classes be restricted and dumb. Slots were used mostly as an explicit list of fields.

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Oct 10, 2015

Unless you're using slots in the superclass

I saw this, but is it doing what's expected? The docs say that "the default can be overridden by defining __slots__ in a class definition." Nowhere do Node or Expr define __slots__ in their definitions, so any subclasses would have a "meaningless" __slots__, AFAICT. It seems like using a subclass' __slots__ isn't sufficient, it has to be defined at the class level for the entire hierarchy.

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Oct 10, 2015

Another reason is simplification of initialization.

This makes sense.

@cpcloud

This comment has been minimized.

Member

cpcloud commented Oct 10, 2015

I saw this, but is it doing what's expected?

In [1]: class Foo(object):
   ...:     __slots__ = ()
   ...:     def __init__(self, *args):
   ...:         assert len(args) == len(self.__slots__)
   ...:         for arg, slot in zip(args, self.__slots__):
   ...:             setattr(self, slot, arg)
   ...:

In [2]: Foo()
Out[2]: <__main__.Foo at 0x1003b4db0>

In [3]: Foo(1)
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-3-6d641180699a> in <module>()
----> 1 Foo(1)

<ipython-input-1-40a0ac03f7ed> in __init__(self, *args)
      2     __slots__ = ()
      3     def __init__(self, *args):
----> 4         assert len(args) == len(self.__slots__)
      5         for arg, slot in zip(args, self.__slots__):
      6             setattr(self, slot, arg)

AssertionError:

In [4]: class Bar(Foo):
   ...:     __slots__ = ('a', 'b')
   ...:

In [5]: b = Bar(1, 2)

In [6]: b.a
Out[6]: 1

In [7]: b.b
Out[7]: 2
@cpcloud

This comment has been minimized.

Member

cpcloud commented Oct 10, 2015

And without __slots__ in the superclass

In [8]: class Foo(object):
    def __init__(self, *args):
        assert len(args) == len(self.__slots__)
        for arg, slot in zip(args, self.__slots__):
            setattr(self, slot, arg)
   ...:

In [9]: class Bar(Foo):
    __slots__ = ('a', 'b')
   ...:

In [10]: Foo(1)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-10-6d641180699a> in <module>()
----> 1 Foo(1)

<ipython-input-8-0020b85a287f> in __init__(self, *args)
      1 class Foo(object):
      2     def __init__(self, *args):
----> 3         assert len(args) == len(self.__slots__)
      4         for arg, slot in zip(args, self.__slots__):
      5             setattr(self, slot, arg)

AttributeError: 'Foo' object has no attribute '__slots__'

In [11]: b = Bar(1, 2)

In [12]: b
Out[12]: <__main__.Bar at 0x109238240>

In [13]: b.a
Out[13]: 1

In [14]: b.b
Out[14]: 2
@cpcloud

This comment has been minimized.

Member

cpcloud commented Oct 10, 2015

If we want to get the reduced space benefits of using __slots__ we should definitely be very strict about defining __slots__ for every subclass

@cpcloud

This comment has been minimized.

Member

cpcloud commented Oct 10, 2015

If we're going to significantly expand the expression system then we should probably add some logic in the Node constructor that raises an error if there's no __slots__ attribute defined on the class

@cpcloud

This comment has been minimized.

Member

cpcloud commented Oct 10, 2015

I plan on expanding the expression system soon enough that it would make sense to do this.

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Oct 10, 2015

@cpcloud @mrocklin thanks, this helps.

For the record, I'm all for using __slots__.

A minor question based on your example @cpcloud :

In [1]: class Foo(object):
   ...:     def __init__(self, *args):
   ...:         assert len(args) == len(self.__slots__)
   ...:         for arg, slot in zip(args, self.__slots__):
   ...:             setattr(self, slot, arg)
   ...:

In [2]: class Bar(Foo):
   ...:     __slots__ = ('a', 'b')
   ...:

In [6]: b = Bar(1, 2)

In [8]: b.a
Out[8]: 1

In [9]: b.b
Out[9]: 2

In [10]: b.__dict__
Out[10]: {}

For the __slots__-less superclass, instances--including subclassed instances--will have an empty and unused __dict__ because Foo does not define __slots__.

It makes sense to me to define an empty __slots__ in Foo (or Node and Expr) to remove this useless __dict__, if one of the goals is to be more efficient space-wise. I like defining __slots__ for the entire hierarchy (even when it's empty) for consistency's sake.

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Nov 15, 2015

Resolved by #1274.

@kwmsmith kwmsmith closed this Nov 15, 2015

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