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

Allow composing using '+' instead of explicit compose() calls. #1

Closed
wants to merge 2 commits into from

Conversation

aalekhpatel07
Copy link

No description provided.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jul 28, 2021

[Edit 2023-10-07: compose-operator.]

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.

Copy link
Owner

@mentalisttraceur mentalisttraceur left a 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

  1. I think this functionality should be provided separately from the compose class itself.

  2. 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.

  3. I am undecided if I want to take on supporting any implementation of operator overloading for function composition in Python.

  4. 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.

  5. 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).

  6. 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.

  7. 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):
Copy link
Owner

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.")
Copy link
Owner

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):
Copy link
Owner

@mentalisttraceur mentalisttraceur Jul 30, 2021

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.)

Copy link
Owner

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.

Comment on lines +65 to +67
return compose(*self.functions, *other.functions)
elif callable(other):
return compose(*self.functions, other)
Copy link
Owner

@mentalisttraceur mentalisttraceur Jul 30, 2021

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 too. (Debatable if acompose should be subclass of compose, and someone could maybe even argue that we should never be subclassing compose, but that's a separate issue - the point is that when writing instance methods, doing this usually helps subclasses inherit more correct behavior.)

Copy link
Owner

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.

Comment on lines +64 to +69
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.")
Copy link
Owner

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.

Comment on lines +14 to +35
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
Copy link
Owner

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).

Copy link
Owner

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.

Comment on lines +39 to +40
def f(s):
return s + 'f'
Copy link
Owner

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:

Suggested change
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'
Copy link
Owner

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).

Comment on lines +44 to +51
raised = False

try:
f_g_wrapped = f_wrapped + g
except ValueError as err:
raised = True
finally:
assert raised
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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:

Suggested change
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


Copy link
Owner

@mentalisttraceur mentalisttraceur Jul 30, 2021

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.

@mentalisttraceur
Copy link
Owner

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 forbiddenfruit to monkeypatch <class 'function'> (which you can access by calling type on any function) to add an operator overload (which works!) but that does not cover callable classes. Trying the same thing to monkeypatch object does not work on at least some systems, and can cause wonderful failures like segmentation faults when trying to do various things, and you can override builtins.object with your own subclass but classes declared without an explicit inheritance from object won't inherit from the overwritten one, so that's where that approach hits a dead end. In PyPy, you cannot monkeypatch either class. I haven't tested Jython, IronPython, MicroPython, Transcrypt (unlikely because I think it uses raw JS functions), Skulpt (maybe possible because it's more removed from the JS than Transcrypt), or Brython. Because obviously there's little point in seeing if it works on those other ones when it doesn't work on CPython and PyPy, which are the biggest Pythons.

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 compose double as a decorator which wraps a callable with a class which implements +. Another alternative which partially achieves the desired goal is a decorator which adds the relevant methods onto a decorated class.)

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 async, I started with something like this:

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 __add__ or whatever. (This is basically what I tested when using forbiddenfruit to monkeypatch <class 'function'>.)

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Aug 15, 2021

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 __add__ on that class (and a reverse version, which does compose(other, self), for __radd__). But obviously for arbitrary callables that we can't or shouldn't mutate, we'd want to implement a wrapper class.

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 @composable_with('+'), which picks __add__ and __radd__ as the method names to use based on the '+' argument it gets, and so on for other operators, rather than assuming one.

@mentalisttraceur
Copy link
Owner

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.

@mentalisttraceur
Copy link
Owner

One thing we quickly notice though, is that if compose does not implement these methods, then we can't just return compose instances from the operator overload anymore, because then you can't chain it. The returned object has to behave both like the current compose, and have the relevant operator overloading methods.

So this suggests either

  1. add the methods on compose (accept hard-coding or global choice of operator),
  2. add the methods on a compose subclass (can have subclass for each overload as needed), or
  3. add the methods on a class which contains a compose instance (composition vs inheritance).

@mentalisttraceur
Copy link
Owner

Also, once we start thinking about a generic decorator, we probably really want an intelligent, general-purpose proxy.

See, compose only cares about composition. But if we have a decorator like @composable_with('*'), then suddenly we can imagine people putting that on a class or function during class declaration, and then they might be interested in accessing other attributes or methods on the decorated thing, sometime after decoration and before function composition through the overloaded operator.

This is something that simple compose can simply disregard. You want to compose(foo, bar) where foo already implements << for something like monadic bind? compose doesn't really need to care because when you call compose(...) you sorta accept that the result is an opaque-ish compose object. But if you put @composable_with('@') at the top of a class or function definition, then you don't have the same expectations. Since using the result of a raw compose as a method sounds like an unlikely use case, we can just say "wrap it in a function" and give people a reasonable recipe for it. But if it's a generic decorator, then the odds of people slapping it onto a method seem higher. And we probably want my_class_instance.my_method + my_function to compose as a bound method, so then we start needing to worry about the descriptor protocol, __get__, etc.

So then I start thinking about maybe using something like wrapt, because that takes care of all of those details.

@mentalisttraceur
Copy link
Owner

To some extent I'm thinking up some unlikely possible uses here, but affordances breed usage - we might add + overload to compose expecting it to just provide shorthand in code which otherwise would do a couple compose(...) calls, but once that's there, I think the odds of people starting to use compose as a decorator go up significantly, because now the functionality of compose affords that usage - makes it a lot easier and simpler.

When I saw this PR and started mentally exploring the idea, I very quickly started thinking about doing something like @compose on classes as a shorthand to add function composition operator overloads to functions and callable classes. That's kinda where this naturally goes.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Aug 15, 2021

For completeness, here's one possibility I picture doing for async support:

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 compose/acompose calls are for when you need to programmatically assemble multiple callables when you don't know their name or number ahead-of-time, or you need the in-line brevity and pickling benefits of a compose call, and those situations are relatively rare in Python, and you generally both do know and need to know if you're going to need await on the end result or not. Whereas operators are obviously more like write-convenience shorthand, and I can easily imagine wanting to compose both regular and asynchronous callables together with the same overloaded operators. Also, the equivalent of two different functions is two different operators overloaded - maybe there's a good argument for that though, at least in some situations.

Basically, when you have compose(foo, bar) and you know that either foo or bar are allowed to be async, you just do acompose(foo, bar) instead. But if you have foo + bar and you know one of them can be or is async, then what? In that case I would expect function-composing + produce an async callable. And most importantly, I would not want whether or not I get compose(foo, bar) or acompose(foo, bar) to be determined purely by whether the left side of the composition operator was async or not.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Aug 15, 2021

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 async considerations or letting users pick their choice of operator, then it's trivial to just do a reduced version of what you already did in this PR, as a couple lines of subclassing within whatever project you're working on. (Composition might be better than inheritance for behavior sharing, but you could reasonably argue that this is true subclassing, and even if it isn't, you can just blame Python for making behavior sharing by inheritance more ergonomic than behavior sharing by compositon).

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 compose library.

I'd be very curious to see some representiative code where you're using + overloaded to compose, or where you feel like it would make the code more readable or clearer or shorter or even just nicer to write.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Sep 22, 2021

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 compose is actually when composing a variable amount of functions.

This has no natural/native Pythonic language:

compose(*functions)

Notably, operator overload doesn't do anything for us there.

compose(f, g, h) is better than an equivalent lambda expression in several ways (repr-able, pickle-able, shorter and simpler and easier to type and to read). But is compose(f) + g + h good in places where we would otherwise use a lambda? I don't think so. In fact I can't think of a specific literal line where I'd be for compose(x) + y over compose(x, y).

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 + overload in effect, etc, and actually in that situation I would just do:

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 compose library.

@mentalisttraceur
Copy link
Owner

Ok, I've decided I don't want to implement this inside the compose library, so I'm going to close this PR.

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 compose.

I fully endorse subclassing compose and acompose to experiment with providing this functionality in your own projects. Right now I consider that kind of purely additive subclassing of compose and acompose as part of the supported stable interface of the compose library/package, so you can rely on that working at least so long as the major version stays at v1 per SemVer.

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.

@mentalisttraceur
Copy link
Owner

Considering this again.

I saw a few users of the toolz library asking for something similar (pytoolz/toolz#523). Funny enough that issue was created just a couple months after this PR.

So I was thinking about it more, and thought:

  1. Okay, so the sync-or-async choice in particular is a little finicky, and it does not seem to automatically occur to most people, but if we're in a world that's embracing function composition through overloaded operator, there's some value to making sure that someone is providing an implementation that gets those details right.

  2. I guess the most appropriate place for the implementation of that overloaded operator maybe is in this library, even if providing the overload itself isn't. Because the actual body of the operator method is entirely concerned with the right way to compose two callables.

@mentalisttraceur
Copy link
Owner

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.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented May 11, 2022

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 _isasync to see if it needs to cover more cases.

I also don't want to add a wrapt dependency for compose, so I may release this as a separate library, or do some sort of progressive enhancement thing where it's provided if wrapt is installed and not defined otherwise.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented May 26, 2022

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 compose.__call__, which

  1. isn't marked as async and never does an await, but
  2. uses inspect.isawaitable on each intermediate result just like acompose.__call__ does, and as soon as that's true,
  3. returns without awaiting an awaitable object which does the rest of that composition. More specifically:
    1. creates an async function (or other awaitable), which
      1. takes one parameter,
      2. awaits that parameter,
      3. passes the result of awaiting that parameter into an acompose (or equivalent) of the remaining functions,
      4. awaits and returns the result of that acompose,
    2. calls that awaitable with the first awaitable intermediate result of the composition,
    3. does not await it and returns it.

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 _finish is async but the return _finish(...) doesn't have await anywhere. This basically kicks the async/await responsibility "down the road" to the caller. Invoking _finish (or any async function) returns an awaitable - instead of awaiting it right then and there, we can just return it to the caller as-is, and have them await it.

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 compose-like, with the __call__ function from the last point.

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 self) but for type-hinting purposes, we really want the "types" of the callables (their signatures!). I don't currently provide type hints but it seems like it would be nice to give people.

@mentalisttraceur
Copy link
Owner

Third,

I think I'm pretty close to concluding that a good operator for compose(foo, bar) is bar | foo (notice the reversed order).

I had previously held off on | for that, because a couple years ago, I formed the idea that | was more appropriate for monadic composition rather than plain function composition. But that was largely because when suggesting that the Python sh package to support | I was thinking about how shell pipes are kinda like monads:

  1. If you were implementing pipes in a world where all side-effects had to use I/O monads, then pipes would have to be implicitly feeding the I/O monad through all of them.

  2. It's like you're letting all these "functions" (shell commands) take turns mutating a piece of data (all the bytes piped through all of them), so it's kinda like a state monad in that sense.

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 toolz issue, people settled on the same | choice as well for their composition implementation, so that's good evidence that more people find it intuitive and sensible.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 21, 2022

So in case anyone missed it, I added the sacompose (mnemonics: "sometimes async compose", or "sync/async compose") variant to compose, which makes implementing Correct(tm) universal function-composing operator overloads way nicer.

(Implementing sacompose corresponds to the "First" section of the last batch of design notes.)

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 21, 2022

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__) + ')'

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 21, 2022

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 wrapt and do something like this:

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__) + ')'

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 22, 2022

Couple things I'm noticing:

  1. A really great solution to function composition with operators is still a hefty chunk of code.

  2. A decent amount of the code is either doing the same thing as the compose classes (method binding), or only necessary due to not inheriting from those classes (its own repr, its own call, etc).

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)

@mentalisttraceur
Copy link
Owner

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 | operator: type union.

So |-overloaded function composition would have a surprising breakage:

composable(partial) | int | str would work the same as compose(str, int, partial), but if you factored out the str_int = str | int part, then composable(partial) | str_int would throw a TypeError.

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 str | int | str can never be factored out of a function composition without changing the result.

This is probably fine. But I could see one module slapping a @composable on an exported class, and then some external code using that module's class in a Foo | Bar | Qux type hint, only to have it blow up. (And it doesn't even blow up near the problem - it blows up when the type hint is finally evaluated as a type hint. Although a static type analyzer which doesn't understand decorators could wrong itself into the right result.)

(This also means that any ideas of aggressivelly monkey-patching builtins, like the interactive convenience in a REPL use-case, couldn't monkey patch | for all classes without also breaking type hints.)

So maybe this is a good argument for some other operator, maybe + as originally suggested here. I'm still also fond of @ due to its visual resemblance to ∘. And * has some claim to that visual resemblance as well.

This also leads to an interesting design think direction:

  1. perhaps results of operator composition should be able to work like type unions inside type hint situations.

  2. Perhaps in the monkey patching for interactive convenience use case, the right way to handle the type union problem is to monkey patch __call__ on the type union type.

@mentalisttraceur
Copy link
Owner

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.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 23, 2022

Couple other ways to approach this:

  1. Have composable.__init__ do if isinstance(function, type): raise TypeError('composable arguments must not be classes').

    And maybe have the composition operator do if isinstance(other, type): return NotImplemented.

    Note that people could still wrap classes in regular functions or callable class instances, and then make those composable.

    This is basically a way to force a distinction between "a type (which might also be a callable that creates instances of that type)" and "a function-like callable (which is not a type in itself)". Those are inherently different kinds of thing (concretely: the former is an instance of type, is meant to appear in an MRO, can appear in the second argument of isinstance and issubclass).

  2. Taking monkey-patching even further, in Pythons where this is possible, type can be monkey-patched to have an __or__ and __ror__ which does what we want, and maybe there's a way to create a subclass of types.UnionType or typing.Union that can be called like a composition, and remembers order and duplicates.

    I once again wish to point out that this level of monkey-patching is fairly perverse, and I would only seriously consider this in very special interactive use-cases where a human user wants the convenience of typing | as write-only shorthand.

@mentalisttraceur
Copy link
Owner

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 | type union situation kinda reveals it to be a very fragile toy even if it does work: the more Python code you import and run in such a hacked runtime, the more risk that something somewhere will break.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 25, 2022

Basically if someone is just using this higher-level composable module I'm building up, then they're probably not using compose directly (if at all... one of the things I haven't written up yet is an idea for letting the user plug in other compose implementations, maybe fully decoupling the libraries).

So the only time it matters is if someone is trying to debug code where composes functions made with compose are composed with these operator-composable callables.

Then and only then is composable(compose(...)) an ambiguous repr.

@mentalisttraceur
Copy link
Owner

Yeah on further thought it's actually good to have the repr raise awareness of sacompose. Get people asking questions, that sort of thing.

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 compose module to rename them:

compose -> scompose
sacompose -> compose

@mentalisttraceur
Copy link
Owner

Or even to just drop compose and rename sacompose to compose?

(Of course variants of the code for no-async Pythons would contain the original compose.)

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 26, 2022

Basically, whatever has the name compose is what most people are going to use, and will want to use.

On the one hand, I still think that:

  1. Being consciously mindful of when you suddenly introduce an async function into the mix is very important. Especially because there is a very strong correlation between asynchronicity and mutable state changes and side effects, which are very important to be aware of.

  2. Asynchronicity is the exceptional case. In general asynchronousity should be pushed out to the edges of our logic, at the interface boundaries and so on, where networking and I/O libraries do their thing, while most of the actual interesting logic should be synchronous. And I suspect that function composition is similar to operations like + in being mostly synchronous.

  3. The vast majority of code, when written well and correctly, should either explicitly use compose for synchronous stuff, or acompose for asynchronous stuff. I was very confident in this when I started this library and I think that's still true.

    If a PR came to me using sacompose in a situation what was statically known to produce an async callable, I would immediately tell them to use acompose.

    Similarly: if a library had a function called compose which could sometimes produce async callables, and an scompose which only ever produced sync callables, I would feel some preference for using the always-sync one in statically-known-to-be synchronous contexts. If we have code that expects a synchronous callable, and we compose an asynchronous function into it, then the end result is either a synchronous callable that explodes with a TypeError when trying to do some operation on an awaitable that came from an async function, or a "synchronous" callable that returns an awaitable. Notably, sacompose will always force the problem to be the second case, while an assumes-sync compose will at least try to execute every function.

    In fact, if you have:

    def ensure_sync(x):
        if inspect.isawaitable(x):
            raise RuntimeError('how did an awaitable get in here!?')
        return x

    then you can compose that in with an assumes-sync compose: compose(ensure_sync, ...) and it will call it, and blow up if an async callable slipped in by mistake. But if you do sacompose(ensure_sync, ...) then that will get captured inside the awaitable (async def _finish) which holds the remaining asynchronous composition as soon as an async callable is encountered.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 26, 2022

(So the last comment rules out just totally deleting compose in favor of sacompose. What's left is possibly renaming compose to scompose and sacompose to compose.)

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 26, 2022

Okay so the "on the other hand" case:

  1. If we want that compose(ensure_sync, ...) protection, we're going to have to do it explicitly anyway. Since it has to be a conscious deliberate action for the most safe-against-accidental-async code, having the relevant compose variant be called scompose doesn't seem as likely to be a problem.

  2. ...

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 compose, and in the new wrapt-based composable-with-operator package I'm designing now, I'm going to go with compose = composable_instances(sacompose).

@mentalisttraceur
Copy link
Owner

Ahhh, problem on the ComposedProxy/compose_proxy front: all the object proxy magic makes the compose methods do subtly wrong things (like putting _wrappers on the decorated class).

Hmm...

@mentalisttraceur
Copy link
Owner

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 compose_proxy: the whole point is that it is still a proxy. The problem is just that the implementation details of composing were not intended to leak through the proxy down to the underlying callable.

It's a problem for the compose = composable_instances(_compose.sacompose) case, because I don't want to mutate sacompose like that "from the outside" (I am thinking of all this wrapt-based operator overload stuff as a separate module). Anyway in principle there are many people writing mutating decorators out there, so it's not unacceptably awful, just not the cleanest it could be as far as mutating state.

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 @property on __wrapped__ could work as that piercer function, but I remember that causing problems with the inspect module's stuff.

But that's more flexibility than we strictly need here.

Anyway, the easiest solution is to just make ._wrappers a slot on the composed proxy.

I was initially extremely resistant to this because I was thinking of ._wrappers as a private implementation detail, which this operator composition stuff should treat as unknown and unpredictably subject to change, because this is an independent module building on compose. But! Then I remembered that privacy means something very different when subclassing is involved. If you support subclassing as part of your official API (and compose does) then all your class and instance attributes are part of that subclassing API (basically, when a class has an attribute _foo, the default assumption of "is thay an implementation detail you are not allowed to rely on?" changes when you go from external usage to subclassing).

So okay, I feel fine with that. As the author of compose I am fine with treating ._wrappers as something that I can't change without a breaking version bump. Arguably I should officially document this somewhere, and maybe in my old "Design" section of the README I even commented on it... I certainly did think about it before, it just slipped my mind.

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)))

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 26, 2022

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)

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 27, 2022

Actually, even though it seems possibly valuable to provide a compose_decorator function and composed proxy class, I kinda want to set that aside for a while and trim the focus down to just the composable stuff.

Once we do that, it becomes way clearer that we can just simplify the implementation down to using @wrapt.decorator instead of hand-rolling another subclass of object proxy:

@_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 repr niceness that happens when I use wrapt.decorator. So I'm tempted to still go with an object proxy subclass variant.

Also, I want to explicitly recap the justification for including a composable_instances (because I myself forgot it already, and went on design detours because of that): it's specifically because if you're defining a class, it would be a little tedious and take some care to make the class instances callable. Notably, this is not the case if you're defining a function - if you want your own function to return composable instances, you can just replace return ... with return composable(...). So composable_instances earns its existence by eliminating a specific exceptionally annoying and not-trivial-to-get-right hassle.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 27, 2022

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:

  • since the wrapt object proxies are slotted, add __slots__ = () so as to not lose some of the slots benefits
  • use the name of the class in construction errors' messages
    • bring back my little _name() helper function since I use it for that, even if I don't use it for repr now (I've often debated if reprshed should make its copy of that function public, but holding off for now)
  • use composable() instead of 'composable' or composable class name style in the error messages
  • now that composable_instances is an object proxy class, have it reuse __repr__ and __reduce_ex__ from composable.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 27, 2022

Next problem: now there's an annoying interaction between composable and the compose classes:

compose(composable(compose(f)), g)

only half-flattens to

compose(compose(f), g)

instead of becoming compose(f, g).

This is because composable instances pass the isinstance(function, compose) checks inside compose.__init__, but then just unwrap one step of wrapping by using function.__wrapped__.

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 _functions.append(function.__wrapped__) when function is composable(compose(f, g)) will append the compose(f, g) pulled out from composable's __wrapped__, but then _functions.extend(function._wrappers)will extend with(f,)becausecomposablewill forward the._wrappersaccess to the wrappedcompose` instance.

One solution would be to have compose do function = inspect.unwrap(function, stop=lambda x: issubclass(type(x), compose) before function.__wrapped__ and function._wrappers. But I really don't like loading up compose with that just to handle wrapper classes like this.

So hopefully I can think of a way to fix this within composable.

But maybe I will convince myself that it's really The Right Way (tm) to solve this in compose. It does unfortunately feel that way, since the alternative is the object proxy needing to know and work around implementation details in compose, when the whole point of object proxies is to compose behavior without having to know about the internals.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 27, 2022

I guess I could change isinstance(function, ...) to issubclass(type(function), ...) in compose inits. That seems at a glance like it protects against all wrappers which aren't subclasses.

It won't flatten, but it's much simpler than trying to work inspect.unwrap into the mix. (Also important question: can I trust all wrappers which pretend to be the same class to actually behave like my class? Clearly I couldn't trust .__wrapped__ attribute access to still work the same way, so maybe I shouldn't trust ._wrappers to work on arbitrary wrappers either.)

(Also the thing that's super annoying about using inspect.unwrap is that I can't just use inspect.unwrap - to support Python <3.4, so in the no_async.py variant, I'd have to try: from inspect import unwrap as _unwrap and then except ImportError to define a manual polyfill.)

def _unwrap_object_proxies(obj, classinfo):
    while not issubclass(type(obj), classinfo):
        obj = obj.__wrapped__
    return obj

I guess using inspect.unwrap isn't actually a big benefit.... From a clarity perspective, the loop is arguably clearer. From an optimization perspective: for well-optimizing Pythons like PyPy both should optimize about the same; for stuff like CPython, unwrap has a chance to be implemented in a more optimized way in Pythons like CPython, but each iteration of the loop will have to call into a Python function to figure out if to stop. If that function was also native that could be native code calling native code, but thanks to Python's lack of function-oriented vision, there's no native implementation of partial_right built-in, no operator-reversed version of issubclass, no native implementation of reverse-the-parameters, and so on - basically no way within the builtins and standard library to get a native version of lambda x: issubtype(type(x), classinfo).

So yeah if I'm going the full unwrap route, I think I should just go with that function above in all variants.

@mentalisttraceur
Copy link
Owner

There was a time when I used .functions inside compose.__init__, precisely because it was the cleaner and more universally resilient to weird stuff "public interface"-based approach.

Ultimately I talked myself into switching because the performance gain seemed worth it: .functions has to create a tuple, after all.

But if I was using .functions instead of .__wrapped__ and ._wrappers separately, then this would all "just work".

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 27, 2022

I'm not sure if the composable side can even fix this while remaining the best object proxy it can be, at least any better than the compose side can.

Because .__wrapped__ is a standard special attribute in Python, and because .__wrapped__ is used extensively by all of the wrapt.ObjectProxy method overloads.

So composable can't make its own .__wrapped__ forward to the underlying composition's .__wrapped__, not without making various other things misbehave.

And composable can't change its own behavior just when inside compose.

So that leaves either breaking all the object proxy behaviors, thereby defeating the purpose of using wrapt and to me much of the value of doing , or somehow changing how ._wrappers behaves.

But ._wrappers can't do anything useful besides presenting itself as empty - which is no better than just solving it on the compose side.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 27, 2022

I thought of a solution: the operator overloads will unwrap other if it is a composable instance:

    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))

This feels like an obviously correct move now that I think about it. A generally reusable wrapper cannot expect arbitrary things it is wrapping to work with it. So as a matter of safety against causing weird hard-to-find and hard-to-debug misbehavior, in situations where the wrapper cannot be certain it might not be transparent enough, it should unwrap itself if it knows it doesn't need to be unwrapped. (edit: in retrospect this doesn't seem like a great conclusion... there's something correct in here but not enough - it felt right at the time but I think I was just too motivated to reach a conclusion that let me solve this.)

I will not do the switch from isinstance(..., classinfo) to issubtype(type(...), classinfo) for now. It is tempting to reach for it to provide more general safety against hard-to-debug and hard-to-find misbehavior, but .__class__-based is-instance faking is potentially very useful, and I think it's fair to say that:

  1. a thing that passes the isinstance(thing, SomeClass) check should fulfill all the relevant interfaces of SomeClass, and
  2. "all the relevant interfaces" grows to the interfaces that subclasses are expected to fulfill when you pass thing into code that's part of the SomeClass implementation.

Also, I'm dropping the compose = composable_instances(sacompose) alias, and replacing it with a simplified interface called force_composable, due to reasons accumulating over several days:

  • self-descriptive: the original reason for adding this was to give users something they can use in compositions like composable(SomeClass) | str to force composability by wrapping str;
  • bad to implicitly shadow compose ("change the interface, change the name");
  • bad to implicitly use compose to refer to sacompose and thus foster confusion.

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))

@mentalisttraceur
Copy link
Owner

A problem remains: fixing this in the overloads only fixes the actual intended use-case of |-composition, but it does nothing to help with explicit compositions using one of the compose classes:

# 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, ...)
    ...

@mentalisttraceur
Copy link
Owner

So, I think I have to fix this in compose.

By "have to" I mean I really don't want to revert an an algorithmic complexity improvement (as the number of composed functions grows, (self.__wrapped__,) + tuple(self._wrappers) gets slower, as does every other way of implementing .functions).

But! We can fix this without losing the performance shape of "compose.init" flattening. For example, we can add a property that redirects to __wrapped__.

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 .__wrapped__ trips over itself. The key thing with .__wrapped__ is that it trips over itself when you try to access .__wrapped__ on another object because it identified as an instance of your class, but then it turns out it is actually a wrapper of an instance of your class, so then __wrapped__ is nested.

What we really want to fix that is a way for compose classes to access an attribute which is truly theirs, one that pierces all wrapper classes. Just shuffling the property name around doesn't really do that, because then what if one day a wrapper like composable also needs to implement its own _wrapped?

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 _wrapper property idea, it would be reasonable for a wrapper to just be required to know which private properties a class is using for such purposes, to not collide with them. But that does not seem reasonable in general, and definitely not for wrappers meant to handle arbitrary wrapped objects! Let's say I wanted to give users a way to change the compose implementation used by composable... suddenly compose would have to somehow know to not collide with any of them. More importantly, how can compose presume to impose a restriction on every single possible wrapper class out there like that? The possibility space of possibly useful wrappers is approximately infinite and approximately infinitely variable.

Anyway so this works, but is still broken design. It's just shifting the problem one step over.

@mentalisttraceur
Copy link
Owner

This is why in my original design of compose, I wanted to make functions one tuple, and implement .__wrapped__ with @property or __getattr__.

Unfortunately that had compatibility and performance downsides, respectively, if I remember right.

Might need to re-test that now.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 27, 2022

Another alternative is to go back to what I was suggesting yesterday - unwrap object proxies (note that this is not special-casing for wrapt - this is following the standard Python interface to unwrap any wrapper object which happens to have passed an isinstance check without actually being an instance) that doesn't require any interface changes on the compose classes:

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:])

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 27, 2022

Of course I could just revert a10c194. It's still very fast.

timeit will reveal a slight O(n) performance worsening, but let me put it this way:

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, _wrapped property and the unwrap-object-proxy loop, in the same test, differ by less than one millisecond from the current released version, and unless the relevant objects have recently been accessed, other overheads (I presume cache latencies, but I haven't dug deeper) slow things down more than switching between either variant.)

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.)

@mentalisttraceur
Copy link
Owner

If performance was a non-issue, then I would 100% without a hesitation use .functions.

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.

@mentalisttraceur
Copy link
Owner

In a perfect demonstration and vindication of the approach described in the last comment:

Cython compiles the .functions variant down to code that performs far better when flattening than the unwrap-object-proxies variant.

@mentalisttraceur
Copy link
Owner

Okay so with that decided and fixed in compose v1.4.8, we can get back on track. Recap:

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:

  1. on the one hand, composable wrappers can now safely assume that compose instances can handle them, so that rationale no longer justifies manually unwrapping this, but
  2. at the same time, it's still probably a good idea because there's little point in holding onto extra composable wrappers inside a composition: does anyone really want f | g to create composable(compose(composable(g), composable(f))), instead of composable(compose(g, f))? I imagine no - it's one level of indirection overhead on the composition's callables, never necessary, and would only be (barely) more convenient in the extremely unlikely case that we're programmatically pulling out functions from .functions and then trying to use the composition operator overload on them. Even even worse, what actually happens without this unwrapping-of-other step is that the result is: composable(compose(composable(g), f)). Eww, not even consistent.

@mentalisttraceur
Copy link
Owner

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.

@mentalisttraceur
Copy link
Owner

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:

  1. Type hints! That was one of the benefits of doing a module like this: that type hinting gets much better and easier when there's a binary operator instead of a variadict function for composition.

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:

  1. Ability to plug in a different compose implementation. I see basically three use-cases for this:

    • Maybe someone wants always-async composition or never-async composition, in which case they may want to use acompose, compose, functools.partial(compose, ensure_sync), or some custom function as the composition function instead of sacompose. This pairs extremely well with the next feature idea (ability to choose operator).
    • Maybe someone has a good reason to subclass or reimplement or wrap or compose compose with some sort of really nice feature that I haven't conceived of yet.
    • Maybe someone likes the compose from toolz or funcy or gamla more than my compose.

    composable, composable_instances, and force_composable are already written to be trivially updated to an optional parameter to take compose (this was one of those back-of-mind design things I hadn't gotten around to writing about, non-essential but providing some design traingulation influence):

    import compose as _compose
    import reprshed as _reprshed
    import wrapt as _wrapt
    
    
    def _name(obj):
        return type(obj).__name__
    
    
    class composable(_wrapt.CallableObjectProxy):
        __slots__ = ('_compose',)
    
        def __init__(self, function, compose=_compose.sacompose):
            if isinstance(function, composable):
                function = function.__wrapped__
            if not callable(function):
                raise TypeError(_name(self) + '() argument must be callable')
            super().__init__(function)
            self._compose = compose
    
        def __or__(self, other):
            if not callable(other) or isinstance(other, type):
                return NotImplemented
            if isinstance(other, composable):
                other = other.__wrapped__
            compose = self._compose
            return type(self)(compose(other, self.__wrapped__), compose)
    
        def __ror__(self, other):
            if not callable(other) or isinstance(other, type):
                return NotImplemented
            if isinstance(other, composable):
                other = other.__wrapped__
            compose = self._compose
            return type(self)(compose(self.__wrapped__, other), compose)
    
        def __repr__(self):
            return _reprshed.pure(self, self.__wrapped__, self._compose)
    
        def __reduce_ex__(self, protocol):
            return (type(self), (self.__wrapped__, self._compose))
    
    @classmethod
    def force(cls, obj):
        return cls(cls._compose(obj), cls._compose)
    
    
    class composable_instances(_wrapt.ObjectProxy):
        __slots__ = ('_compose', '_composable')
    
        def __init__(self, cls, compose=_compose.sacompose, composable=composable):
            if not isinstance(cls, type):
                raise TypeError(_name(self) + '() argument must be a class')
            super().__init__(cls)
            self._compose = compose
            self._composable = composable
            # The only use-case I can think of for customizing `composable`
            # is to change which operator is overloaded. Just
            # adding it to the draft for now anyway.
    
        def __call__(self, /, *args, **kwargs):
            return composable(self.__wrapped__(*args, **kwargs), self._compose)
    
        __repr__ = composable.__repr__
        __reduce_ex__ = composable.__reduce_ex__

    I'm on the fence about adding this support for now, but that's the idea, and now it's "ready to go" in case it seems worth it to add it later.

    Plus, this is the thing that made me realize I should move force_composable to be a class method named force on composable. There's that triangulation again. It's actually generally a good idea even if we never let users customize the compose type.

  2. Ability to choose the operator.

    This PR originally proposed +, and one of my reasons for not implementing this in this library was that I didn't think any single operator was clearly the best one for the job. After much thinking, an epiphany that UNIX pipes are in some ways just function composition, and seeing other people reaching consensus on | in the toolz issue on the matter, I ended up settling on | as a good enough choice if people had to be stuck with just one. But I still see some pretty strong reasons why someone might want to support other operators.

    What's interesting is that the choice of | came with constraints that basically no other operator has, so if I was giving users this kind of flexibility, I would want to provide sensible defaults for each operator of what operand types were allowed (luckily, I think right now basically only | is natively overloaded within Python for any callable).

    I am fairly decided that the API for it should look like a function called composable_with, which takes an operator argument which can be either:

    • a string containing one of the operators (composable_with("+")), or
    • one of the operator functions (composable_with(operator.add))

    In fact if I did both the custom-composer and custom-operator features, I might make the compose argument into composable_with as well. One way to do it would be to have composable_with just be a thin wrapper around what is currently class composable, or even just a rename and enhancement of that class, except that class might be renamed to some other name. Another way to do it is to have it work like some sort of decorator around a composable instance, mutating the state of the composable wrapper, but that seems like more boilerplate for no good reason.

    Anyway, I think this is maybe not worth the complexity at this time, but I'd be seriously open to considering it if users wanted it. I do kinda want to think through this at some point: what does the user-chooses-operator version of this interface looks like? And so on. Although just sorta mentally sketching it out, I don't love how boilerplate-y it gets.
    Also! If a person wants to use a different operator overload, or change the order, this is probably good enough for most needs:

    class composable_with_plus(composable):
        __add__ = composable.__ror__
        __radd__ = composable.__or__
       
        # Bonus points if you want to be extra thorough
        # or need to never overtake another callable's
        # overloads of `|`:
        __or__ = wrapt.ObjectProxy.__or__
        __ror__ = wrapt.ObjectProxy.__ror__

    And most of all, in a lot of ways providing this feature feels like a huge amount of code in order to achieve... what, exactly? Encourage and enable fragmentation and bikeshedding, that's for sure, while at best maybe protecting against some extremely rare edge-cases or letting people have something slightly better.

    (Oh, last year when asked for this feature I had suggested that maybe it would be good to create a library that was more general-purpose - let the user choose a syntatical operator and a callable, and implement a more generic wrapper that can handle that. I don't have the drive currently to derail onto that, and there was a lot of details that needed to be thought through painstakingly, specifically for composition. It does still seem like it has some potential though.)

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 compose, but I want to just throw it out there as a big ambitious thought anyway:

  1. It could be nice to hook into the import system, to let users import a module through our library and get a proxy or copied module where every callable is decorated composable and every class is decorated composable instances.

    By the way, one possibly useful feature to maybe add to composable_instances (whether as the default behavior or as an optional parameter or as separate class) is to have the it do if not callable(instance): return instance - basically bypass wrapping instead of erroring out. The idea being, if you expected it to be callable or composable, you'll hit an error soon enough anyway, but this way it's safe to use composable_instances on a class even if you don't know if it's instances are going to be callable (which is a surprisingly tricky problem in Python, because even if you get past the problem of checking if MyClass.__call__ is implemented by the class or by the class' metaclass, __call__ could be something weird like a property which returns a callable, which is callable but basically impossible to statically check for (in fact, slapping an @property on __call__ and then having it return a non-callable is basically the only way I know to trick callable into returning a false positive) - anyway, it's much simpler to check if an instance is callable once you have an instance, than to check if instances will be callable when you only have the class).

    Anyway, I don't currently want to work on an everything-is-composable module proxy like this, and I think The Correct(tm) way to do it is genericized away from just this composable stuff. (But then again I originally thought that the best way to implement compose-as-an-operator was to provide a more general library which is genericized away from just composition - letting you basically make wrappers that overload operators with some function. However, it was much mentally easier to design just a version specialized to compose-as-an-operator, and maybe that's going to turn out to be the case with any module-proxy thing too.

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 28, 2022

@aalekhpatel07 so, finally! would this be a good fit for all the use cases you had in mind? (Composing with |, in order of execution, provided by a separate package which builds on top of this one.)

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 isinstance(other, type) to an (isinstance(other, type) and isinstance(self, type)) is that composable(some_function) | SomeClass is not ambiguous with a type union, but was being wrongly rejected if only other was checked for being a type.)

@mentalisttraceur
Copy link
Owner

mentalisttraceur commented Jun 28, 2022

I've gone ahead and created compose_operator (PyPI, GitHub), to get the names established and so on, now that I'm fairly confident this is worth doing.

I'll bump the version up to 1.0.0 once the docstrings and README are done, if I don't get any comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants