Argument collision with @decorator about funcy HOT 19 CLOSED

.__getattr__() is only called in absence of attr, so there is no destructive issue. Yes, you can't access those args by name, but I will ignore that.

from funcy.

I'm talking about the case that you'd "call @decorator's __getattr__(self, ...) manually" (emphasis added).

from funcy.

Python have open objects, which everyone can break, one can just:

call._args = something

No need to call .__getattr__(). One can break any class or overwrite builtins, it's not supposed to be protected.

from funcy.

One of the first likely things a person will try to do if they run into this corner case is call __getattr__ manually.

They'd be wrong for doing so without thinking it through and checking the code for errors, but it's understandably intuitive to try it.

And one they do, this is one of those potentially initially silent hard-to-debug bugs where the actual exception or unexpected behavior might get thrown way later.

Because any decorated function with _func, _args, and _kwargs as argument names is probably using them the same way.

So unless the signature mismatches it could silently go through, only to throw an exception or misbehave way later in the execution, when the source of the wrong arguments having propagated through is not obvious.

Again, I fully acknowledge that this is a small corner-case that funcy probably doesn't need to care about it, but if that's the decision I think it ought to be explicitly documented somehow.

from funcy.

I am not into fixing bugs that never happened and probably never will. And, BTW, this is not intuitive at all)

from funcy.

"Intuitive" is obviously subjective - it was intuitive for me, which is why I'm here.

Following this reasoning:

1. My first reaction was to check if the attributes are statically set on call by doing dir on it.
2. That shows that they are not, which obviously suggests __getattr__ is being used.
3. precisely because __getattr__ doesn't get called for existing attributes,
4. __getattr__ has absolutely no reason to set attributes again that it has already set.
5. Therefore why would I ever expect __getattr__ to re-set already set attributes?

If a user starts thinking "I wonder how I can access the _args argument for my decorated function inside my decorator?", what do you think is more intuitive?

from funcy.

I also really don't understand your reactions in this entire conversation.

What are you against? Adding two sentences of documentation?

Is there any reason for the __getattr__ code to prefer

self.__dict__[name] = arggetter(name, ...)

over

self.__dict__.setdefault(name, arggetter(name, ...))

? Would it break anything?

Did I offend you somehow?

from funcy.

I see your logic, but by using .__getattr__() you broke the abstraction. And while staying within one assigning to dict is clear optimization.

from funcy.

Maybe using .setdefault() is ok if it's as fast as item set. And I see now that this is not theoretical, so you did use those reserved names. Initially I chose to just ignore such case.

from funcy.

Anyway, thank you for hearing me out.

I think funcy is a really great library, and I'm glad and thankful for the work you put into it.

from funcy.

Sorry, I wrote my last message before I saw your last message.

from funcy.

I estimated overhead of this change and it will increase current one by 20% having single arg access in decorator. Won't fix for now. And maybe never.

from funcy.

How did you measure this? 20% difference between what code exactly?

My results show a much smaller and inconsistent slowdown when accessing arguments as attributes on the Call objects.

I also see consistently faster runs when not accessing arguments as attributes with my change.

My Change

In the tests below, funcy.decorator is the current funcy 1.12 code and decorator is exactly the same except class Call has __getattr__ modified like so:

    def __getattr__(self, name):
        try:
-            res = self.__dict__[name] = arggetter(self._func)(name, self._args, self._kwargs)
+            res = arggetter(self._func)(name, self._args, self._kwargs)
+            self.__dict__.setdefault(name, res)
            return res
        except TypeError as e:
            raise AttributeError(*e.args)

When __dict__[key] = value is faster:

Single argument accessed as attribute on call object:

@funcy.decorator
def use_getattr_old(call):
    foo = call.foo
    call()
    return foo

@decorator
def use_getattr_new(call):
    foo = call.foo
    call()
    return foo

@use_getattr_old
def test_use_getattr_old(foo):
    return foo

@use_getattr_new
def test_use_getattr_new(foo):
    return foo

Windows 7, Python 3.7.0:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_use_getattr_old as t' 't(0)'
1000000 loops, best of 5: 2.6 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_use_getattr_new as t' 't(0)'
1000000 loops, best of 5: 2.52 usec per loop

