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

custom init method #393

Closed
sky-code opened this issue Jun 15, 2018 · 25 comments
Closed

custom init method #393

sky-code opened this issue Jun 15, 2018 · 25 comments
Labels

Comments

@sky-code
Copy link

sky-code commented Jun 15, 2018

From attrs docs:

Create a __init__ method that initializes the
attrs attributes. Leading underscores are stripped for the
argument name. If a __attrs_post_init__ method exists on the
class, it will be called after the class is fully initialized.

attrs inject init method directly to class and provide only __attrs_post_init__ hook for customization, so we can't use method overriding (inheritance), this behavior make customization of __init__ method very limited.

attrs must be able to inject __init__ method with different name, as an example:

@attrs
class Example:
    def __init__(*args, **kwargs):
       # do something before or after call
        self.__attrs_init__(*args, **kwargs) # created by attrs
        # equal to super().__init__(*args, **kwargs) in inheritance method overriding

So we can add custom logic before or after attrs init.

There are two options how this behavior can be added
1 - Add new attrs boolean parameter i.e. attrs_init or custom_init, if set to True, attrs will inject __attrs_init__ instead of default __init__ method.
2 - inject __attrs_init__ automatically if class has __init__ method.

This feature will resolve:
#342
#354

@wsanchez
Copy link

This won't resolve #342 or #354, it just creates a different way to work around them.

@hynek
Copy link
Member

hynek commented Jun 16, 2018

I am not entirely opposed to add more control to __init__ generation. I just woke up but the proposed inversion of control might even be a better approach than __attrs_post_init__. 🤔

@sky-code sky-code changed the title custom init method feature custom init method Jun 16, 2018
@simlei
Copy link

simlei commented Nov 23, 2018

I read the whole #68 thread in looking for hints on how to proceed with an attrs-styled VIMscript OO library. I like how you made the __attrs_post_init__ method only a completely-after-the-fact hook, everything else would have meant problems with going a step further, like the suggestion is in this thread. And yes, I am of the strong opinion that the attrs-generated __init__ method should be just the lightest-possible shim relegating to the real initialization business logic monster which should be appropriately far out of sight, i.e. the proposed __attrs_init__.
I don't see any drawbacks actually. Other than, you gain increased flexibility in what users can do with attrs, will attract a lot of unpleasant head-in-the-clouds FP people asking for more meta-programming-support and typing support and get pissy if you don't want to (and no, this is not in the slightest exaggerated.. ^^)

As for __attrs_post_init__, this does not actually conflict with this idea, right?

    1. you did not use __attrs_post_init__ before: no harm done.
    1. you did use it before: It is aptly named with a reference to attrs, so at least to me it would be apparent, that it would come after the attrs initialization routine, not after some __init__ I just happened to declare. You will in any case have to pass by the docs where you find explanations, see iii)
    1. post_init it works as intended as long as you don't override __init__; and if you do, without this issue's suggestion as well as with it, you would debug, go consult the docs. The difference is, without this suggestion implemented, you find out you just can't do that with attrs and will rewrite it with the post_init hook. If you now arrive at that step in consulting the docs, you find out you may proceed as you would have thought, and the behavior of __attrs_init__ as well as of __attrs_post_init__ documented side-by-side - a choice between simple hook based, and you're-in-charge-of-calling-attrs approaches, which is better as "go rethink how you'd do it with a post-init hook" IMO. Of course, I cannot deny that it is added complexity, but at least you have already established the easy case of low-complexity-workaround with that hook.

@hynek
Copy link
Member

hynek commented Nov 24, 2018

I don't see any drawbacks actually.

Just to be clear, you’re making the case for __attrs_init__ that is called from a custom __init__ written by the user?

If you now arrive at that step in consulting the docs,

Oh sweet summer child. ;)

@simlei
Copy link

simlei commented Nov 24, 2018

Yep, I am for the attrs_init.
And if you have already overridden init, not knowing the implications, with or without this change, you will only be the wiser once you consult the docs, or stackoverflow, right? But the jest is appreciated ;)

@wsanchez
Copy link

And… are we settling on the mechanism for enabling this? I think I prefer option two, where attrs notices that you've defined __init__ and therefore writes __attrs_init__ instead.

The parameter to attrs has the advantage of being explicit, but I think that defining your own __init__ is sufficiently explicit, and in that case it's rather obvious (I think) that you'd need to explicitly call attrs' generated init yourself, since you've taken over control.

@RonnyPfannschmidt
Copy link

what does this add that cant be done by say a named ctor?

@wsanchez
Copy link

@RonnyPfannschmidt: What a named constructor doesn't let you do is… change the behavior of the default constructor.

I get the argument that named constructors are cool and may be generally preferable, but that's not always an option, and attrs probably shouldn't make it impossible to use it if direct control of __init__ is a requirement (eg. porting an exciting class and you need compatibility), right?

