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

Add context-preserving decorator for generators #375

Merged

Conversation

exarkun
Copy link
Collaborator

@exarkun exarkun commented Feb 22, 2019

Fixes #259

In addition to a decorator for plain old generators, this also introduces the tiny extra helper necessary to get Twisted's inlineCallbacks playing nicely with Eliot. This takes the form of a new decorator that just applies both inlineCallbacks and the generator decorator being introduced.

Suggestions for better names for any of this welcome. Let me know if there's any other structural work to be done (docs or whatever).

This work made possible by https://leastauthority.com/

@exarkun
Copy link
Collaborator Author

exarkun commented Feb 22, 2019

Here is a silly little microbenchmark that I wrote to evaluate a couple different implementations against each other. I wasn't able to discern any difference with it, so maybe it's invalid or there was no meaningful performance difference in my candidates.

def benchmark():
    from eliot import start_action

    def measure(label, f):
        from time import clock
        g = f(1000)
        start = clock()
        set(g)
        end = clock()
        print("{} seconds: {}".format(label, end - start))

    @eliot_friendly_generator_function
    def friendly(baseline, depth, n):
        if depth == 0:
            for i in xrange(n):
                with start_action(action_type=u"foo"):
                    if not baseline:
                        yield
        else:
            with start_action(action_type=u"nesting"):
                set(friendly(baseline, depth - 1, n))

    measure("baseline", lambda n: friendly(True, 15, n))
    measure("decorated", lambda n: friendly(False, 15, n))


if __name__ == '__main__':
    benchmark()

I suppose?  Folks should be aware of the incompatibility with asyncio support
I guess.
exarkun added a commit to tahoe-lafs/tahoe-lafs that referenced this pull request Feb 22, 2019
…pport

Add an Eliot-friendly inlineCallbacks-alike decorator.

Fixes: ticket:2973

See also upstream effort: itamarst/eliot#375
@itamarst
Copy link
Owner

Probably want documentation for regular generators too. Might take a stab at that as part of verifying the semantics make sense to me.

---------------------------

Eliot provides a decorator that is compatible with Twisted's ``inlineCallbacks`` but which also behaves well with Eliot's actions.
Simply substitute ``eliot.twisted.inline_callbacks`` for ``twisted.internet.defer.inlineCallbacks`` in your code.
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like this documentation is insufficient, since there is also requirement for use_generator_context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, that's true.

@itamarst
Copy link
Owner

itamarst commented Mar 6, 2019

Just figured out how to use magit with github, I think, so will try to make some local changes and push them to your fork/branch. Which might or might not work? I may need to ask you to give me write access to the fork.

@exarkun
Copy link
Collaborator Author

exarkun commented Mar 6, 2019

Just figured out how to use magit with github, I think, so will try to make some local changes and push them to your fork/branch. Which might or might not work? I may need to ask you to give me write access to the fork.

I thiiiink it will just work. Because of that checkbox that defaults to checked that I probably didn't fiddle with. But, annoyingly, it seems like github doesn't tell you if you left it checked or not? Anyway, let me know if you have trouble.

@itamarst
Copy link
Owner

itamarst commented Mar 6, 2019

Question 1: What should we do about regular generators?

After playing around with some code, I feel like the behavior when you don't decorate it with eliot_friendly_generator is actually closer to what you'd expect: the context at time of iteration is what is used. With the decorator you get context at time of construction. However, with start_action(): yield is completely broken.

(For generator-as-pseudo-coroutine the decorator behavior does make sense.)

So perhaps for generators in general this could be solved by documentation ("don't use start_action() around a yield"). Telling people "you must use this decorator on generators" is also asking them to remember something, so I'm not sure the cognitive load is any different.

Thoughts?

@itamarst
Copy link
Owner

itamarst commented Mar 6, 2019

Question 2: Can we get rid of use_generator_context? Possibly we can have the decorator automatically run it?

@itamarst
Copy link
Owner

itamarst commented Mar 6, 2019

Todo: I would like an API to set the sub-context-factory, and for it to blow up if you set it more than once.

I can imagine this might cause problems for someone somewhere who wants to use Twisted inlineCallbacks in same program as asyncio and Eliot, but given Eliot userbase is small enough that's a rare use-case we can punt until that's really an issue.

@exarkun
Copy link
Collaborator Author

exarkun commented Mar 7, 2019

FWIW, I don't currently have any use-cases involving regular generators. I'm sure they're out there I just haven't bumped into them yet. The codebase I'm working in uses inlineCallbacks a lot and other forms of generators not so much.

@itamarst
Copy link
Owner

itamarst commented Mar 7, 2019

Yeah, I realize this is for inlineCallbacks only, but generators are code people use too, and this is effectively a possible solution for that, so want to figure out what to do while we're at it.

@itamarst
Copy link
Owner

itamarst commented Mar 7, 2019

OK, so I guess for this to be merged I'd like:

  1. Internal API for setting sub-context-factory, which complains if it's called twice with different factories.
  2. @eliot_friendly_generator_etc. should probably be private, and it should automatically set the sub-context-factory, unless that turns out to be too tricky (since typically this is done at import time I'm not too worried about performance... but might need a lock.)
  3. Documentation for generators that basically says "don't do yield inside with start_action."

If you'd rather I do the above let me know.

@itamarst itamarst changed the base branch from master to 259.generator-decorator March 14, 2019 21:44
@itamarst
Copy link
Owner

Merging to non-master branch in main repo for continued development.

@itamarst itamarst merged commit 8e467bd into itamarst:259.generator-decorator Mar 14, 2019
@exarkun exarkun deleted the 259.generator-decorator branch March 14, 2019 22:30
tomprince pushed a commit to tp-la/eliot that referenced this pull request Jul 14, 2021
…orator

Add context-preserving decorator for generators
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