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

Extend attrs by adding custom decorators to attributes #340

Closed
benbovy opened this issue Jan 30, 2018 · 16 comments
Closed

Extend attrs by adding custom decorators to attributes #340

benbovy opened this issue Jan 30, 2018 · 16 comments
Labels

Comments

@benbovy
Copy link

benbovy commented Jan 30, 2018

I'm thinking about refactoring a project so that it uses attrs under the hood (for more info, see benbovy/xarray-simlab#19), and I'd like to be able to do something like this:

@process
class Foo(object):
    bar = variable()

    @bar.custom_decorator
    def meth(self):
        pass

In the example above, process wraps attr.s and variable wraps attr.ib. Custom decorators would be a very nice way of implementing operations like validating input value or setting default value, but here more domain-specific.

@wsanchez
Copy link

@benbovy: This request could use some more specifics… can you describe an actual problem that needs solving which isn't addressed by existing mechanisms? Why the wrappers around attr.s and attr.ib?

@hynek
Copy link
Member

hynek commented Jan 30, 2018

Yeah I’m pretty sure that’s already possible. Check out https:/hynek/environ_config/blob/master/src/environ/_environ_config.py for a practical example.

@benbovy
Copy link
Author

benbovy commented Feb 1, 2018

@wsanchez happy to provide more details. Basically, I'm developing a package in which I want to create very succinct classes that will be used as components of numerical models (a model = a collection of such classes that I call "processes"). Each of these classes declare some variables (e.g., physical fields) + some methods that will be called during a simulation to get, set or update values for these variables.

I think attrs is very suited for this kind of project. I could simply reuse features like default values, validators, etc.

I think that the reasons why I add wrappers around attr.s and attr.ib are quite similar to environ_config: I use some reserved keys in attr.ib metadata and there is additional logic in order to create process classes that is based on these reserved keys. Additionally, the attr way of creating classes is something that I'd like to expose to users (e.g., scientists, modellers), and I think that process and variable are more meaningful names than attr.s and attr.ib in this specific case.

@Tinche
Copy link
Member

Tinche commented Feb 1, 2018

Ah, you're basically asking for _CountingAttr to become public, so you could subclass it. It's not an unreasonable request I think.

@Tinche
Copy link
Member

Tinche commented Feb 1, 2018

You can wrap attr.s and attr.ib today, and it's fine (except a little non-composable). I used to do it in cattrs until attrs got its own type metadata.

@benbovy
Copy link
Author

benbovy commented Feb 1, 2018

Unfortunately, I didn't succeed in trying to add a custom decorator to attr.ib.

The closer (though not working) solution I found is to subclass _CountingAttr, which already looks very fragile:

from attr._make import _CountingAttr


class CountinAttrExtended(_CountingAttr):
    def custom_decorator(self, meth):
        self._custom_decorator = meth
import attr


def variable(dims, intent='input', description=''):
    metadata = {'attr_type': AttrType.VARIABLE,
                'intent': intent,
                'description': description}
    
    return CountinAttrExtended(
        default=attr.NOTHING,
        validator=None,
        repr=False,
        cmp=False,
        hash=None,
        init=False,
        converter=None,
        metadata=metadata,
        type=None,
    )

    return a

def process():
   # wrapper around attr.s

The basic example below runs fine but won't fully work because the _CountingAttr instance is later turned into an Attribute instance, which doesn't keep the custom decorator.

@process(time_dependent=False)
class Grid(object):
    length = variable(intent='input', description='grid total length')
    spacing = variable(intent='input')
    x = variable('x', intent='output')
    nnodes = variable((), intent='output')
    
    @nnodes.custom_decorator
    def get_nnodes(self):
        return self.x.size
    
    def initialize(self):
        self.x = np.arange(0, self.length + self.spacing, self.spacing)

Also, I can't add attributes dynamically to neither _CountingAttr or Attribute because these both use __slots__ I guess...

Is there any other way to achieve this?

@benbovy
Copy link
Author

benbovy commented Feb 1, 2018

Ah, you're basically asking for _CountingAttr to become public, so you could subclass it. It's not an unreasonable request I think.

Yeah exactly! That's a much better way to expose the problem :)

@Tinche
Copy link
Member

Tinche commented Feb 1, 2018

Yeah, the problem is _CountingAttr will get replaced, so what you want to do is: in the custom_decorator method, stick something on the function it's decorating. Then wrap attr.s and read the thing you've stuck on and do some extra work. I think that's the best you can do atm.

@benbovy
Copy link
Author

benbovy commented Feb 1, 2018

in the custom_decorator method, stick something on the function it's decorating. Then wrap attr.s and read the thing you've stuck on and do some extra work.

Sounds like a reasonable workaround.

However, in the example above @nnodes.custom_decorator will still give a AttributeError: '_CountingAttr' object has no attribute 'custom_decorator', right?

Although @nnodes.custom_decorator is nice and consistent with built-in decorators (validator and default), I'm fine using something like @custom_decorator('nnodes') atm.

@Tinche
Copy link
Member

Tinche commented Feb 1, 2018

Yeah, you're still going to need to subclass _CountingAttr to make the decorator available in the first place. It's private but the cops won't show up at your door if you do it :)

@benbovy
Copy link
Author

benbovy commented Feb 1, 2018

OK, I see, thanks @Tinche

@benbovy
Copy link
Author

benbovy commented May 14, 2018

So I finally refactored my project so that it now extends attrs and it works great!! The code is here: https:/benbovy/xarray-simlab.

Thanks again for the help here!

[slightly off-topic] The way attrs is used in that project may be a bit unusual? There is a wrapper around attr.s that turns all (wrapped) attr.ib defined in the decorated class into properties of that class (all attr.ib wrappers thus set init=False). I hope that this is not something you consider as a hack and that next versions of attrs will continue to support this.

Back to this issue, I'm still a bit worried about the fact that I had to use attrs' internals (i.e., subclass _CoutingAttr here). Any chance of eventually making it part of the public API?

Feel free to close this issue if this is never going to happen.

@benbovy
Copy link
Author

benbovy commented Aug 29, 2018

I'm still a bit worried about the fact that I had to use attrs' internals (i.e., subclass _CoutingAttr here). Any chance of eventually making it part of the public API?

Any chance of eventually having _CoutingAttr in the public API (or any alternative that allows adding custom decorators)?

The problem with my subclass of _CoutingAttr is that it's now broken with the new kwd_only argument introduced in #411. I guess this is likely to happen again in the future.

I certainly could maintain it, but it would be nice if a more stable solution exists. If you think this request is reasonable, I'd be happy to work on it!

@hynek
Copy link
Member

hynek commented Sep 1, 2018

Hm off top of my head that seems like another huge backward compatibility headache? 🤔

@benbovy
Copy link
Author

benbovy commented Sep 10, 2018

Hm off top of my head that seems like another huge backward compatibility headache?

Yes I agree, it is probably not worth this edge case.

Instead of subclassing _CoutingAttr I ended-up monkey-patching it with an extra compute method. I think it's quite safe (unless you plan to add a method with that name later) and I'm fine with this solution.

@wsanchez
Copy link

@benbovy I'm reading the above as you found a satisfactory solution, so I'm going to close. Holler if you think that was premature.

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

4 participants