@RonnyPfannschmidt
Copy link

@wsanchez but its still not clear to me why its valuable to have that capability - in particular its not clear if its worth the complexity

@RonnyPfannschmidt
Copy link

to elaborate more - my personal experience is that smart ctors have a tendency to eventually be too smart to sanely debug and/or refactor - im facing that issue now with pytest nodes for example

@wsanchez
Copy link

Sure, but that's your experience and YYMV.

Customization of __init__ is a pretty common thing in Python and compatibility with an API you don't necessarily control is a common requirement. I don't think this is an unreasonable request, even if I wouldn't recommend doing this in most cases.

@hynek
Copy link
Member

hynek commented Nov 29, 2018

I'm mostly with Ronny on this issue however I keep running into legit uses for this kind of behavior, especially if I need attrs classes to integrate with other stuff.

So yeah, Ronny is 💯 right, but it looks like there's enough edge cases that would warrant the complexity. Although I'd totally have to see it first.

BTW with Wilfredos suggestion this became related to #324 and #416 should be taken into account. I'm not positive we should make auto-detection opt-out since it would cause potential breakage.

It might become opt-out in “Project import attrs” but I doubt anybody is gonna step up and implement it by then. 🤔

@dojoteef
Copy link

@hynek I want to start by saying, thanks for making such a useful library! That said, I find myself in the camp where I need the ability to call the super class's __init__ before setting attributes (in conjunction with Pytorch). I've read through #68 and it seems you all did consider enabling this usecase at one point, but it got lost in the fold. I'm sure you'll continue to get asked questions about supporting it.

I came up with a workaround to the issue without needing to modify the library. It seems to support my usecase well and is reasonably flexible. I figured I would at least document it in this comment in case others want a similar workaround (or in case it inspires a solution that can be incorporated into attrs). Here's a minimal working example showing the approach in action:

from attr import attrs, attrib


def attrs_wrapper(maybe_cls=None, super_init=None, **kwargs):
    '''
    Decorator that wraps attrs to allow calling a super class initializer. Note
    that super_init must return the args and kwargs that should be passed to the
    underlying __init__ that attrs automatically creates.
    '''
    def wrap(cls):
        ''' The internal wrapper method for the decorator. '''
        new_cls = attrs(**kwargs)(cls)
        if super_init is not None:
            __init__ = getattr(new_cls, '__init__')

            def __super_init__(self, *args, **kwargs):
                '''
                This is a version of __init__ that calls the user defined super
                class initializer, which may consume some args and kwargs as
                needed.
                '''
                args, kwargs = super_init(super(new_cls, self), *args, **kwargs)
                __init__(self, *args, **kwargs)

            setattr(new_cls, '__init__', __super_init__)

        return new_cls

    if maybe_cls is None:
        return wrap

    return wrap(maybe_cls)


# an example when called without arguments
@attrs_wrapper 
class BaseClass(object):
    ''' A base class '''
    foo = attrib(type=int, default 1)


def child_base_init(_super, *args, foo=1, **kwargs):
    ''' A method to call the initializer of child's super class '''
    _super.__init__(foo=foo)

    # return the args and kwargs not consumed by the base class
    return args, kwargs


# an example with arguments
@attrs_wrapper(auto_attribs=True, super_init=child_base_init)
class ChildClass(BaseClass):
    ''' A child class '''
    bar: int = 2


BaseClass()  # BaseClass(foo=1)
BaseClass(2)  # BaseClass(foo=2)
ChildClass()  # ChildClass(foo=1, bar=2)
ChildClass(3)  # ChildClass(foo=3, bar=2)
ChildClass(3, 4)  # ChildClass(foo=3, bar=4)
ChildClass(bar=4)  # ChildClass(foo=1, bar=4)

@lig
Copy link

lig commented Sep 24, 2019

I'm not entirely sure that this makes the case for __attrs_init__ but I've come here looking for a solution to somehow prevent attrs from removing leading _ from an attribute in the auto generated __init__.

For whatever reason, I really need __init__ to accept underscored argument as it is.:(

@wbolster
Copy link
Member

For whatever reason, I really need __init__ to accept underscored argument as it is.:(

perhaps init=... can take either a boolean or a string (which will become the arg name for __init__), similar to what i did to the repr=... arg recently in #568 fo fix #212 / #567.

@hynek
Copy link
Member

hynek commented Oct 19, 2019

I don’t think I’m comfortable for another ad-hoc solution like this. The repr thing was just very obvious and pragmatic.

This one seems more like an candidate for pluggable method makers plus metadata. 🤔

@hynek
Copy link
Member

hynek commented Oct 27, 2019

I have found another instance where the idea of __attrs_init__ would make sense: https://stackoverflow.com/questions/58571329/attrs-library-and-super/58581192#58581192

When you subclass Tensorflow models, it will intercept __setattr__ and raise Runtime errors when attrs tries to set an attribute. It enforces that you run super() first.

I wanted to implement auto-detection of __init__ next anyways so this may actually tie in nicely.

@dojoteef
Copy link

@hynek This is likely a similar issue to the one I reported above using Pytorch. I wonder if my workaround would help for Tensorflow as well.

@hynek
Copy link
Member

hynek commented Oct 27, 2019

Ohhh, maybe? But I suspect it wouldn't be very convincing to someone without any buy in so far. 🤔 But I'll link it, thanks for pointing it out!

@hynek
Copy link
Member

hynek commented Oct 27, 2019

Also thanks for the kind words above, currently there seems to be some law of physics that whenever someone asks an attrs question on SO, someone will suggest to use dataclasses instead 🙄 It is left as an exercise to the reader to figure out how demotivating that is.

@lig
Copy link

lig commented Nov 4, 2019

whenever someone asks an attrs question on SO, someone will suggest to use dataclasses instead

@hynek I'm trying to do the opposite whenever I have an opportunity:)

Lang lebe attrs!

@spalato
Copy link

spalato commented Oct 29, 2020

Hi all,

To add food for thought, I have found the following behavior when using attrs with mixin classes.

The behavior is different when subclassing directly (case Data1 below) vs using a dummy class to preserve calling __init__ (case Data2, below). That's somehow breaking my expectations.

import attr

class Mixin(object):  # just a humble mixin providing orthogonal functionality
    def __init__(self, *args, **kwargs):
        self.param = []
        super().__init__(*args, **kwargs)

# Inheritance case 1
@attr.s
class Data1(Mixin, object):
    value: int = attr.ib(default=1)

# Inheritance case 2
@attr.s
class Base2(object):
    value: int = attr.ib(default=1)

class Data2(Mixin, Base2): pass

# make instances
d1 = Data1()
d2 = Data2()

assert isinstance(d1, Mixin)
assert isinstance(d2, Mixin)

assert not hasattr(d1, "param")
assert     hasattr(d2, "param")   # < - extra whitespace for emphasis

@hynek
Copy link
Member

hynek commented Oct 30, 2020

It does make sense though:

  • Data1 is an attrs class with an attrs-made __init__, so self.param is never set.
  • Data2 is not and attrs class and you mix in your Mixin in a way, that Mixin is before Base2 in the MRO and therefore its __init__ is called (which calls super()).

It is somewhat unintuitive, but that's not really attrs' fault and more a property of multiple inheritance/mixins. :|

@spalato
Copy link

spalato commented Oct 30, 2020

I understand how the MRO differs in both cases. As a new user of attr, I was surprised that defining a class using attr.s implicitly changes the behavior of __init__. I note that when reading the class definition of Mixin, one expects it to provide a param attribute.

I was expecting a call to super given it's the standard procedure in python. Also because I read this bit in the attrs docs: The class belongs to the users. (emphasis not mine). Actually, the whole "Philosophy" section is golden. Sadly, as I understand it the __init__ written by attr.s breaks points 2, 3 and 5.
https://www.attrs.org/en/latest/overview.html#philosophy

Multiple inheritance has the drawback that it is highly sensitive to the MRO, it is easy to get wrong and is often brittle. One of the safer use cases is to mix classes with orthogonal functionality and short disjoint MROs. In the example provided above, the pitfalls of the MRO can be safely avoided in most cases (one exception is attrs!). I note that traitlets preserves the expected behavior, despite it's addiction to heavy stuff.

My use case seems closely related to other use cases in this thread. I am working on a library to notify GUI widgets (Pyside2) whenever an attribute changes. This accomplishes the very important task of keeping multiple independent widgets in sync with the underlying data, cutting on the boilerplate code, and without forcing the user to rewrite their entire codebase when they decide they want a GUI for their task (go read the docs about Qt Model/View, I'll wait...). To minimize potential conflicts, I add a single attribute, a single method, and intercepting calls to __setattr__ to notify widgets (if necessary). So far this seems to work with vanilla classes, when using properties and with traitlets. For attrs one has to be careful with the MRO (and we don't like that, do we?).

Finally, here is one more example to ponder:

import attr

class Mixin(object):  # test mixin please ignore
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.param = []

@attr.s(auto_attribs=True)
class Data1(Mixin):
    value: int = 1

class Data2(Mixin):
    value: int = 1

d1 = Data1()
d2 = Data2()

assert not hasattr(d1, "param")  # I think that qualifies as a surprise!
assert     hasattr(d2, "param")

@hynek
Copy link
Member

hynek commented Jan 26, 2021

This has been fixed by #750 and #731.

@hynek hynek closed this as completed Jan 26, 2021
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

9 participants