Decorators called multiple times

[cdonovick]

Ive discovered a bug (maybe undocumented requirement is a better term) that is hard to resolve.

Currently given:

@wrapper2
@end
@...
@begin
@wrapper1
def foo(...): ...

end will exec:

@wrapper2
@wrapper1
def rewritten_foo(...): ...

so in effect we get

@wrapper2 # from the original code
@wrapper2 # in the execed code
@wrapper1
def rewritten_foo(...): ... 

So wrapper2 is called twice. Now this is pretty easy to fix, instead of stripping decorators between begin/end I just need to strip decorators before begin (in terms of line numbers).

end will exec:

@wrapper1
def rewritten_foo(...): ...

so in effect we get

@wrapper2 # from the original code
@wrapper1
def rewritten_foo(...): ... 

However, everything I just said has a slight error. Not only is wrapper2 called twice so is wrapper1. wrapper1 is first called on foo and then its return value is feed into begin and so forth. It is then called a second time on rewritten_foo.

I don't really see anyway to avoid this as I can't undo the call and I can't assume the changes will be somehow propagated. For example if wrapper1 sets an attr then it must be called a second a time as rewritten_foo will not have that attribute set. However if wrapper1 does something stateful or has differing behavior when called on foo and rewritten_foo then perhaps calling a second time is bad.

So I propose that we require rewrites be the innermost decorators.

[cdonovick]

Actually this requirement might slightly annoying for peak. So I think we just need to be okay with inner decorators being called multiple times.

[cdonovick]

Okay it turns out that this really hard to get right in the face of multiple rewrite groups

Suppose I use the strip all decorators before a rewrite group as I suggested (I will use an apply_rewrites style instead of begin/end for the sake of brevity)

@wrapper1
@apply_rewrites([...])
@wrapper2
@apply_rewrites([...])
@wrapper3
def foo(...): ...

Now the first apply_rewrites will strip everything other than wrapper3 so we end up with the following being execed and returned to wrapper2:

@wrapper3
def rewriten_foo(...): ...

Now after wrapper2 returns the second apply_rewrites queries the function for its ast and gets the preceding back. So it applies its rewrites and returns the following to wrapper1

@wrapper3
def rewritten_rewriten_foo(...): ...

So in the end we have:

@wrapper1
@wrapper3
def rewritten_rewriten_foo(...): ...

or if we think about apply_rewrites as a more normal decorator:

@wrapper1
@wrapper3
@apply_rewrites([...])
@wrapper2
@wrapper3
@apply_rewrites([...])
@wrapper3
def foo(...): ...

Note how wrapper2 is not called on the rewritten_rewriten_foo

[cdonovick]

Now one possible solution is to lie. By that I mean serialized a file with only the rewriter stripped but execute the version with all decorators before the rewrite group stripped.

So given:

@wrapper1
@apply_rewrites([...])
@wrapper2
@apply_rewrites([...])
@wrapper3
def foo(...): ...

the first apply_rewrites would execute:

@wrapper3
def rewriten_foo(...): ...

but serializes:

@wrapper1
@apply_rewrites([...])
@wrapper2
@wrapper3
def rewriten_foo(...): ...

Now when the second apply rewrites it would get the preceding as the ast and would execute:

@wrapper2
@wrapper3
def rewritten_rewriten_foo(...): ...

and serialize:

@wrapper1
@wrapper2
@wrapper3
def rewritten_rewriten_foo(...): ...

So in the end we would get:

@wrapper1
@wrapper2
@wrapper3
def rewritten_rewriten_foo(...): ...

or

@wrapper1
@wrapper2
@wrapper3
@apply_rewrites([...])
@wrapper2
@wrapper3
@apply_rewrites([...])
@wrapper3
def foo(...): ...

Which is about as close to correct as we can hope to get without forbidding multiple rewrite groups / requiring rewrites be the innermost decorators.

[leonardt]

I think the idea of effectively applying all the non-rewrite decorators to the final rewritten tree seems to make sense. Assuming those decorators don't do rewrites to the tree (that you would want a subsequent rewrite to see), but it seems difficult/impossible to support this case.

Is there a potential use case for a decorator being applied before a rewrite that would cause the result to change if it was applied after?

Perhaps one simple conceptual model that we could also try is differentiating between compile/run times (ala macros), so AST rewrites happen before regular decorators which happen at runtime. I think your last solution effectively does this (but allows the decorators to be arbitrarily ordered). One potential issue is side-effects in decorators (e.g. state or globals that cause multiple invocations to have different behavior).

What is a peak code example that uses a decorator before applying a rewrite?

[cdonovick]

Is there a potential use case for a decorator being applied before a rewrite that would cause the result to change if it was applied after?

I am not sure if there is a real use case but I could certainly construct an example:

def bar(...): ...

def stupid_wrapper(fn):
      return bar

@stupid_wrapper
def foo(...): ...

Applying stupid_wrapper before and after rewrites has very different effects.
I feel like this a pathological case though and there is really no good way to deal with it.

One potential issue is side-effects in decorators (e.g. state or globals that cause multiple invocations to have different behavior).

Agreed I think we just have to live with this though.

What is a peak code example that uses a decorator before applying a rewrite?

This is more of code organization thing rather than an actual requirement but family.assemble automatically applies rewrites to __call__.
https://github.com/cdonovick/peak/blob/master/tests/test_constructors.py#L39
https://github.com/cdonovick/peak/blob/master/peak/family.py#L95

However we also decorate __call__ with name_outputs.
Now it turns out I am already kinda hacking around this (I had forgotten about my hack before right now, it was previously required because the rewrites destroyed type annotations) so it's not really a big problem. Also now that we can construct products in line name_outputs is not technically even necessary. Further if for some reason the hack was undesirable and we didn't want to force people to construct products we could always move name_outputs to a class decorator. So long story short PEak in fact does not really require decorators prior to rewriting but does use them.
https://github.com/cdonovick/peak/blob/master/peak/family.py#L185

[cdonovick]

Actually the destruction of type annotations is an example of why apply a decorator before and after rewrites might have different effects. Now moving to cst will fix that as they preserve type annotations but for now it does matter.

[leonardt]

If possible, I would vote for having rewrites be applied first in the execution order of decorators, so having some convention where passes are run before "normal" python decorators are run. This seems like the simplest organization and allows for some easier assumptions in our design (treat it more like a "compile" time macro or something). I think having these rewrites play nicely with more conventional runtime behavior modifications used in Python will likely be really difficult to have work in the general case.

[cdonovick]

I don't really follow. There is no way to get rewrites to be applied before other decorators (unless of course they are innermost). Now a lot of the complexity disappears if we forbid multiple rewrite groups but I think fundamentally inner decorators must be called multiple times and if our users don't like that they will have to not make them inner decorators.

[leonardt]

Yea, I was advocating for them to be the innermost

[leonardt]

(rewrites)

[cdonovick]

Hmmm... I think in principle I agree but I also want to be able to apply rewrites automatically and, not simultaneously prevent people from using decorators. I think the just saying that any decorator inside of a rewrite may be called multiple times seems fine. Also the destruction of type annotations by rewrites is pretty problematic.

[leonardt]

I was assuming we could preserve the type annotations, but I guess that involves a big effort to migrate to CST (but we're doing this anyway?). Allowing decorators inside as long as they are "idempotent" does seem okay as a relaxation

[cdonovick]

I think I am going to go with my approach of serializing and executing different things. If the rewrites are the innermost than there is no strange side-effects and performs "reasonably" when they are not.