Linux (Ubuntu 16.04) Python 3.5.2:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_use_getattr_old as t ' 't(0)'
1000000 loops, best of 5: 3.16 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_use_getattr_new as t ' 't(0)'
1000000 loops, best of 5: 3.19 usec per loop

Note: Although the trend is towards setdefault being slower, sometimes it actually runs faster. For example:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_use_getattr_old as t ' 't(0)'
1000000 loops, best of 5: 3.14 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_use_getattr_new as t ' 't(0)'
1000000 loops, best of 5: 3.09 usec per loop

Eight arguments accessed as attributes on call object:

@funcy.decorator
def use_getattr_8x_old(call):
    return (call.a, call.b, call.c, call.d, call.e, call.f, call.g, call.h)

@decorator
def use_getattr_8x_new(call):
    return (call.a, call.b, call.c, call.d, call.e, call.f, call.g, call.h)

@use_getattr_8x_old
def test_use_getattr_8x_old(a, b, c, d, e=5, f=6, g=7, h=8):
    return None

@use_getattr_8x_new
def test_use_getattr_8x_new(a, b, c, d, e=5, f=6, g=7, h=8):
    return None

Windows 7, Python 3.7.0:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_use_getattr_8x_old as t' 't(1,2,3,4)'
1000000 loops, best of 5: 12.1 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_use_getattr_8x_new as t' 't(1,2,3,4)'
1000000 loops, best of 5: 12.6 usec per loop

Linux (Ubuntu 16.04) Python 3.5.2:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_use_getattr_8x_old a s t' 't(1,2,3,4)'
1000000 loops, best of 5: 12.4 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_use_getattr_8x_new a s t' 't(1,2,3,4)'
1000000 loops, best of 5: 13 usec per loop

When __dict__.setdefault(key, value) is faster:

Basic passthrough

(Presumably this is representative of doing unrelated work in the decorator like logging)

@funcy.decorator
def passthrough_old(call):
    return call()

@decorator
def passthrough_new(call):
    return call()

@passthrough_old
def test_passthrough_old(foo):
    return foo

@passthrough_new
def test_passthrough_new(foo):
    return foo

Windows 7, Python 3.7.0:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_passthrough_old as t' 't(0)'
1000000 loops, best of 5: 1.22 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_passthrough_new as t' 't(0)'
1000000 loops, best of 5: 1.18 usec per loop

Linux (Ubuntu 16.04) Python 3.5.2:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_passthrough_old as t ' 't(0)'
1000000 loops, best of 5: 1.56 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_passthrough_new as t ' 't(0)'
1000000 loops, best of 5: 1.47 usec per loop

Appending argument to call:

@funcy.decorator
def add_arg_old(call):
    return call(0)

@decorator
def add_arg_new(call):
    return call(0)

@add_arg_old
def test_add_arg_old(foo):
    return foo

@add_arg_new
def test_add_arg_new(foo):
    return foo

Windows 7, Python 3.7.0:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_add_arg_old as t' 't()'
1000000 loops, best of 5: 1.52 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_add_arg_new as t' 't()'
1000000 loops, best of 5: 1.42 usec per loop

Linux (Ubuntu 16.04) Python 3.5.2:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_add_arg_old as t' 't ()'
1000000 loops, best of 5: 1.9 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_add_arg_new as t' 't ()'
1000000 loops, best of 5: 1.8 usec per loop

Adding keyword argument to call:

@funcy.decorator
def add_kwarg_old(call):
    return call(foo=0)

@decorator
def add_kwarg_new(call):
    return call(foo=0)

@add_kwarg_old
def test_add_kwarg_old(foo):
    return foo

@add_kwarg_new
def test_add_kwarg_new(foo):
    return foo

Windows 7, Python 3.7.0:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_add_kwarg_old as t' 't()'
1000000 loops, best of 5: 1.74 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_add_kwarg_new as t' 't()'
1000000 loops, best of 5: 1.58 usec per loop

Linux (Ubuntu 16.04) Python 3.5.2:

$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_add_kwarg_old as t'
't()'
1000000 loops, best of 5: 2.05 usec per loop
$ python -m timeit -n 1000000 -r 5 -s 'from t1 import test_add_kwarg_new as t'
't()'
1000000 loops, best of 5: 2.02 usec per loop

Test Details

The contents of t1.py are basically just a copy-paste of funcy/decorators.py, with the following four differences:

1. Normal assignment changed per the above diff code block.

2. Python 2 support deleted

3. import funcy added to get access to the original

4. All of the test functions shown above added to the bottom.

Variability

I ran timeit many, many times with the above arguments for each test. I just copied the last runs of each for the above examples.

During the runs I've seen a lot of variability, but notably more variability in the cases where the new version was slower, and a lot more consistency in the cases where the new version was faster.

Final thought

I think the performance difference here is within the territory of "performance fuzz", where tiny changes in usecase and implementation of surrounding code easily "wash out" this change's effects. Because:

1. The change would improve performance for some people, and would harm performance for others.

2. This is Python, and we're looking at 10 to 100 nanosecond differences: just calling a function costs about as much:

   Compare python -m timeit -n 1000000 -r 5 -s 'def t(x):' -s ' return x' 't(0)', which shows 77.4 nsec (== 0.0774 usec) on my Windows test box and 0.0792 usec (== 79.2 nsec) on my Linux test box.

from funcy.

When you don't have argument access you will run exactly the same code and get same speed. Your "being faster" could be because of changing code layout or removing Python 2 support or just random fluctuation.

I compared my decorator with handcrafted one with single attribute access. In this scenario now I have 2.5 mks overhead and 3 mks with new code. This is where 20% came from.

from funcy.

The bigger question is what we get for this half of a microsecond? For me it doesn't look like much.

from funcy.

Well, I suspect we aren't really going to come to an agreement on this.

I have an overriding heuristic against edge-cases with special behavior motivating my preference, and also for helping people have maximal flexibility on top of code without having to touch implementation internals.

I presume you have something similarly compelling in your mind for not making this change, though I must admit I don't fully see or understand what it is.

About the tests

I can't comment further on the relative applicability of our tests, because you haven't posted yours to compare. I can only conclude that there is something different about your tests vs. mine, because your numbers show a difference that my numbers don't.

I can say that your suggested explanations for my "being faster" don't seem to fit what I observed to my mind:

1. I always run tests many times to rule out random system activity effecting things, and the above examples were representative of many runs.

2. The relevant Python 2 support code only executes at import time, which is done outside of timeit's timings (-s flag) and is hidden behind if-else statements that don't execute on Python 3, so the only overhead should be a small amount of Python bytecode sitting unused in memory, likely flushed out of cache and never touched. I've dealt with tiny memory-constrained cache-less systems where I could see that making a difference, but it should be negligible on typical modern hardware, and at that point you're looking at whole-system differences like memory load, bus speeds between CPU, memory, and disk, and even ephemeral incendentals that could change with every code change and Python version anyway, like which Python bytecode lands on which cache lines.

My tests were convincing enough to me, which was why I ran them - my intuition is that convincing you would have to happen on a deeper human level anyway, and that's not enough of a priority for me right now.

Concluding Thoughts

You're probably right that in the grand scheme of things this doesn't really matter.

I think it will effect someone someday, but probably in a minor, inconsequential way, and it probably won't effect me, but even if it does, I'll probably be able to deal with it more efficiently at that time, than continuing this here.

Thanks for considering it, and thanks again for all the work you've put into making funcy. You make good stuff. In fact I currently use your "Why Every Programming Language Needs Its Underscore" which I found through funcy's readthedocs as my go-to link for showing people why and how functional programming can make code better.

Feel free to close this if you'd like.

from funcy.

Thank you for your time. We both spent probably more time than it is worth here)

from funcy.

For anyone who cares, nowadays I would probably suggest this design adjustment:

• call.args instead of call._args
• call.kwargs instead of call._kwargs,
• call.func instead of call._func, and
• call.arg.{{ name }} instead of call.{{ name }}.

Besides resolving the collision and ambiguity potential, call.arg.foo is more intuitive and self-explanatory than call.foo when reading unfamiliar code.

from funcy.

Maybe. This is backwards incompatible unfortunately.

from funcy.