pytoolz/toolz

curry captures and silences TypeError incorrectly

jluttine opened this issue · 6 comments

curry incorrectly captures and silences TypeError that was raised nested inside the curried function:

>>> import toolz
>>> f = lambda x, y: x + y
>>> g = toolz.curry(lambda *args: f(*args))
>>> g(1,2,3)
<function <lambda> at 0x7f80ac7b2ae8>

There's nothing currying can do to fix this, because the problem isn't related to the signature of g. Thus the type error should have been raised. Instead, the result is some weird function.

I would have expected an error like this:

TypeError: <lambda>() takes 2 positional arguments but 3 were given

Here's an even simpler case:

>>> f = toolz.curry(lambda x, y, *args: x + y)

This works as expected:

>>> f(1, 2)
3

But this fails to raise an error but instead returns a function:

>>> f("foo", 42)
<function <lambda> at 0x7f561ad6dc20>

I would have expected:

>>> f("foo", 42)
TypeError: can only concatenate str (not "int") to str

To me, the implementation of toolz.curry looks overly complex. Why is it so complex? I made a simple curry function for my own purposes in HaskPy package: https://github.com/jluttine/HaskPy/blob/720ebb3c028e017449467a92d736cc5be48f8223/haskpy/utils.py#L23 It's relatively simple function (just a lot of comments), but it handles all optional arguments similarly as toolz.curry, as far as I can tell.

And here is the comprehensive test suite it passes: https://github.com/jluttine/HaskPy/blob/720ebb3c028e017449467a92d736cc5be48f8223/haskpy/tests/test_utils.py#L6 I tried to test almost all possible ways of using a curried function.

So I don't see what extra features toolz.curry brings with its added complexity. Do you have any insight? Why toolz.curry isn't similarly simple as that function in HaskPy? (Perhaps it's a Python 2 thing?)

Hi @jluttine! I'm sorry to hear toolz.curry can't work for you. Thanks for starting this discussion.

This behavior is intentional. Specifically, when a function that has *args raises a TypeError, we give the user the benefit of the doubt such that binding more arguments may succeed (see the comment here: https://github.com/pytoolz/toolz/blob/master/toolz/functoolz.py#L324). For example, this is useful when currying functions that have been decorated and now have the call signature of f(*args, **kwargs). Perhaps if a TypeError is raised and we encounter this condition, the returned object could save the exception as an attribute that the user may then identify and take appropriate action. Alternatively, perhaps there could be a nice way to control this behavior such as by setting an attribute on a curried object. One suitable solution may be for the user to simply use functools.partial instead.

curry is one of the most loved and developed functions in toolz. It has seen a steady evolution over many years and from many people. I'm sure some of its complexity is due to supporting Python 2.6, 2.7, 3.3-3.7 over the years. Over these versions, the inspection capabilities given to us by inspect have been moving targets and sometimes outright broken. Regardless, toolz.curry tries really, really hard to do the right thing (which is apparently different behavior from what you want). Beyond this, I'm afraid I can't speak much to why it looks complex to you. Would you care to provide specifics? Have you looked at the tests for curry?

@eriknw Thanks for the response! Yes, toolz.curry is definitely one of the most beloved functions in toolz and I think toolz is a great package! If I understood correctly, the usecase you want to support is:

>>> f = lambda *args: (lambda x, y, z: x + y + z)(*args)
>>> curry(f)("a")("b")("c")
"abc"

That is, although the user curries the outer lambda with *args, also the inner lambda wth arguments x, y and z is curried. Indeed, my own implementation doesn't support this, because the inner function isn't curried. Note, though, that in my implementation decorating a curried function works just fine:

f = lambda *args: curry(lambda x, y, z: x + y + z)(*args)
>>> f("a")("b")("c")
'abc'

Or even currying also the decorated function:

>>> f = curry(lambda *args: curry(lambda x, y, z: x + y + z)(*args))
>>> f("a")("b")("c")
'abc'

Personally, I prefer having it explicit which exact function is curried and not trying to guess that perhaps the function which should have been curried is somewhere nested inside the function which the user curried. Perhaps this is a matter of taste, but I would be very much worried about suppressing TypeError and returning a non-sense result in that case (please try my examples and play with the non-sense result). At least for me, it's a definite no-go for toolz.curry and I see toolz.curry broken. It hit me quite hard. But as it's an intentional design decision in toolz.curry, I close this issue (feel free to re-open if you want).

About the complexity, my own implementation is about 30 lines of code (ignoring comments). It's just a simple function decorator. In comparison, toolz.curry is 180 line class with lots of methods that seem to do a lot of things I can't easily understand. I'm just thinking if there are some features beside "keep the function partially evaluated until all required arguments have been provided" that I'm neglecting, otherwise I don't understand why it needs to be so complex. So I was just curious to understand what I might be missing. It could be related to supporting older Python versions as you said. I can imagine the difficulty of supporting all those Python versions..

Anyway, thanks a lot for your response!

No worries, and thanks for the thoughtful and constructive discussion! I appreciate that you opened this issue. I don't recall this ever being discussed in detail, and this is the first that I recall this behavior being an issue for somebody. In truth, toolz.curry could do better here, so the issue is valid. The other valid issue is having clearer code comments so that a user shouldn't be overwhelmed when peaking at the implementation.

We could be smarter about distinguishing problematic TypeError exceptions and ones for which we should curry. For example, in Python 3, we could look for the messages "missing \d+ required positional argument" and "missing \d+ required keyword-only argument" (Python 2, which is being dropped shortly, would require different handling) and react accordingly. This could potentially miss some TypeErrors raised by builtins (for example, sorted() has different messages based on Python version), but maybe that's okay.

Here are a few things that add to the complexity of toolz.curry:

  1. ability to pickle
  2. ability to compare and hash curried objects
  3. handle builtins or c extension functions that aren't inspectable
  4. define it's signature so it's inspectable
  5. fast execution

We do care about speed in toolz, and toolz.curry has undergone iterations of optimization. Let's compare to the implementation of curry that you shared:

In [43]: def f(x, y):
    ...:     pass
    ...:
    ...: partial_f = functools.partial(f, 1)
    ...: curried_f = curry(f)(1)  # yours
    ...: toolz_f = toolz.curry(f)(1)
    ...: cytoolz_f = cytoolz.curry(f)(1)

In [44]: %timeit f(1, 2)
57.1 ns ± 2.15 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [45]: %timeit partial_f(2)
87.7 ns ± 1.58 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [46]: %timeit curried_f(2)
35.3 µs ± 948 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

In [47]: %timeit toolz_f(2)
229 ns ± 14 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [48]: %timeit cytoolz_f(2)
137 ns ± 2.53 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In this example, toolz.curry is about 150 times faster, and cytoolz.curry is about 250 times faster.

@eriknw Thanks a lot for the insight and the analysis! I hadn't thought about point (3), so that's probably something I'm missing. Also, I haven't cared about speed at all since I'm thinking it's going to be slow anyway and should be used on high-level code only. But it seems you've managed to get really fast speed! 👍

By the way, thanks for pointing out the speed difference! I made a trivial speed improvement to my implementation. Now it's a bit faster than toolz but slightly slower than cytoolz for the benchmarking you provided:

In [9]: %timeit f(1, 2)
81.5 ns ± 1.31 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [10]: %timeit partial_f(2)
134 ns ± 2.22 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [11]: %timeit curried_f(2)
285 ns ± 1.35 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [12]: %timeit toolz_f(2)
373 ns ± 1.69 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [13]: %timeit cytoolz_f(2)
202 ns ± 0.95 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

I have one more idea how to possibly make it slightly faster when partially evaluating (not fully evaluating) an already partially evaluated function. But as the speed-up would probably be quite small, the usecase quite rare and the implementation a bit more complex and error-prone, I think I'll leave that for now.