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

Don't recommend using addCallback as a function decorator #2523

Merged
merged 2 commits into from Dec 13, 2016

Conversation

Projects
None yet
6 participants
@wallrj

wallrj commented Dec 1, 2016

addCallback returns a Deferred.

A decorator should return a callable.

In [10]: d = succeed(object())

In [11]: @d.addCallback
def foo(result):
    pass
   ....: 

In [12]: foo
Out[12]: <Deferred at 0x7f6d5ca75170 current result: None>
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Dec 1, 2016

@wallrj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @djmitche, @tardyp and @tychoish to be potential reviewers.

mention-bot commented Dec 1, 2016

@wallrj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @djmitche, @tardyp and @tychoish to be potential reviewers.

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Dec 2, 2016

Member

Well, I think this is a useful style for writing inline anonymous(ish) callabacks, so I'll be inclined to keep this style.
Those functions are not supposed to be reused outside of the addCallbacks, so I'd say it's okay that addCallbacks does not return the callback.

Member

tardyp commented Dec 2, 2016

Well, I think this is a useful style for writing inline anonymous(ish) callabacks, so I'll be inclined to keep this style.
Those functions are not supposed to be reused outside of the addCallbacks, so I'd say it's okay that addCallbacks does not return the callback.

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Dec 6, 2016

Member

@wallrj I don't want to forcefully close this PR as you may have more arguments against this practice.
Here is an email on python-idea suggesting that this is an existing practice (I couldn't find another)
https://mail.python.org/pipermail/python-ideas/2007-December/001279.html

Please let me know why you think this is a bad idea.
Apart from its not really a decorator, if this is the only argument, then we can change this PR with a warning about that.

Member

tardyp commented Dec 6, 2016

@wallrj I don't want to forcefully close this PR as you may have more arguments against this practice.
Here is an email on python-idea suggesting that this is an existing practice (I couldn't find another)
https://mail.python.org/pipermail/python-ideas/2007-December/001279.html

Please let me know why you think this is a bad idea.
Apart from its not really a decorator, if this is the only argument, then we can change this PR with a warning about that.

@tardyp tardyp added the needs reply label Dec 6, 2016

@wallrj

This comment has been minimized.

Show comment
Hide comment
@wallrj

wallrj Dec 6, 2016

Hey @tardyp

I've tried using this style before on https://github.com/ClusterHQ/flocker or https://github.com/twisted/twisted/ and it was rejected by @exarkun for the same reasons, I think (but I can't find the reference. )
Perhaps I was doing something subtly different.

Then a colleague of mine, @moshez used this style recently on a private repo and I grumbled about it.

I found a couple of examples, introduced by @glyph in the Twisted code:

This buildbot documentation is the only place I can find it discussed / documented so I decided to start a discussion :-)

I guess the main objection is that addCallback, addErrback and addBoth weren't intended to be used this way and addCallbacks can't be used this way....which makes it a bit of a hack.

wallrj commented Dec 6, 2016

Hey @tardyp

I've tried using this style before on https://github.com/ClusterHQ/flocker or https://github.com/twisted/twisted/ and it was rejected by @exarkun for the same reasons, I think (but I can't find the reference. )
Perhaps I was doing something subtly different.

Then a colleague of mine, @moshez used this style recently on a private repo and I grumbled about it.

I found a couple of examples, introduced by @glyph in the Twisted code:

This buildbot documentation is the only place I can find it discussed / documented so I decided to start a discussion :-)

I guess the main objection is that addCallback, addErrback and addBoth weren't intended to be used this way and addCallbacks can't be used this way....which makes it a bit of a hack.

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Dec 6, 2016

Member

Since we are adding other people, adding @djmitche who write the recommendation in the first place (the git blame says it came from our wiki)

I agree its a bit of a hack, but it imho adds readability compared to

def postProcess(res):
      pass
d.addCallback(postProcess)

Which is written backward imho.

Member

tardyp commented Dec 6, 2016

Since we are adding other people, adding @djmitche who write the recommendation in the first place (the git blame says it came from our wiki)

I agree its a bit of a hack, but it imho adds readability compared to

def postProcess(res):
      pass
d.addCallback(postProcess)

Which is written backward imho.

@exarkun

This comment has been minimized.

Show comment
Hide comment
@exarkun

exarkun Dec 6, 2016

Contributor

It's true the trick does make the order more readable for some folks. However, the resulting code is still rather dense and can be difficult to read overall. A recommendation I would make is that functions should focus on doing one thing well. A function that starts an async operation and then is packed full of nested functions for operating on the result may be just as poorly factored as a synchronous function that does ten unrelated operations.

Instead of defining postProcess inline, consider whether it can be factored out. Then the code in question is just:

    d.addCallback(parseCommandIntoTestResult)

which I think we should be able to agree is more readable than either of the other options presented here.

Contributor

exarkun commented Dec 6, 2016

It's true the trick does make the order more readable for some folks. However, the resulting code is still rather dense and can be difficult to read overall. A recommendation I would make is that functions should focus on doing one thing well. A function that starts an async operation and then is packed full of nested functions for operating on the result may be just as poorly factored as a synchronous function that does ten unrelated operations.

Instead of defining postProcess inline, consider whether it can be factored out. Then the code in question is just:

    d.addCallback(parseCommandIntoTestResult)

which I think we should be able to agree is more readable than either of the other options presented here.

@glyph

This comment has been minimized.

Show comment
Hide comment
@glyph

glyph Dec 6, 2016

Personally I like this style very much. For a while I worried that this was a too-clever impulse (I do have those sometimes) and an abuse of syntax; however, recent teaching experiences have confirmed my suspicions that explaining the @ idiom once is much easier than repeatedly having to point out the subtle ordering issue presented by callbacks appearing before the method invocation that kicks them off. It's a "weird once" vs. "weird every time" sort of difference.

Combined with another (unfortunately somewhat obscure) idiom, it can avoid redundant temporary naming as well as putting things in the right order.

For example, instead of:

d = foo()
def bar(result):
    return doStuff(result)
return d.addCallback(bar)

you can do

passthru = lambda x:x # (or 'from twisted.internet.defer import passthru')
@passthru(foo())
def d(result):
    return doStuff(result)
return d

Notice that bar disappears here.

However, I'd agree with @exarkun in that if you're inclined to have an intense discussion about the merits of this style, chances are that the real issue is that you've got too many callbacks, nested too deeply, and you should consider breaking things up into more top-level functions instead. Buildbot certainly has that problem regardless of its stylistic representation :-).

glyph commented Dec 6, 2016

Personally I like this style very much. For a while I worried that this was a too-clever impulse (I do have those sometimes) and an abuse of syntax; however, recent teaching experiences have confirmed my suspicions that explaining the @ idiom once is much easier than repeatedly having to point out the subtle ordering issue presented by callbacks appearing before the method invocation that kicks them off. It's a "weird once" vs. "weird every time" sort of difference.

Combined with another (unfortunately somewhat obscure) idiom, it can avoid redundant temporary naming as well as putting things in the right order.

For example, instead of:

d = foo()
def bar(result):
    return doStuff(result)
return d.addCallback(bar)

you can do

passthru = lambda x:x # (or 'from twisted.internet.defer import passthru')
@passthru(foo())
def d(result):
    return doStuff(result)
return d

Notice that bar disappears here.

However, I'd agree with @exarkun in that if you're inclined to have an intense discussion about the merits of this style, chances are that the real issue is that you've got too many callbacks, nested too deeply, and you should consider breaking things up into more top-level functions instead. Buildbot certainly has that problem regardless of its stylistic representation :-).

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Dec 6, 2016

Member

I think the problem of nested function, and callback hell is orthogonal to this debate.

With or without @d.addCallback, we all agree that more than 2 inline functions is to be avoid in python code (which btw is far from being the case in the example given twisted/twisted@e331b4e)

Buildbot used to suffer from callback hell, and I think we removed most of that technical debt from the core (we still have a lot of the source steps which is quite hard to follow)

We replaced lots of the callback hell code with inlineCallbacks logic.
This helps a lot readability, but in exchange I fear we traded performance. I may want to reintroduce some callbacks in the critical paths.

Member

tardyp commented Dec 6, 2016

I think the problem of nested function, and callback hell is orthogonal to this debate.

With or without @d.addCallback, we all agree that more than 2 inline functions is to be avoid in python code (which btw is far from being the case in the example given twisted/twisted@e331b4e)

Buildbot used to suffer from callback hell, and I think we removed most of that technical debt from the core (we still have a lot of the source steps which is quite hard to follow)

We replaced lots of the callback hell code with inlineCallbacks logic.
This helps a lot readability, but in exchange I fear we traded performance. I may want to reintroduce some callbacks in the critical paths.

@glyph

This comment has been minimized.

Show comment
Hide comment
@glyph

glyph Dec 6, 2016

This helps a lot readability, but in exchange I fear we traded performance. I may want to reintroduce some callbacks in the critical paths.

If you're concerned about performance, the thing to introduce is benchmarks, not callbacks :-). PyPy might be the right option, or shifting back to callbacks, or perhaps the lower-overhead async/await trampoline in 3.5 instead of the yield-based trampoline (i.e. switch to ensureDeferred instead of inlineCallbacks).

glyph commented Dec 6, 2016

This helps a lot readability, but in exchange I fear we traded performance. I may want to reintroduce some callbacks in the critical paths.

If you're concerned about performance, the thing to introduce is benchmarks, not callbacks :-). PyPy might be the right option, or shifting back to callbacks, or perhaps the lower-overhead async/await trampoline in 3.5 instead of the yield-based trampoline (i.e. switch to ensureDeferred instead of inlineCallbacks).

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Dec 7, 2016

Member

@glyph we do have live profiling:
https://pypi.python.org/pypi/buildbot-profiler/

I was surprised to see inlineCallback trampoline high in some of those profile, but I did not investigate further enough yet. It wasn't obvious which code path(s) those traces were about

Member

tardyp commented Dec 7, 2016

@glyph we do have live profiling:
https://pypi.python.org/pypi/buildbot-profiler/

I was surprised to see inlineCallback trampoline high in some of those profile, but I did not investigate further enough yet. It wasn't obvious which code path(s) those traces were about

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Dec 13, 2016

Member

@wallrj I updated your branch with a (opinionated I concede) summary of the discussion

Member

tardyp commented Dec 13, 2016

@wallrj I updated your branch with a (opinionated I concede) summary of the discussion

Show outdated Hide outdated master/docs/developer/style.rst Outdated
@wallrj

Couple of typos but otherwise looks good to me @tardyp

@wallrj

Actually I spotted a couple of other typos and bad references and there's a sphinx lint failure.
Answer or address those and merge when you get green builds.

Show outdated Hide outdated master/docs/developer/style.rst Outdated
``d.addCallback`` is not really a decorator as it does not return a modified function.
As a result in previous code, ``rev_parse`` value is actually the Deferred.
In general the :class:`inlineCallbacks` method is preferred inside new code as it keeps the code easier to read.

This comment has been minimized.

@wallrj

wallrj Dec 13, 2016

shouldn't this be something like

twisted:func:`inlineCallbacks` 
@wallrj

wallrj Dec 13, 2016

shouldn't this be something like

twisted:func:`inlineCallbacks` 

This comment has been minimized.

@wallrj

wallrj Dec 13, 2016

Otherwise how will it know where to link to when rendering?

@wallrj

wallrj Dec 13, 2016

Otherwise how will it know where to link to when rendering?

This comment has been minimized.

@tardyp

tardyp Dec 13, 2016

Member

It does not this is just used for styling. We don't have back reference helpers to twisted docs

@tardyp

tardyp Dec 13, 2016

Member

It does not this is just used for styling. We don't have back reference helpers to twisted docs

Show outdated Hide outdated master/docs/developer/style.rst Outdated
Show outdated Hide outdated master/docs/developer/style.rst Outdated
@@ -176,6 +186,19 @@ losing any readability.
Note that code using ``deferredGenerator`` is no longer acceptable in Buildbot.
The previous :py:func:`getRevInfo` example implementation should rather be written as:

This comment has been minimized.

@wallrj

wallrj Dec 13, 2016

Same here.

@wallrj

wallrj Dec 13, 2016

Same here.

@tardyp tardyp merged commit a25c812 into buildbot:master Dec 13, 2016

21 checks passed

bb Build done.
Details
bb/coverage/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/docs/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/js/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/lint/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/tarballs/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:14.0.2/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:15.4.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:15.5.0/sqla:0.8.0/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:15.5.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:latest/sqla:latest/db:mysql+mysqldb://travis@127.0.0.1/bbtest Build done.
Details
bb/trial/2.7/tw:latest/sqla:latest/db:mysql+mysqldb://travis@127.0.0.1/bbtest?storage_engine=InnoDB Build done.
Details
bb/trial/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:latest/sqla:latest/db:sqlite:////tmp/test_db.sqlite Build done.
Details
bb/trial_worker/2.7/tw:10.2.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial_worker/2.7/tw:11.1.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial_worker/2.7/tw:12.2.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial_worker/2.7/tw:13.2.0/sqla:latest/db:sqlite:// Build done.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wallrj wallrj deleted the wallrj:patch-1 branch Dec 13, 2016

@wallrj

This comment has been minimized.

Show comment
Hide comment
@wallrj

wallrj commented Dec 13, 2016

👍

@gastlygem

This comment has been minimized.

Show comment
Hide comment
@gastlygem

gastlygem Apr 24, 2017

I'm not sure, but I just can't find the better implementation mentioned here.

gastlygem commented on master/docs/developer/style.rst in 4c069a2 Apr 24, 2017

I'm not sure, but I just can't find the better implementation mentioned here.

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Apr 24, 2017

Member

line 194 of this commit

Member

tardyp replied Apr 24, 2017

line 194 of this commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment