[wip] Playing with generator support for Python 3 so as to give more control to the advisor

[jdelic]

I believe this is the correct fix. I'm still testing, will update as it goes. I fear that yhis project is inactive.

[ionelmc]

Oooof ... so there are some test configuration issues. But the changes seem wrong at first glance - what's the problem you are trying to fix more exactly?

[jdelic]

@ionelmc

hey, awesome that you're here :). Since I started work on this, I have realized that aspeclib's Python 2 and 3 code behave the same. So it seems that this behavior is entirely by design. My code completely deviates from the original idea and that the title of this PR is now misleading, given that the original code was probably not "broken". So let's start by stating what I want to do and how I arrived at the current incarnation of this PR:

What I'm trying to do

In optile/ghconf github.py I'm using aspectlib to recursively patch all objects returned by PyGithub to correctly handle, among other things, GitHub API rate limiting.

There is a class pygithub.github.PaginatedList that is iterable and is used to return lists of objects. A call to __iter__() on an instance of PaginatedList will yield a generator. aspectlib is correctly identifying __iter__() as a generator function and wrapping it in advising_generator_wrapper_py3. When the calling code, for example, calls

from ghconf.github import gh
org = gh.get_organization('myorg')
for repo in org.get_repos():
    # using my code, get_repos() will return a `advising_generator_wrapper_py3`
    pass

I would like to be able to add my Advice to every Repository object iterated over in this loop and modify it as needed as well as influence the execution of the generator.

What this PR does

Where I arrived with this PR is a way to steer the execution of a generator per iteration:

• So Proceed means "iterate once on the wrapped generator" and give me the generated value
• Return means "yield the last value or the one I'm giving to you" in this iteration
• My current WIP PR does not currently allow the caller to send() to the underlying generator which is definitely a problem
• Counter-intuitively if the advisor wants to close the generator it itself has to raise StopIteration.

Also this PR would totally change the behavior of aspectlib, so it's probably a non-starter.

What the original Python 3 code does

The original code employs yield from. I believe that the intention once was to iterate over the wrapped generator and on each iteration send the values to the advisor and get new advice on how to continue, at least that's how I interpret the while True. Instead the orignal code does

• get advice exactly once when the generator is created
• and when that advice is Proceed it generates all results of the cutpoint generator through yield from
• then send the last generated value (when StopIteration was raised) to the advisor, usually None

So, what now... :)

The original code seems to give very little control to Aspects for generators, only allowing them to go ahead or blocking them. Not controlling their contents or execution. But that's what I need to do...

do you have any ideas on how to accomplish that in the original framework?

[ionelmc]

Ooof so there's lots of things going on there:

• aspectlib.weave shouldn't raise any exception. If it does then you either patch the wrong way or there's a bug in aspectlib.weave.
• the aspect is basically a decorator ... if you get it from get_repos then something is wrong with the patching (aka weaving) process
• in the aspect you can get the result and play with inputs/outputs - eg: https://github.com/ionelmc/python-aspectlib/blob/master/tests/test_aspectlib.py#L1277

[ionelmc]

What I would try is to stop weaving at run time. It means you'd have to patch all the classes in github at initialization (and consequently have a bit more coupling than now) but it would be more predictable cause you won't have to weave results during runtime.

[ionelmc]

Another advantage of not doing runtime weaving is that you will never accidentally double weave.

[jdelic]

@ionelmc

• aspectlib.weave shouldn't raise any exception. If it does then you either patch the wrong way or there's a bug in aspectlib.weave.

Oh man, yes, you would think so. However most properties in PyGithub are constructed like this, where a pure read on the attribute can incur a network request. And aspectlib calls all properties on an instance during weave because hasattr(module, alias) for a reason that I don't quite grok yet, calls the property.

In [1]: class Test:
   ...:     @property
   ...:     def a(self):
   ...:         print('call a')
   ...:         return "a"
   ...:

In [2]: hasattr(Test(), "a")
call a
Out[2]: True
# wat?

• the aspect is basically a decorator ... if you get it from get_repos then something is wrong with the patching (aka weaving) process

I think this comment is based on a misunderstanding of the previous PR title or old code in ghconf... I only get the decorated generator, which I believe is the correct behavior. I have not observed different behavior.

• in the aspect you can get the result and play with inputs/outputs - eg: https://github.com/ionelmc/python-aspectlib/blob/master/tests/test_aspectlib.py#L1277

But how can I play with the outputs? Seriously, with the original code (as I pointed out above) I can only get it to request advice exactly once, after that it uses yield from to send the contents of the wrapped generator directly to the caller, without giving the advisor any chance to intervene. Perhaps I'm missing something obvious?

What I would try is to stop weaving at run time. It means you'd have to patch all the classes in github at initialization (and consequently have a bit more coupling than now) but it would be more predictable cause you won't have to weave results during runtime.

I really don't want to have tighter coupling, but to be honest the current approach is also quite intensive and slowing things down a lot, especially with added network requests during weaving 🤷‍♂️ . So perhaps I should investigate "weaving through" PyGithub at startup instead.

[jdelic]

Thanks to your advice I rewrote the weaving to happen at startup by walking the PyGithub module tree. It seems to work (and obviously is much faster) and additionally removes the requirement to be able to see the generator outputs, therefor is works with aspectlib 1.4.2 as is.

Given my comments above I still feel that some improvements could be done in the generator handling, but that's outside of the scope of this discussion I guess :)

[ionelmc]

Open a documentation issue then? ;)

But seriously, what's wrong with the current handling?

[jdelic]

But seriously, what's wrong with the current handling?

An Advice instance bound to a cutpoint generator function can neither inspect or modify the values that the generator returns.

[jdelic]

To answer my own question from above:

because hasattr(module, alias) for a reason that I don't quite grok yet, calls the property.

From the Python docs:

The arguments are an object and a string. The result is True if the string is the name of one of the object’s attributes, False if not. (This is implemented by calling getattr(object, name) and seeing whether it raises an exception or not.)

Obviously 🤦‍♂️

[ionelmc]

An Advice instance bound to a cutpoint generator function can neither inspect or modify the values that the generator returns.

It should be possible. Afaik I considered having support for this but I deemed it unnecessary, inconsistent with regular advicing and too complex. It should be possible but you see it's not a trivial change - need to have type checks and slow unwrapping of every generator. From my perspective it's basically adding cython speedups in aspectlib. It would take serious time.

To answer my own question from above:

because hasattr(module, alias) for a reason that I don't quite grok yet, calls the property.

It's because you're most likely accessing it through the instance. I doubt there's a classproperty or metaclass implementation there that would call the function wrapped by property when accessing something on a class. Just weave the classes instead of the instances.

[jdelic]

An Advice instance bound to a cutpoint generator function can neither inspect or modify the values that the generator returns.

It should be possible. Afaik I considered having support for this but I deemed it unnecessary, inconsistent with regular advicing and too complex. It should be possible but you see it's not a trivial change - need to have type checks and slow unwrapping of every generator. From my perspective it's basically adding cython speedups in aspectlib. It would take serious time.

Yes, I agree.. giving too much control about how the generator is consumed is probably counter-productive and, as you said, inconsistent with regular aspects.

A compromise might be a solution that allows the Advice to return a different generator. Currently when Advice yields Return aspectlib will raise StopIteration. But what if code like this would work instead:

@aspectlib.Advice(bind=True)
def proposal_return_different_generator(cutpoint, *args, **kwargs):
    # we don't want the caller to use the original generator so we...
    # wrap it
    def my_gen_wrapper(gen):
        sent = None
        while True:
            if sent:
                x = gen.send(sent)
            else:
                x = next(gen)
            x += 1  # modify x in some way
            sent = yield x
    # and return our wrapper instead
    yield aspectlib.Return(my_gen_wrapper(cutpoint(*args, **kwargs)))

Advice for generating functions can then either yield Proceed to call the generator function as is, a Proceed instance to modify its parameters, or yield a Return instance to yield a different generator.

[ionelmc]

aspectlib.Return can't do anything else, otherwise it'd break expectations. Generators do have a return value (in addition to items being yielded), and that is what aspectlib.Return is for.

[jdelic]

aspectlib.Return can't do anything else, otherwise it'd break expectations

aspectlib could, for example, behave differently if the Return is yielded by the Aspect before the caller has started consuming the generator or after. You can argue that it's a breaking change, as currently yielding return will immediately raise StopIteration with a value.

Currently, an Aspect can not influence a generator function at all beyond changing its parameters. It can either allow the caller to consume the whole generator (Proceed) or not (Return). It can't change the generator returned by the generator function, but that's what I would expect to be able to do from an AOP library, right?

[ionelmc]

I agree that it should allow influencing the generator, just that this feature shouldn't be met halfway. IMO the only way to support this usecase is to implement a special advice (aspectlib.Yield or similar) .

[jdelic]

Hmm, isn't there a consistency argument to be made here?

Regular function Advice

• aspectlib.Proceed allows the original implementation to go ahead by calling the cutpoint, potentially with changed parameters, and yielding control back to the Aspect with the return value of the cutpoint call
• aspectlib.Return changes the return value for the caller either before or after execution depending on when the Return advice is yielded to either the parameter passed to Return(result) or None
• None or no advice means proceeding and returning the result unchanged

Generator function Advice

• aspectlib.Proceed allows the original implementation to go ahead by calling the cutpoint, potentially with changed parameters, not yielding control back to the Aspect, the Aspect doesn't get to see the return value of the cutpoint call at all
• aspectlib.Return terminates the returned generator either before or after it has been consumed and changes the return value in the StopIteration exception, a value that is commonly ignored.
• None or no advice means proceeding and yielding from the generator unchanged

Wouldn't the consistent behavior for generator functions look like this:

Proposed Generator Function Advice

• aspectlib.Proceed allows the original implementation to go ahead by calling the cutpoint, potentially with changed parameters, yielding control back to the Aspect, returning the generator function
• aspectlib.Return when given a generator as it's parameter starts yielding from the generator it was passed. This generator can immediately raise StopIteration/return to prevent consumption.
• None or no advice means proceeding and yielding from the original generator unchanged.

[ionelmc]

Ok so basically the change you're proposing is to allow aspectlib.Return to replace the wrapped generator with a different one right?

[jdelic]

@ionelmc

Ok so basically the change you're proposing is to allow aspectlib.Return to replace the wrapped generator with a different one right?

not only. In my comment I also propose to change aspectlib.Proceed for generators to behave consistently with how it behaves with functions. Namely, returning control to the Aspect instance giving the generator returned from the cutpoint to the Aspect. Otherwise there is no way for the Aspect to get or inspect the generator.

That's how aspectlib behaves right now. On a generator cutpoint, yielding aspectlib.Proceed will not allow the Aspect to inspect anything about the generator returned by the cutpoint. My comment above describes the current behavior of aspectlib for functions/methods, the current behavior for generators and my proposed behavior that I would feel is more consistent.