Skip to content
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

Class variables #220

Open
hynek opened this issue Jul 22, 2017 · 51 comments
Open

Class variables #220

hynek opened this issue Jul 22, 2017 · 51 comments
Labels

Comments

@hynek
Copy link
Member

hynek commented Jul 22, 2017

Every now and then I stumble into wanting to have class variables that are part of everything (e.g. repr) except __init__.

We can do:

@attr.s
class C:
    x = attr.ib(default="42", init=False)

but that seems wasteful.

It would be nice to have something like:

@attr.s
class C:
    x = attr.ib_class(42)

that just promotes the value into x in class level and adds an Attribute to attrs's bookkeeping.


Opinions?

@Tinche
Copy link
Member

Tinche commented Jul 22, 2017

Somehow this doesn't sit right with me for reasons difficult to articulate before my brain processes this a little more.

A class variable wouldn't be part of __init__, __hash__ or __eq__ right?

@hynek
Copy link
Member Author

hynek commented Jul 23, 2017

Well that’s the big question how to actually treat them. You can overwrite class variables in Python too and then they become instance variables:

>>> class C:
...     x = 42
>>> i = C()
>>> i.x = 23
>>> C.x
42
>>> i.x
23

Maybe all I need is an option that pushes the value on the class and otherwise works as usual?

class C:
   x = attr.ib(default=42, class_var=True)

which would be equivalent to:

class C:
    x = 42

but otherwise it’d be a regular attrs attribute? I like this one better indeed. 🤔

@Tinche
Copy link
Member

Tinche commented Jul 23, 2017

Ah, I see. This technique of using a class variable as a default fallback for an unset instance variable is interesting; I have to admit I only learned of it a few months ago. I use class variable in different ways so I wasn't sure what use case you had in mind, this clears it up.

This is a way of conserving memory, right? Instead of a million instance dicts having a key set, the key is only in the class dict, but it can be overridden on a particular instance if needed.

The first thing that comes to mind is slot classes can't really support this, and they are generally the more memory-efficient option. Of course if your class has a hundred default fields, doing it this way is going to be more efficient than a slot class with a hundred slots, so it's worth considering.

These attributes must have a default, the class must be non-slots, and they would be handled differently in two ways:

  • attr.s sets the actual class variable to the given default
  • in the generated __init__, if the value is not provided, do nothing.

All the other generated methods can continue using self.x as if there's no difference. What about factories? If the factory takes self, that's invalid too I think.

Since these attributes must have defaults, what about this for syntax:

@attr.s
class A:
    x = attr.ib(default=class_var(42))

(if not, your suggestion with default=42, class_var=True is good.

@wsanchez
Copy link

wsanchez commented Aug 2, 2017

It seems like overloading attr.ib, versus adding a new attr.ib_class may become increasingly problematic. It's already clear here that there are different behaviors in play here, so a lot of arguments and options that attr.ib accepts are not compatible with the class variable behavior.

For example, if attr.s sets x to 42 on the class, then the Attribute object is gone, right? So adding a validator would not work, but you might think that's a thing you want, given that a use case noted about is that you might want to set the value on an instance of the class… But my point is that validator= may not make sense there, and you'd end up with some switching logic in attr.ib that may unnecessarily complicate it.

@hynek
Copy link
Member Author

hynek commented Aug 2, 2017

Just quickly:

  • Attributor access on the class body is deprecated and will be relived this year anyway.
  • Class attributes can be overwrites within instances and not affect the class which makes them kind of useful and doesn't preclude validators.

However they're still pretty special so I tend to an own attr.ib_class still.

@Tinche
Copy link
Member

Tinche commented Aug 2, 2017

For example, if attr.s sets x to 42 on the class, then the Attribute object is gone, right?

This is already deprecated behavior. C.x will either be a slot member descriptor (a Python thing) if the class is a slot class, or an AttributeError if the class is a dict-class. I think we're already out of the depreciation period, so the next release should be OK for messing with C.x? (The canonical way of getting to the Attribute is attr.fields(C).x.)

So adding a validator would not work [...]

It'll work.

This is the equivalent of what is being proposed:

class C:
    x = 42

    def __init__(self, x=NOTHING):
        if x is not NOTHING:
            self.x = x
            __attr_validator(self.x)

Then you get:

>>> C()
C(x=42)

with C().__dict__ being empty, which is the point.

@wsanchez
Copy link

wsanchez commented Aug 2, 2017

OK, y'all in deeper in the black magic that I've normally gone, so my assumptions are off. Uh… thanks for doing that do I don't have to. :-)

@wsanchez
Copy link

wsanchez commented Aug 2, 2017

There's a bit you left out in the code you say this is equivalent to, which is what happens when you say:

>>> c = C()
>>> c.x = "some value"

I'm assuming "magic, duh", but noting it for completeness.

@wsanchez
Copy link

wsanchez commented Aug 2, 2017

Oh, I guess for more complete completeness:

C.x = "another value"

Which I assume is out of the attrs problem scope…?

@Tinche
Copy link
Member

Tinche commented Aug 2, 2017

Well Hynek kinda explained it in #220 (comment). This is just normal Python, feel free to copy/paste the code, play around with it and discover exactly how it works ;)

@hynek
Copy link
Member Author

hynek commented Aug 8, 2017

@Tinche deprecation period ends 2017-08-30…can’t wait!

Originally, I wanted to get 17.3 out asap but I think I'll wait for this to pass. Guess I’ll be releasing out of South Africa… :)

@chadrik
Copy link
Contributor

chadrik commented Aug 29, 2017

This is already deprecated behavior. C.x will either be a slot member descriptor (a Python thing) if the class is a slot class, or an AttributeError if the class is a dict-class.

If C.x will no longer hold an Attribute I would expect the next best thing for it to hold would be the default value. This would also play nicely with static type checking, where both C.x and c.x are expected to be the same type. Also, it's a common idiom in static type checking to define attribute defaults and their types at the class level.

Where can I catch up on the discussion surrounding this proposal?

@hynek
Copy link
Member Author

hynek commented Aug 29, 2017

Which discussion exactly? The removal of Attribute will happen before the next release FWIW.

And yep, we probably could just pass immutable default values thru into class variables? I wonder if that’d have any weird side-effects, because otherwise it'd be a nice speed boost.

(P.S. as a general disclaimer: I'll be off the grid Sep 1–17 plus on vacation until Sep 24. Right now I’m having hell at work due to preparations for that so I’m less responsive than I’d like to be and it’ll be worse in the near future. OTOH I hope I’ll get a bunch of FOSS work done afterwards because I’ll be a kind of retreat until Oct 18)

@chadrik
Copy link
Contributor

chadrik commented Aug 29, 2017

Which discussion exactly? The removal of Attribute will happen before the next release FWIW.

I was just wondering if there was a publicly-viewable discussion (or code) around the removal of Attribute access from attrs classes, in favor of the new (descriptor-based?) design. It sounds like the plan has been fairly well fleshed out, so I thought I'd do you the favor of getting caught up before jumping in with suggestions that you've already thought of.

@hynek
Copy link
Member Author

hynek commented Aug 29, 2017

It happened pretty much exactly a year ago by that decision is orthogonal to this ticket. It was just a bad idea to attach more than necessary to classes. There's also no real plans around descriptors. Current plan is to just delete the attr.ibs and leave the class clean (or for slots classes: not attaching anything in the first place.)

@hynek hynek mentioned this issue Nov 4, 2017
@agronholm
Copy link

So can someone explain why we should include class variables in the attrs system?

@agronholm
Copy link

agronholm commented Nov 4, 2017

Quoting Hynek from the other thread:

In non-slots you can overwrite class variables and they become instance variables, see #220 for a discussion. It kind of gives you “free” defaults.

Yes, but this still doesn't explain why the class vars should be attr()ibuted. They work quite well without it.

@hynek
Copy link
Member Author

hynek commented Nov 4, 2017

You’re definitely on to something. We could argue whether or not attrs should support it at all, however I think you’re making a good case to make it opt-in.

IOW, the API would go more like:

@attr.s(auto_attribs)
class C:
    x: ClassVar[int] = attr.ib(default=42)

because collecting it would be impossible to prevent undesired class vars to be collected without setting it to attr.ib() which is a bad API.


The important point here is that that would make #220 not a blocker for 17.3 anymore.

@hynek
Copy link
Member Author

hynek commented Nov 4, 2017

Yes, but this still doesn't explain why the class vars should be attr()ibuted. They work quite well without it.

If it’s just a free default, you def want it as part of your reprs and cmps because they can vary wildly.

@agronholm
Copy link

I don't think ClassVar and "free" defaults should be mixed. If you want this, why not just have attrs set the default value directly in the class?

@agronholm
Copy link

Or at least give the user the option to do so.

@agronholm
Copy link

To my understanding (and in my opinion), something annotated with ClassVar is never intended to be set as an instance variable. Otherwise you could just do foo: int = 4.

@hynek
Copy link
Member Author

hynek commented Nov 4, 2017

That was kind of the original plan of this ticket. :)

But yeah, I think we’re in agreement, that this is not a blocker for 17.3 in any way. phew

@agronholm
Copy link

Are we in agreement about how to handle ClassVars with auto_attribs=True then?

@hynek
Copy link
Member Author

hynek commented Nov 4, 2017

I think so. :)

@sobolevn
Copy link
Contributor

sobolevn commented Aug 4, 2019

We now have ClassVar type in typing: https://docs.python.org/3/library/typing.html#typing.ClassVar

It would be awesome to have this clean API:

@attr.s(auto_attribs=True)
class MyClass(object):
    class_var: ClassVar[int]

    instance_var: str

Will result in:

  1. MyClass.class_var as a typed class variable
  2. MyClass().instance_var as a typed instance variable

@hynek
Copy link
Member Author

hynek commented Aug 6, 2019

Isn't that exactly what happens? You just have to assign it a value:

In [7]: @attr.s(auto_attribs=True)
   ...: class MyClass(object):
   ...:     class_var: ClassVar[int] = 5
   ...:
   ...:     instance_var: str
   ...:

In [8]: MyClass.class_var
Out[8]: 5

In [9]: MyClass("hi").class_var
Out[9]: 5

Fields decorated as ClassVar are ignored.

@kryft
Copy link

kryft commented Sep 24, 2019

@hynek Related to this, would it make sense for the following to produce an error?

@attr.s(auto_attribs=True)
class MyClass(object):
    class_var = 5
    instance_var: str = 'foo'

Currently (in attrs 19.1.0) class_var becomes a class variable and instance_var becomes an instance variable, which seems a little unintuitive and a potential source of bugs, since accidentally forgetting a type annotation will give you a class variable instead of an instance variable. I just started using attrs and was confused by this for a while.

I think it would make sense for auto_attribs=True to require the explicit ClassVar[int] annotation, just like it requires attr.ib fields to be type-annotated.

@hynek
Copy link
Member Author

hynek commented Sep 25, 2019

There's two things that's speak against that:

  1. Totally wrecks backward compatibility :)
  2. I find this against attrs's philosophy of staying out of your business unless told otherwise.

@RonnyPfannschmidt
Copy link

would a warning when a class variable is not annotated as such be in the game of ensuring people correctly annotate intent

@hynek
Copy link
Member Author

hynek commented Sep 25, 2019

Yeah, that sounds fair.

@kryft
Copy link

kryft commented Sep 25, 2019

There's two things that's speak against that:

1. Totally _wrecks_ backward compatibility :)

2. I find this against attrs's philosophy of staying out of your business unless told otherwise.

Ah, I see. The suggested warning sounds fine then. :)

Maybe it would also be good to clarify the current behavior somehow in the API documentation? The section on auto_attribs currently says

If True, collect PEP 526-annotated attributes (Python 3.6 and later only) from the class body.

In this case, you must annotate every field. If attrs encounters a field that is set to an attr.ib() but lacks a type annotation, an attr.exceptions.UnannotatedAttributeError is raised. Use field_name: typing.Any = attr.ib(...) if you don’t want to set a type.

If you assign a value to those attributes (e.g. x: int = 42), that value becomes the default value like if it were passed using attr.ib(default=42). Passing an instance of Factory also works as expected.

Attributes annotated as typing.ClassVar are ignored.

It says that you must annotate every field, which is why I was puzzled to see that it's fine to leave something unannotated, and that it will give you a class variable instead of an instance variable with a default value. I realize that this is how you normally initialize a class variable in a Python class declaration, but it didn't occur to me that that might be the intended behavior here.

I confess that I also didn't understand the last line correctly at first. Maybe something like "Attributes annotated as typing.ClassVar are ignored (that is, they will become class variables)" could be considered? I know the typing.ClassVar should be a hint, but I can't be the only dense person out there. ;)

@hynek
Copy link
Member Author

hynek commented Oct 1, 2019

I have changed the wording to:

Attributes annotated as typing.ClassVar, and attributes that are neither annotated nor set to an attr.ib are ignored.

I hope that clarifies it.

@Arcitec
Copy link

Arcitec commented Oct 2, 2019

@hynek Hi! :-) Can you please summarize the correct way to set class variables?

I need a normal python shared class variable which is totally ignored by attrs.

Is this right? Will attrs understand that ClassVar[Optional[int]] is a ClassVar?

from typing import ClassVar, Optional

import attr
import numpy as np

@attr.s(slots=True)
class ScreenGrabber:
    connection: ClassVar[Optional[int]] = None
    img = attr.ib(type=np.ndarray)

    def getConnection(self) -> int:
        if ScreenGrabber.connection is None:
            ScreenGrabber.connection = some stuff
        return ScreenGrabber.connection

@hynek
Copy link
Member Author

hynek commented Oct 2, 2019

Will attrs understand that ClassVar[Optional[int]] is a ClassVar?

It should – does it not work?

@Arcitec
Copy link

Arcitec commented Oct 2, 2019

@hynek I don't know... How can I check?

I'm looking at print(inspect.signature(ScreenGrabber.__init__)) and getting (self, img: numpy.ndarray) -> None. Is there some other way I can check that attrs has fully ignored that variable?

Also: I tried: x = ScreenGrabber(np.zeros([1,1,3], np.uint8)); print(x.__slots__) and get ('img', '__weakref__'). And if I print(x) I get this __repr__ result: ScreenGrabber(img=array([[[0, 0, 0]]], dtype=uint8))

Is this conclusive proof that the "connection" variable is totally ignored by attr and isn't stored inside the individual instances? Is it also proof that __slots__ is working properly and not storing connection in the per-instance variables?

@Arcitec
Copy link

Arcitec commented Oct 2, 2019

Also, if I tried x.connection = 5 outside the class or self.connection = 5 inside the class (in a function), I get AttributeError: 'ScreenGrabber' object attribute 'connection' is read-only in both cases.

So then I did the final test:

from typing import ClassVar, Optional

import attr
import numpy as np

@attr.s(slots=True)
class ScreenGrabber:
    connection: ClassVar[Optional[int]] = None
    img = attr.ib(type=np.ndarray)

    def getConnection(self) -> int:
        if ScreenGrabber.connection is None:
            ScreenGrabber.connection = 2
        return ScreenGrabber.connection

x = ScreenGrabber(np.zeros([1,1,3], np.uint8))
print(x.connection)
print(x.getConnection())
print(x.connection)
x.connection = 3

Result:

None
2
2
AttributeError: 'ScreenGrabber' object attribute 'connection' is read-only

So it seems like it is indeed only stored as a single, shared class variable, with no per-instance value. Can I safely put this worry to rest now? :D

@Arcitec
Copy link

Arcitec commented Oct 2, 2019

I just noticed that I did all my tests with just slots=True, and did not use auto_attribs (which according to http://www.attrs.org/en/stable/api.html is required for reading types from attribute annotations)...

Anyway, I retried the test as follows (with a few extra print-statements included) and got the exact same results as described above... And I tried the below test with auto_attribs=False too. Same result!

from typing import ClassVar, Optional

import attr
import numpy as np
import inspect

@attr.s(slots=True, auto_attribs=True)
class ScreenGrabber:
    connection: ClassVar[Optional[int]] = None
    img: np.ndarray = attr.ib()

    def getConnection(self) -> int:
        if ScreenGrabber.connection is None:
            ScreenGrabber.connection = 2
        return ScreenGrabber.connection

print(inspect.signature(ScreenGrabber.__init__))
x = ScreenGrabber(np.zeros([1,1,3], np.uint8))
print(x)
print(x.__slots__)
print(x.connection)
print(x.getConnection())
print(x.connection)
x.connection = 3

Result:

(self, img: numpy.ndarray) -> None
ScreenGrabber(img=array([[[0, 0, 0]]], dtype=uint8))
('img', '__weakref__')
None
2
2
AttributeError: 'ScreenGrabber' object attribute 'connection' is read-only

PS: It's clear that attrs reads the type annotations even when auto_attribs=False. So I don't see the purpose of that flag at all...

@euresti
Copy link
Contributor

euresti commented Oct 2, 2019

It's clear that attrs reads the type annotations even when auto_attribs=False.

The point of auto_attribs=True is to not have to say = attrib() on every attribute.

@attr.s(slots=True, auto_attribs=False)
class Example:
   clsvar = 15

    x = attrib()
    y = attrib()
    z = attrib(True)
@attr.s(slots=True, auto_attribs=True)
class Example:
   clsvar: ClassVar[str] = 15

    x: int
    y: str
    z: bool = True

So it only looks like it's reading it, but in truth it's ignoring them.

@Arcitec
Copy link

Arcitec commented Oct 2, 2019

@euresti Ah yes I see the thing now! xyz: ClassVar-annotated variables are ignored even when auto_attribs=False. But all other variables need auto_attribs if you use Python annotations for them.

Example:

from typing import ClassVar
import inspect

import attr

@attr.s(slots=True, auto_attribs=False)
class Example:
    clsvar: ClassVar[int] = 15

    x: int
    y: str
    z: bool = True
    a = attr.ib(type=int)

print(inspect.signature(Example.__init__))

Result:

(self, a: int) -> None

And with auto_attribs=True instead (slight tweaks to variables too):

from typing import ClassVar
import inspect

import attr

@attr.s(slots=True, auto_attribs=True)
class Example:
    clsvar: ClassVar[int] = 15

    x: int
    y: str
    z: bool
    a: int = attr.ib(kw_only=True)

print(inspect.signature(Example.__init__))

Result:

(self, x: int, y: str, z: bool, *, a: int) -> None

@Arcitec
Copy link

Arcitec commented Oct 2, 2019

Well, that answers it... the attr library always properly ignores xyz: ClassVar-annotated variables and lets them become a global class variable instead (no matter what auto_attribs is set to).

And if slots=True is used, attr doesn't even reserve a slot for the classvar, which means that there's (that's good btw) no way to write a class-instance specific value (self.nameofclassvar = my private value) for the shared variable. :-) So having slots helps truly ensure that the class variable is never screwed up per-instance. But that's beside the original point of asking if attr properly ignores class vars. It does! :-)

@Arcitec
Copy link

Arcitec commented Oct 6, 2019

Okay, here's the actual attrs code:

attrs/src/attr/_make.py

Lines 324 to 342 in 754fae0

elif auto_attribs is True:
ca_names = {
name
for name, attr in cd.items()
if isinstance(attr, _CountingAttr)
}
ca_list = []
annot_names = set()
for attr_name, type in anns.items():
if _is_class_var(type):
continue
annot_names.add(attr_name)
a = cd.get(attr_name, NOTHING)
if not isinstance(a, _CountingAttr):
if a is NOTHING:
a = attrib()
else:
a = attrib(default=a)
ca_list.append((attr_name, a))

attrs/src/attr/_make.py

Lines 274 to 282 in 754fae0

def _is_class_var(annot):
"""
Check whether *annot* is a typing.ClassVar.
The string comparison hack is used to avoid evaluating all string
annotations which would put attrs-based classes at a performance
disadvantage compared to plain old classes.
"""
return str(annot).startswith(_classvar_prefixes)

_classvar_prefixes = ("typing.ClassVar", "t.ClassVar", "ClassVar")

So yeah, if any annotation starts with those string sequences, the attribute is totally ignored by the attrs parser.

Edit: But this "is class var? ignore" is only executed if auto_attrs = True. So I dunno how come slots=True, auto_attribs=False also fully ignores ClassVar... Oh well... I'm thinking of moving all my code to auto_attribs=True anyway... #582

@micklat
Copy link

micklat commented Nov 19, 2019

Let me point out that the current behavior works well for me (with no warning for non-annotated class variables). That's exactly what I want. Having to annotate a class-level constant is inelegant - the value is there and it's in all caps, so the intent is very clear to the humans.

For instance:

@attrs(auto_attribs=True)
class FooInfo:
  SUFFIX = '.foo'
  path : Path
  entries : List[str]

Yeah, it wouldn't be terrible if I had to annotate SUFFIX with ClassVar, but it wouldn't look good or make the code clearer. And if I had lots of constants in there, it would be quite annoying.

@wsanchez
Copy link

@micklat Point taken about your preference, bur I don't agree that ALL CAPS implies class variable. Nothing about SUFFIX in your example tells me that you mean for it to be a class variable, so it's not clear to all humans. :-)

@micklat
Copy link

micklat commented Nov 22, 2019

@wsanchez It's a bit more than my personal preference. ALL_CAPS_WITH_UNDERSCORES is the idiomatic way to write constants in the python community, as PEP8 attests. Since a reader accustomed to idiomatic code would understand that SUFFIX is a constant, it would imply that it is a class-level constant, since there's no point in replicating a binding for a constant into each of the instances.

@wsanchez
Copy link

@micklat: Constant != Class Variable.

Perhaps it's obvious to you in this case, but I disagree that it's very clear to others. In any case, not all class variables are constants, and not all constants are named in all caps; PEP8 is convention, not law. But while we're being Pythonic: explicit is better than implicit.

@txemi
Copy link

txemi commented Oct 21, 2022

I would like to see this implemented, now I am using this wasteful use:

@attr.s
class C:
    x = attr.ib(default="42", init=False)

because I do not have this

@attr.s
class C:
    x = attr.ib_class(42)

or similar and I am not sure next is explicit or regular enough

@attr.define(auto_attribs=True)
class C:
    x = 543

with attr.ib_class or similar I suppose we could have same validators as attr.ib and more.

@hynek
Copy link
Member Author

hynek commented Nov 21, 2022

you can have:

@attrs.define
class C:
   x: ClassVar[int] = 543

which looks terse and idiomatic enough to me.

@Tinche
Copy link
Member

Tinche commented Nov 21, 2022

Lol seeing this thread in my inbox provided me with an amusing history retrospective.

@hynek How about we also make final fields classvars?

@define
class C:
    x: Final = 50

There's no way otherwise to have a Final classvar (Final[ClassVar] and ClassVar[Final] are disallowed).

@hynek
Copy link
Member Author

hynek commented Nov 23, 2022

if you say it makes sense… but it would be Final[int], right?

@Tinche
Copy link
Member

Tinche commented Nov 23, 2022

Yes, but Mypy and Pyright are smart enough. (The inferred type will actually be Final[Literal[50]] which is even better.)

We'll need to change the Mypy plugin (I can do that), as this would technically be an incompatible change. If there is any of this in the wild, I'd imagine people just use C() (letting the default provide the value) instead of C(50) anyway in order to not repeat themselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests