-
Notifications
You must be signed in to change notification settings - Fork 3
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
Allow composing using '+' instead of explicit compose() calls. #1
Conversation
[Edit 2023-10-07: Thank you for the contribution, I appreciate it! I need to think about this some more, but I'll try to reply as soon as I can with a code review and either a verdict or thoughts for discussion on the PR idea as a whole. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so, first I'll describe my overall current thoughts, and then I'll also provide a review for the code you've submitted.
Big Picture
-
I think this functionality should be provided separately from the
compose
class itself. -
I relate to the desire to have an operator for function composition, and I think that it is probably good for the world to have the option to do that in Python.
-
I am undecided if I want to take on supporting any implementation of operator overloading for function composition in Python.
-
The biggest motive I feel in favor of me adopting this functionality is that I can already see several pitfalls and edge cases that as a user I'd want to be sure were gotten right.
-
I am undecided if this functionality should be part of the base
compose
package, a separately installable "extra", or a separate project (which can import this one). -
The biggest reason for making it into a separate package is if it makes more sense to have a generalized package providing operator overloads for more than just composition.
-
I am undecided if
+
,*
, or,@
, or maybe even some other overloadable binary operator, is more appropriate for composition.
I've tried to keep all these points very concise - I'll try to elaborate as conversation reveals a need, but also feel free to ask for elaboration on any of them.
Code Review
Per the above, I might not end up merging this, but I wanted to go ahead and provide feedback on the code anyway, since I appreciate anyone who contributes to my software, I can see a lot of places where I can suggest improvements, and lots of people tell me they benefit from my code reviews. Of course you don't have to make any code changes on any of this until/unless I decide that I do want to take this in - I don't want you doing any work that you would feel wasn't worth it if it ends up not getting merged/used.
@@ -59,6 +59,15 @@ def __call__(self, /, *args, **kwargs): | |||
result = function(result) | |||
return result | |||
|
|||
def __add__(self, other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When overriding operators, make sure to also override the reverse function (in this case __radd__
).
elif callable(other): | ||
return compose(*self.functions, other) | ||
else: | ||
raise ValueError("Can only compose callables and 'compose' instances.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python operator overloads are special, so instead of raising an exception in such cases, they should return NotImplemented
, because that's what tells Python to give the other operand a chance to implement the operator, if it hasn't already.
Also, if we were returning an exception here, the appropriate exception would be TypeError
, not ValueError
, because the distinction between callable things and not-callable things is more of a type-of-thing mismatch than a right-type-of-thing-but-wrong-in-a-more-specific-way mismatch.
@@ -59,6 +59,15 @@ def __call__(self, /, *args, **kwargs): | |||
result = function(result) | |||
return result | |||
|
|||
def __add__(self, other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we did implement this as an operator overload on the compose
class, we would need to make sure that acompose
has correctly mirrored behavior which returned acompose
instances instead of compose
instances.
Mainly, because if compose
has a feature, acompose
should match it.
Secondarily, because acompose
is a subclass of compose
, so if we aren't careful, acompose
will inherit this implementation from compose
, and its behavior is wrong for acompose
.
(Incidentally, this is a strong reminder that acompose
should maybe not have been a subclass of compose
. I committed the sins of using inheritance to share behavior, violating the substitutability principle, and conflating a class representing composed callables with a function which does the composition, and this is a good reminder of all of that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to the thoughts triggered by this, I went ahead and fixed acompose
to not be subclass of compose
.
return compose(*self.functions, *other.functions) | ||
elif callable(other): | ||
return compose(*self.functions, other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a class instance constructs another instance of itself, it should use whatever class it actually is, not hardcode the class: type(self)(...)
instead of compose(...)
.
Notice how this makes . (acompose
work correctly tooDebatable if someone could maybe even argue that we should never be subclassing acompose
should be subclass of compose
, andcompose
, but that's a separate issue - the point is that when writing instance methods, doing this usually helps subclasses inherit more correct behavior.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that acompose
is fixed to not be a subclass of compose
, everything in this comment still applies, it's just that now acompose
would have to explicitly reuse the same implementation instead of implicitly inheriting it, like it now does with __init__
, __repr__
, and functions
.
if isinstance(other, compose): | ||
return compose(*self.functions, *other.functions) | ||
elif callable(other): | ||
return compose(*self.functions, other) | ||
else: | ||
raise ValueError("Can only compose callables and 'compose' instances.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to detect compose
instances or *
-splat .functions
, since compose
already does all of that. Just pass in self, other
as the two argumens.
def test_compose_add_compose(): | ||
def f(s): | ||
return s + 'f' | ||
def g(s): | ||
return s + 'g' | ||
f_wrapped = compose(f) | ||
g_wrapped = compose(g) | ||
f_g_wrapped = f_wrapped + g_wrapped | ||
|
||
assert f_g_wrapped('') == 'gf' # Remember that it's f(g(...)) so g runs first | ||
assert f_g_wrapped.functions == (g, f) # functions exposed in execution order | ||
|
||
|
||
def test_compose_add_function(): | ||
def f(s): | ||
return s + 'f' | ||
def g(s): | ||
return s + 'g' | ||
f_wrapped = compose(f) | ||
f_g_composed = f_wrapped + g | ||
assert f_g_composed('') == 'gf' # Remember that it's f(g(...)) so g runs first | ||
assert f_g_composed.functions == (g, f) # functions exposed in execution order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we find ourselves having this much of this kind of repetition in our tests, that suggests we're testing the wrong thing, and should think more precisely about how to test just what we're adding on top of the already-tested behavior.
In this case, the +
in this PR does same thing as compose
itself, and we already have the prior test verifying that compose(f, g)
actually composes f
and g
. So personally, my first reflex was that the only assertions for these two +
tests should look more like
assert (compose(f) + f) == compose(f, f)
but then I remembered that we don't have ==
implemented for compose
, and I don't want to get into designing and implementing equality check overloads for compose
instances, so I guess the next best thing for the purposes of these tests is
assert (compose(f) + f).functions == compose(f, f).functions
Key point is that we want to eliminate all the irrelevant stuff. Why have both f
and g
? Having more than one function is primarily useful for verifying that they are ordered correctly when composed. Why execute the composed callable? Calling it is primarily useful for verifying that calling compose
instances actually works. Since a + b
is supposed to be equivalent to compose(a, b)
, and all that is already part of compose
functionality, then it should be getting tested by another test (and it is) - all we need to test here is whether a + b
actually produces something equivalent to compose(a, b)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I think if I thought about this more, I would probably have other refactors to suggest for these tests, but I don't want to spend more time on thinking and describing how I would further refine the tests for how it is now, given my "big picture" leanings.
def f(s): | ||
return s + 'f' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the actual function is never used, I would probably use the closest thing to an appropriately "nil" function here:
def f(s): | |
return s + 'f' | |
def f(_): | |
return None |
(Of course, a completely nil function has zero arguments and no explicit return value, but function composition logically requires every function except the last one returns something, and every function except the first one takes exactly one argument, so the closest to a nil function which still fits every possible position in a function composition takes one argument and returns something.)
def f(s): | ||
return s + 'f' | ||
f_wrapped = compose(f) | ||
g = 'string' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is just a very minor matter, almost just personal taste, but I would've used None
instead of a string here, because if I'm only using one example, I like to use the most conceptually simple/basic example, and None
is more simple/basic than a string (at least once you've reified nothingness into a placeholder nil value which represents the absence of a value).
raised = False | ||
|
||
try: | ||
f_g_wrapped = f_wrapped + g | ||
except ValueError as err: | ||
raised = True | ||
finally: | ||
assert raised |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raised = False | |
try: | |
f_g_wrapped = f_wrapped + g | |
except ValueError as err: | |
raised = True | |
finally: | |
assert raised | |
try: | |
f_wrapped + g | |
except TypeError: | |
return | |
assert False, '+ should have raised' |
Because of a bunch of tiny optimizations of various factors:
Simpler (less logical operations, less indirection, less things, less complecting of things together), shorter (so more relevant information fits within the detailed central spot of human vision, so you pay less saccades and human working memory for reading the same code behavior), produces a relevant self-explanatory failure message, not naming/storing things which we're not using as if we're going to use them (so the reader has more immediate local cues that they eliminate or discount the possibility that they are used later), the error type matches what Python normally raises in these cases.
Actually, testing if an error is raised is not the right test - Python is responsible for raising that TypeError
if both operands return NotImplemented
. We as operator overloaders are responsible for returning NotImplemented
. So on further thought I'd go with:
raised = False | |
try: | |
f_g_wrapped = f_wrapped + g | |
except ValueError as err: | |
raised = True | |
finally: | |
assert raised | |
result = f_wrapped.__add__(g) | |
assert result is NotImplemented |
finally: | ||
assert raised | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think these tests only felt like they belonged in this spot of the file because the first two tests start out by re-testing everything that the first test tests. But once we clear that up, it becomes more obvious that these tests are not particularly related to the first test at all, and thus are just testing a distinct feature.
And in fact they're breaking up the flow of the tests, because the tests that immediately follow are still testing compose
functionality, while these new tests are testing the layered-on addition of +
behaving as an alias/shorthand for compose
.
So obviously in an ideal world/language, if we want an operator overload for function composition, we would make it happen for all callables, but ideally at least by default only within the scope of the code that we wanted this behavior in, so that we couldn't subtly change the behavior of existing code. (I believe Racket is probably the closest to being able to do this, because in Racket the syntax itself is programmable and scoped as part of the language. Ruby could probably do it too, but without the scoping, because I think in Ruby you can inject new methods onto the builtin base classes.) In Python we mostly can't do this. For starters, as far as I know, we basically can't monkeypatch all callable things to have an operator overload in a scoped way. Perhaps someone really clever could figure out how to do it, but I so far can't. Even if we accept global monkey-patching (I wouldn't in most situations, but it's probably acceptable for certain situations, and I could see it being really nice in interactive REPL use), we can't do it. In CPython, the closest we could get is using So since scoped monkey-patching is impossible and global monkey-patching is not generally desirable, the realistic ideal solution would have to include something shaped like what this PR implements: a decorator which adds an operator overload which does function composition. (This PR basically makes But also since monkey-patching could be desirable in some edge cases, and because it can help us get a more complete feel for the design space, I encourage considering what an ideal monkey-patching implementation looks like: just the relevant method implemented as a standalone function, and some way to add that method onto things. For example, starting with a simple example where we ignore def composing_operator(self, other):
if callable(self) and callable(other):
return compose(self, other)
return NotImplemented ..and then I tried to find a way to set this as the |
So then the next thing I started thinking about, was how do we implement this as a general decorator. Coming at it from an add-methods-to-classes perspective, I initially think about a decorator which takes a class as an argument, and assigns the above function onto Coming at it from a there-are-multiple-good-choices-for-what-operator-to-overload perspective, I want that decorator to also take an argument telling it what operator to overload. So I'm imagining something like |
The last part of each of the last two comments starts showing why I quickly started thinking that it was better design to have this as a separate decorator, which either adds the relevant two methods or wraps the given object in a wrapper class. |
One thing we quickly notice though, is that if So this suggests either
|
Also, once we start thinking about a generic decorator, we probably really want an intelligent, general-purpose proxy. See, This is something that simple So then I start thinking about maybe using something like |
To some extent I'm thinking up some unlikely possible uses here, but affordances breed usage - we might add When I saw this PR and started mentally exploring the idea, I very quickly started thinking about doing something like |
For completeness, here's one possibility I picture doing for def operator_overload(self, other):
if callable(self) and callable(other):
if _isasync(self) or if _isasync(other):
return acompose(self, other)
return compose(self, other)
return NotImplemented
def _isasync(thing):
return inspect.iscoroutinefunction(thing) or inspect.isasyncgenfunction(thing) Explicit Basically, when you have |
The other thing that's been on my mind a lot is that I keep having a hard time seeing a sufficiently compelling benefit to providing this. For example, if all you care about is the shorthand, and you don't care about the And that's perfectly reasonable to do in some situations, it's just too specific and write-convenience focused for me to want in the I'd be very curious to see some representiative code where you're using |
I think it's worth noting that a lot of the time in Python it makes more sense to just write def foo(...):
return bar(qux(...)) instead of foo = compose(foo, bar) The biggest use case I see for This has no natural/native Pythonic language: compose(*functions) Notably, operator overload doesn't do anything for us there.
So that leaves looping situations. Something like this: action = f
for ... in ...:
...
action = compose(action, ...)
... and that's a place where we could reach for an operator overload, like this: action = compose(f)
for ... in ...:
...
action += ...
... but personally I think that arguably obfuscates more than it clarifies - it feels nice and elegant to write, but it forces a reader to remember that there's a actions = [f]
for ... in ...:
...
actions.append(...)
...
action = compose(*actions) That leaves the decorator use-cases, monkey-patching use-cases, and interactive REPL use-cases, I think. And that's why I'm still having trouble seeing a great use-case or justification for this to be included as a built-in feature within the |
Ok, I've decided I don't want to implement this inside the Of course, if anyone wants to argue for it, you are still welcome to comment! I'm particularly open to seeing any use-cases in real code or interactive sessions where you found yourself wishing this feature was in I fully endorse subclassing I am still open to generalizing this PR's idea into its own package/library, but so far I don't yet have enough vision on what that looks like, nor the time to finish developing that vision. |
Considering this again. I saw a few users of the So I was thinking about it more, and thought:
|
One interesting point raised in the toolz and mypy issues is of type-hinting. I always got the impression Python's type-hinting stuff just wasn't powerful enough to express function composition, but I guess if you break up function composition into two-callables-at-a-time chunks, static type analysis becomes much more doable. |
Here's a sketch of my current idea for providing this: from wrapt import ObjectProxy as _ObjectProxy
def compose_operator(self, other):
if not callable(self) or not callable(other):
return NotImplemented
if _isasync(self) or _isasync(other):
return Composable(acompose(self, other))
return Composable(compose(self, other))
class Composable(_ObjectProxy):
__add__ = compose_operator
__radd__ = compose_operator_reversed Untested, might need fixing/optimization and so on. Also, I want to carefully review I also don't want to add a |
Latest design ideas snapshot: First,don't try to detect "is this async" ahead-of-time, that's less universal and more fragile/unreliable in the edge cases. Instead, implement another variant of
That was way too complicated to say in English, here's a simple sketch of the two relevant functions: def __call__(self, /, *args, **kwargs):
"""Call the composed function."""
result = self.__wrapped__(*args, **kwargs)
for index, function in enumerate(self._wrappers):
if _isawaitable(result):
return _finish(acompose(self._wrappers[index:]), result)
result = function(result)
return result
async def _finish(function, awaitable):
return await function(await awaitable) Notice how This is great because it gets us all three of dynamically becoming async as needed, [I forgot what I was going to write here]. As this very function shows, it's possible to have a function that is impossible to detect as asynchronous until after it returns an awaitable object (and sometimes it's that way for good reason). So the "true" and "best" way to handle asynchronicity is by detecting if something returned an awaitable object after you've called it. Second,since we no longer have to worry about selecting between sync and async classes in the operator overload, we can actually provide a really nice compose operator function: def compose_operator(self, other):
if not callable(self) or not callable(other):
return NotImplemented
return type(self)(self, other) Then that operator overload gets added on a class which is already I can't tell if this makes type-hinting better or worse. At first glance better (the actual Python type of the return is the same type as |
Third,I think I'm pretty close to concluding that a good operator for I had previously held off on
But since then I've realized that more abstractly, shell pipes are conceptually closer to composition of functions than monads: commands are like functions, passing arguments to commands is like partially applying arguments to the function, standard input is the final argument, and standard output is like a return value. And in the previously-linked |
So in case anyone missed it, I added the (Implementing |
So if I was going to make the best possible implementation of operator overloads for function composition I could build, right now it would look kinda like this: from compose import sacompose
class composable:
def __init__(self, function):
if not callable(function):
raise TypeError('composable argument must be callable')
self.__wrapped__ = sacompose(function)
def __or__(self, other):
if not callable(other):
return NotImplemented
return type(self)(sacompose(other, self.__wrapped__))
def __ror__(self, other):
if not callable(other):
return NotImplemented
return type(self)(sacompose(self.__wrapped__, other))
def __call__(self, /, *args, **kwargs):
return self.__wrapped__(*args, **kwargs)
def __get__(self, obj, objtype=None):
return type(self)(self.__wrapped__.__get__(obj, objtype))
def __repr__(self):
return _name(self) + '(' + repr(self.__wrapped__) + ')' |
However! As a decorator the above version is still not great, because it's not really transparent to a bunch of operations. To achieve that, I would bring in class composable(wrapt.ObjectProxy):
def __init__(self, function):
if not callable(function):
raise TypeError('composable argument must be callable')
super().__init__(function)
def __or__(self, other):
if not callable(other):
return NotImplemented
return type(self)(sacompose(other, self.__wrapped__))
def __ror__(self, other):
if not callable(other):
return NotImplemented
return type(self)(sacompose(self.__wrapped__, other))
def __call__(self, /, *args, **kwargs):
return self.__wrapped__(*args, **kwargs)
def __get__(self, obj, objtype=None):
wrapped = self.__wrapped__
try:
bind = type(wrapped).__get__
except AttributeError:
return self
bound_wrapped = bind(wrapped, obj, objtype)
if bound_wrapped is wrapped:
return self
return type(self)(bound_wrapped)
def __repr__(self):
return _name(self) + '(' + repr(self.__wrapped__) + ')' |
Couple things I'm noticing:
The only reason I've held back from a subclassing-based solution is that it is more opaque than it needs to be in the decorator case, and that bothers me. But if someone doesn't mind that, then they can do this much simpler version: class composable(sacompose):
def __or__(self, other):
if not callable(other):
return NotImplemented
return type(self)(other, self)
def __ror__(self, other):
if not callable(other):
return NotImplemented
return type(self)(self, other) |
Vaguely relevant: classes are callable and it can be pretty useful to compose classes just like functions, but as of Python 3.10, classes have a meaning for the So
Could be fixed by introspecting type union operands in the composition operator as a special case. But type unions also deduplicate (and might more generally not be guaranteed to preserve order?), so a construct like This is probably fine. But I could see one module slapping a (This also means that any ideas of aggressivelly monkey-patching builtins, like the interactive convenience in a REPL use-case, couldn't monkey patch So maybe this is a good argument for some other operator, maybe This also leads to an interesting design think direction:
|
Despite repeatedly bringing up the monkey patching use case, I think that for the most part monkey patching is bad and shouldn't be done except for very exceptional situations. And more generally, I want to be clear that "oh this works well for everything except one corner case when monkey patching the whole language" is not a blocker for me if there is a great design for other use-cases, it's just something I want to keep holding in mind... I find that the best design is more possible to recognize when keeping as many use-cases as possible in mind. |
Couple other ways to approach this:
|
I think I've basically convinced myself that the monkey patching for interactive usage idea is approximately never better than writing a small wrapper layer around a REPL/interpreter which converts a composition operator of your choice into the right compose calls. At the same time, I think I see the monkey-patching thing as reasonably "solved" (or at least solvable) on the Pythons where such deep hacks are possible (CPython yes, PyPy no, ...) but not worth chasing further. The biggest thing that stands out to me about deep monkey patching is that the |
Basically if someone is just using this higher-level composable module I'm building up, then they're probably not using So the only time it matters is if someone is trying to debug code where composes functions made with Then and only then is |
Yeah on further thought it's actually good to have the repr raise awareness of Also, the more I think about this, the more I wonder if in a language with asynchronous functions, the natural / default function composition should perhaps be the one that gracefully handles becoming async if any of the functions it is composing is async. Basically maybe a relevant change here is for the
|
Or even to just drop (Of course variants of the code for no- |
Basically, whatever has the name On the one hand, I still think that:
|
(So the last comment rules out just totally deleting |
Okay so the "on the other hand" case:
Honestly I'm having trouble coming up with great arguments in favor of changing it, now that I've re-traced all my reasoning (and realized some new reasoning along the way) in favor of keeping it as-is. So I'm just going to keep them named as they were in |
Ahhh, problem on the Hmm... |
I'd almost say that's not an inherently wrong thing... after all there are mutating decorators out there, and in principle someone could import your class and use such a decorator on it. In fact it's almost a natural result of the original intent for It's a problem for the What would really be nice is some sort of generic combinator which could combine object proxy behavior with other behavior... Basically this combinator would just need two arguments: a thing that it is wrapping, and a "piercer" function that given that thing, finds the thing it should really be proxying all the not-overloaded operations to. It's possible that a But that's more flexibility than we strictly need here. Anyway, the easiest solution is to just make I was initially extremely resistant to this because I was thinking of So okay, I feel fine with that. As the author of Anyway, so I think this is an easy fix: class ComposedProxy(_compose.sacompose, _wrapt.ObjectProxy):
+ __slots__ = '_wrappers'
def __init__(self, *functions):
_compose.sacompose.__init__(self, *functions)
_wrapt.ObjectProxy.__init__(self, functions[-1])
def __reduce_ex__(self, protocol):
return (type(self), tuple(reversed(self.functions))) |
import functools as _functools
import compose as _compose
import reprshed as _reprshed
import wrapt as _wrapt
class composable(_wrapt.CallableObjectProxy):
def __init__(self, function):
if isinstance(function, composable):
function = function.__wrapped__
if not callable(function):
raise TypeError('composable argument must be callable')
super().__init__(function)
def __or__(self, other):
if not callable(other) or isinstance(other, type):
return NotImplemented
return type(self)(_compose.sacompose(other, self.__wrapped__))
def __ror__(self, other):
if not callable(other) or isinstance(other, type):
return NotImplemented
return type(self)(_compose.sacompose(self.__wrapped__, other))
def __repr__(self):
return _reprshed.pure(self, self.__wrapped__)
def __reduce_ex__(self, protocol):
return (type(self), (self.__wrapped__,))
class _ComposedObjectProxy(_compose.sacompose, _wrapt.ObjectProxy):
__slots__ = '_wrappers'
def __init__(self, *functions):
_compose.sacompose.__init__(self, *functions)
_wrapt.ObjectProxy.__init__(self, functions[-1])
def __reduce_ex__(self, protocol):
return (type(self), tuple(reversed(self.functions)))
def compose_decorator(*functions):
return _functools.partial(_ComposedObjectProxy, *functions)
def composable_instances(cls):
return _ComposedObjectProxy(composable, cls)
compose = composable_instances(_compose.sacompose) |
Actually, even though it seems possibly valuable to provide a Once we do that, it becomes way clearer that we can just simplify the implementation down to using @_wrapt.decorator
def _composable_instances(wrapped, instance, args, kwargs):
return composable(wrapped(*args, **kwargs))
def composable_instances(cls):
if not isinstance(cls, type):
raise TypeError('composable_instances argument must be callable')
return _composable_instances(cls) But I don't necessarily love the loss of Also, I want to explicitly recap the justification for including a |
So, latest draft: import compose as _compose
import reprshed as _reprshed
import wrapt as _wrapt
def _name(obj):
return type(obj).__name__
class composable(_wrapt.CallableObjectProxy):
__slots__ = ()
def __init__(self, function):
if isinstance(function, composable):
function = function.__wrapped__
if not callable(function):
raise TypeError(_name(self) + '() argument must be callable')
super().__init__(function)
def __or__(self, other):
if not callable(other) or isinstance(other, type):
return NotImplemented
return type(self)(_compose.sacompose(other, self.__wrapped__))
def __ror__(self, other):
if not callable(other) or isinstance(other, type):
return NotImplemented
return type(self)(_compose.sacompose(self.__wrapped__, other))
def __repr__(self):
return _reprshed.pure(self, self.__wrapped__)
def __reduce_ex__(self, protocol):
return (type(self), (self.__wrapped__,))
class composable_instances(_wrapt.ObjectProxy):
__slots__ = ()
def __init__(self, cls):
if not isinstance(cls, type):
raise TypeError(_name(self) + '() argument must be a class')
super().__init__(cls)
def __call__(self, /, *args, **kwargs):
return composable(self.__wrapped__(*args, **kwargs))
__repr__ = composable.__repr__
__reduce_ex__ = composable.__reduce_ex__
compose = composable_instances(_compose.sacompose) Besides the simplification to eliminate the composed proxy and compose decorator helpers:
|
Next problem: now there's an annoying interaction between compose(composable(compose(f)), g) only half-flattens to compose(compose(f), g) instead of becoming This is because It gets worse. compose(composable(compose(f, g)), h) isn't just suboptimally unflattened, it's outright wrong: compose(f, compose(f, g), h) This happens because One solution would be to have So hopefully I can think of a way to fix this within But maybe I will convince myself that it's really The Right Way (tm) to solve this in |
I guess I could change It won't flatten, but it's much simpler than trying to work (Also the thing that's super annoying about using def _unwrap_object_proxies(obj, classinfo):
while not issubclass(type(obj), classinfo):
obj = obj.__wrapped__
return obj I guess using So yeah if I'm going the full unwrap route, I think I should just go with that function above in all variants. |
There was a time when I used Ultimately I talked myself into switching because the performance gain seemed worth it: But if I was using |
I'm not sure if the Because So And So that leaves either breaking all the object proxy behaviors, thereby defeating the purpose of using But |
I thought of a solution: the operator overloads will unwrap def __or__(self, other):
if not callable(other) or isinstance(other, type):
return NotImplemented
+ if isinstance(other, composable):
+ other = other.__wrapped__
return type(self)(_compose.sacompose(other, self.__wrapped__))
def __ror__(self, other):
if not callable(other) or isinstance(other, type):
return NotImplemented
+ if isinstance(other, composable):
+ other = other.__wrapped__
return type(self)(_compose.sacompose(self.__wrapped__, other))
I will not do the switch from
Also, I'm dropping the
Which brings us to: import compose as _compose
import reprshed as _reprshed
import wrapt as _wrapt
def _name(obj):
return type(obj).__name__
class composable(_wrapt.CallableObjectProxy):
__slots__ = ()
def __init__(self, function):
if isinstance(function, composable):
function = function.__wrapped__
if not callable(function):
raise TypeError(_name(self) + '() argument must be callable')
super().__init__(function)
def __or__(self, other):
if not callable(other) or isinstance(other, type):
return NotImplemented
if isinstance(other, composable):
other = other.__wrapped__
return type(self)(_compose.sacompose(other, self.__wrapped__))
def __ror__(self, other):
if not callable(other) or isinstance(other, type):
return NotImplemented
if isinstance(other, composable):
other = other.__wrapped__
return type(self)(_compose.sacompose(self.__wrapped__, other))
def __repr__(self):
return _reprshed.pure(self, self.__wrapped__)
def __reduce_ex__(self, protocol):
return (type(self), (self.__wrapped__,))
class composable_instances(_wrapt.ObjectProxy):
__slots__ = ()
def __init__(self, cls):
if not isinstance(cls, type):
raise TypeError(_name(self) + '() argument must be a class')
super().__init__(cls)
def __call__(self, /, *args, **kwargs):
return composable(self.__wrapped__(*args, **kwargs))
__repr__ = composable.__repr__
__reduce_ex__ = composable.__reduce_ex__
def force_composable(cls):
return composable(_compose.sacompose(cls)) |
A problem remains: fixing this in the overloads only fixes the actual intended use-case of # A user composes a composable callable:
g_of_f = composable(f) | g
# then passes the composable callable elsewhere:
someone_elses_code(g_of_f, ...)
# inside someone else's code:
def someone_elses_code(function, ...):
...
compose(..., function, ...)
... |
So, I think I have to fix this in By "have to" I mean I really don't want to revert an an algorithmic complexity improvement (as the number of composed functions grows, But! We can fix this without losing the performance shape of "compose.init" flattening. For example, we can add a class compose:
def __init__(self, *functions):
if not functions:
raise TypeError(_name(self) + '() needs at least one argument')
_functions = []
for function in reversed(functions):
if not callable(function):
raise TypeError(_name(self) + '() arguments must be callable')
if isinstance(function, compose):
- _functions.append(function.__wrapped__)
+ _functions.append(function._wrapped)
_functions.extend(function._wrappers)
else:
_functions.append(function)
self.__wrapped__ = _functions[0]
self._wrappers = tuple(_functions[1:])
+ @property
+ def _wrapped(self):
+ return self.__wrapped__ At first glance, this pattern ensures that a class is always accessing its own wrapped, not itself through the wrapped of a wrapper of itself. However, this sucks because there's no universalizable ruleset which makes this kind of pattern composable in non-coordinating wrappers without tripping over itself much like What we really want to fix that is a way for I said earlier that it's reasonable to force wrappers to take on the responsibility of fulfilling the requirements that subclasses must fulfill, if those wrappers are going to go into code that's part of the implemention of the thing they're wrapping.... and if that was really true and sensible then in combination with this Anyway so this works, but is still broken design. It's just shifting the problem one step over. |
This is why in my original design of Unfortunately that had compatibility and performance downsides, respectively, if I remember right. Might need to re-test that now. |
Another alternative is to go back to what I was suggesting yesterday - unwrap object proxies (note that this is not special-casing for class compose:
def __init__(self, *functions):
if not functions:
raise TypeError(_name(self) + '() needs at least one argument')
_functions = []
for function in reversed(functions):
if not callable(function):
raise TypeError(_name(self) + '() arguments must be callable')
+ while (isinstance(function, compose)
+ and not issubclass(type(function), compose)):
+ function = function.__wrapped__
if isinstance(function, compose):
_functions.append(function.__wrapped__)
_functions.extend(function._wrappers)
else:
_functions.append(function)
self.__wrapped__ = _functions[0]
self._wrappers = tuple(_functions[1:]) |
Of course I could just revert a10c194. It's still very fast.
On CPython in Termux on my Pixel 5, when flattening 1024 compose instances each of which composes 255 functions, the difference that commit makes is still less than three milliseconds. (For context, the other two alternatives I mentioned, At more realistic composed function sizes, like, say, two, the difference is like one or two 10000ths of a second. (One or less for wrapped property or the unwrap object proxy loop.) |
If performance was a non-issue, then I would 100% without a hesitation use And if I actually listen to the results of my own painstaking thought process about how to approach problems like this, I would think: semantics don't have performance, implementations do - so express the desired behavior and trust in optimizations to handle it. |
In a perfect demonstration and vindication of the approach described in the last comment: Cython compiles the |
Okay so with that decided and fixed in import compose as _compose
import reprshed as _reprshed
import wrapt as _wrapt
def _name(obj):
return type(obj).__name__
class composable(_wrapt.CallableObjectProxy):
__slots__ = ()
def __init__(self, function):
if isinstance(function, composable):
function = function.__wrapped__
if not callable(function):
raise TypeError(_name(self) + '() argument must be callable')
super().__init__(function)
def __or__(self, other):
if not callable(other) or isinstance(other, type):
return NotImplemented
if isinstance(other, composable):
other = other.__wrapped__
return type(self)(_compose.sacompose(other, self.__wrapped__))
def __ror__(self, other):
if not callable(other) or isinstance(other, type):
return NotImplemented
if isinstance(other, composable):
other = other.__wrapped__
return type(self)(_compose.sacompose(self.__wrapped__, other))
def __repr__(self):
return _reprshed.pure(self, self.__wrapped__)
def __reduce_ex__(self, protocol):
return (type(self), (self.__wrapped__,))
class composable_instances(_wrapt.ObjectProxy):
__slots__ = ()
def __init__(self, cls):
if not isinstance(cls, type):
raise TypeError(_name(self) + '() argument must be a class')
super().__init__(cls)
def __call__(self, /, *args, **kwargs):
return composable(self.__wrapped__(*args, **kwargs))
__repr__ = composable.__repr__
__reduce_ex__ = composable.__reduce_ex__
def force_composable(cls):
return composable(_compose.sacompose(cls)) Note that I'm keeping this part: if isinstance(other, composable):
other = other.__wrapped__ I can't tell if the the rationale for it which I gave earlier was too distorted by my craving for a solution, but in either case:
|
I basically removed one of these earlier, but it's debatable if it's worth adding these by default: compose = composable_instances(_compose.compose)
acompose = composable_instances(_compose.acompose)
sacompose = composable_instances(_compose.sacompose) For now I'm leaning "no" personally, but in this separate composable-with-operator wrapper library, I would probably be much more receptive to just piling that stuff in if someone asked. That's more of the kind of library that's maybe worth making a kitchen sink of convenience features, after all. |
So yeah, I think the above is finally a minimal viable product, by my standards - something I could in good conscience polish with some docstrings and maybe some portability cruft, and then ship as a module. One thing I would love to add:
Two things I often imagine adding, but which might not actually bring much value to add, and which I maybe only imagine adding because of an unhealthily over-active empathy for niche use-cases:
And finally, one thing I think might be way too complicated to do well, and would have a bunch of fragile edge-cases, and is probably better done as yet another library layered on top of this one I've been coming up with layered on top of
|
@aalekhpatel07 so, finally! would this be a good fit for all the use cases you had in mind? (Composing with I'll probably go ahead and release it to PyPI soon, hopefully tomorrow but might drag out longer depending on what life throws at me. In the meantime I'd love to hear any feedback, concerns, or critique that you may have about it. Implementation: # SPDX-License-Identifier: 0BSD
# Copyright 2022 Alexander Kozhevnikov <mentalisttraceur@gmail.com>
"""TODO"""
__all__ = ('composable', 'composable_instances')
__version__ = '1.0.0-almost'
from compose import sacompose as _compose
from wrapt import CallableObjectProxy as _CallableObjectProxy
from wrapt import ObjectProxy as _ObjectProxy
import reprshed as _reprshed
def _name(obj):
return type(obj).__name__
class composable(_CallableObjectProxy):
"""TODO"""
__slots__ = ()
def __init__(self, function):
"""TODO"""
if isinstance(function, composable):
function = function.__wrapped__
if not callable(function):
raise TypeError(_name(self) + '() argument must be callable')
super().__init__(function)
def __or__(self, other):
"""TODO"""
if (not callable(other)
or (isinstance(other, type) and isinstance(self, type))):
return NotImplemented
if isinstance(other, composable):
other = other.__wrapped__
return type(self)(_compose(other, self.__wrapped__))
def __ror__(self, other):
"""TODO"""
if (not callable(other)
or (isinstance(other, type) and isinstance(self, type))):
return NotImplemented
if isinstance(other, composable):
other = other.__wrapped__
return type(self)(_compose(self.__wrapped__, other))
def __repr__(self):
"""TODO"""
return _reprshed.pure(self, self.__wrapped__)
def __reduce_ex__(self, protocol):
"""TODO"""
return (type(self), (self.__wrapped__,))
@classmethod
def force(cls, other):
"""TODO"""
return cls(_compose(other))
class composable_instances(_ObjectProxy):
"""TODO"""
__slots__ = ()
def __init__(self, cls):
"""TODO"""
if not isinstance(cls, type):
raise TypeError(_name(self) + '() argument must be a class')
super().__init__(cls)
def __call__(self, /, *args, **kwargs):
"""TODO"""
return composable(self.__wrapped__(*args, **kwargs))
__repr__ = composable.__repr__
__reduce_ex__ = composable.__reduce_ex__ Usage examples: def f(whatever):
...
def g(whatever):
...
composable(f) | g # composable(sacompose(g, f))
f | composable(g) # composable(sacompose(g, f))
@composable
def h(whatever):
...
h | f # composable(sacompose(f, h))
f | h # composable(sacompose(h, f))
h | g | f # composable(sacompose(f, g, h))
class C:
...
h | C # composable(sacompose(C, h))
composable(C) | f # composable(sacompose(f, C))
@composable
class D:
...
D | f # composable(sacompose(f, D))
D | C # TypeError because ambiguous with type union
composable.force(D) | C # composable(sacompose(C, D))
D | composable.force(C) # composable(sacompose(C, D))
D | f | C # composable(sacompose(C, f, D))
f | D | C # composable(sacompose(C, D, f))
@composable_instances
class F:
def __call__(self, whatever):
...
F() | f # composable(sacompose(f, F()))
C | F() # composable(sacompose(F(), C))
@composable
@composable_instances
class G:
def __call__(self, whatever):
...
G | G() # composable(sacompose(G(), G))
async def a(whatever):
...
# Everything below needs an await, unlike any of the above:
h | a # composable(sacompose(a, h))
a | h # composable(sacompose(h, a))
D | a # composable(sacompose(a, D))
a | D # composable(sacompose(D, a))
@composable
async def b(whatever):
...
f | b # composable(sacompose(b, f))
b | f # composable(sacompose(f, b))
C | b # composable(sacompose(b, C))
b | C # composable(sacompose(C, b)) (Just adding this for the design record: the reason I changed |
No description provided.