Opened 9 years ago

Closed 8 years ago

Last modified 5 years ago

#6213 enhancement closed fixed (fixed)

Docs for Cooperator and friends don't say what they are for

Reported by: Jonathan Lange Owned by: Jonathan Lange
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: Branch: branches/cooperator-docs-6213
branch-diff, diff-cov, branch-cov, buildbot
Author: jml

Description

The API docs for Cooperator, coiterate and cooperate do an excellent job at explaining how to use them, but don't cover why anyone would want to use them. There's also no such documentation in the howtos.

Attachments (2)

cooperator-docs-6213.patch (2.5 KB) - added by Jonathan Lange 8 years ago.
cooperator-docs-6213-2.patch (2.5 KB) - added by Jonathan Lange 8 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 8 years ago by Jonathan Lange

Owner: set to Jonathan Lange
Status: newassigned

Changed 8 years ago by Jonathan Lange

Attachment: cooperator-docs-6213.patch added

comment:2 Changed 8 years ago by Jonathan Lange

Keywords: review added
Owner: Jonathan Lange deleted
Status: assignednew

Sorry for patch vs branch. Don't have keys on this laptop.

comment:3 Changed 8 years ago by Jonathan Jacobs

It's always good to see the documentation improve, and in particular something like Cooperator.

Not a review but I have a comment: The documentation hints at an answer to the ever popular question of limiting parallelism but doesn't go as far as to explain how you would do it or how it works. However, documenting this aspect of it seems better suited to a howto than a docstring and it would be great to have this useful bit of knowledge in the Twisted documentation.

Also, there's a small grammar error: "it distribute work between all tasks."

comment:4 Changed 8 years ago by Jonathan Lange

Thanks for the comments.

I filed #6693, since I think that the right way to deal with the 'limited parallelism' problem is to provide a helper, and that such is beyond the scope of this ticket.

Good catch on the grammar mistake. Will upload a fix.

Changed 8 years ago by Jonathan Lange

comment:5 Changed 8 years ago by Jonathan Lange

Author: jml
Branch: branches/cooperator-docs-6213

(In [39817]) Branching to 'cooperator-docs-6213'

comment:6 Changed 8 years ago by Jonathan Lange

(In [39819]) Apply Alex_Gaynor's suggestions (refs #6213)

comment:7 Changed 8 years ago by David Reid

Owner: set to David Reid

comment:8 Changed 8 years ago by David Reid

Keywords: review removed
Owner: changed from David Reid to Jonathan Lange

Hey jml,

This new description is great, however when explaining the difference between coiterate and cooperate you only really talk about coiterate it might be helpful to point out that cooperate returns an object that can be paused, resumed, and stopped for greater control over the execution of the task.

And a few formatting issues.

  • The opening and the closing of the docstrings should be on a line by themselves. https://github.com/twisted/twisted/compare/trunk...cooperator-docs-6213#L1R500
  • We appear to use L{} even when referring to the class that is currently being documented (based on documentation from twisted.web.client.Response). So C{Cooperator} should probably be L{Cooperator}}.
  • L{coiterate}. They are equivalent, but C{coiterate} returns a Second reference to coiterate should use L{}.

Once these are addressed you should force a build so we can make sure no new pydoctor errors were introduced.

comment:9 Changed 8 years ago by Jean-Paul Calderone

"""Cooperative task scheduler.

This is not the docstring style used by Twisted. The old style:

"""
Cooperative task scheduler.

was correct.

This new description is great, however when explaining the difference between coiterate and cooperate you only really talk about coiterate it might be helpful to point out that cooperate returns an object that can be paused, resumed, and stopped for greater control over the execution of the task.

Indeed. I think cooperate should be encouraged and coiterate should be discouraged. cooperate is strictly more functional and a lot of people seem to get confused by the word coiterate.

Also, for Cooperator to achieve maximum effect, the global instance should be used in most cases. Multiple Cooperators do not cooperate with each other!

comment:10 Changed 8 years ago by Jonathan Lange

(In [39824]) Correctly formatted docstring (refs #6213)

comment:11 Changed 8 years ago by Jonathan Lange

(In [39825]) Always link to methods & classes, rather than just the first time (refs #6213)

comment:12 Changed 8 years ago by Jonathan Lange

(In [39827]) Recommend cooperate over coiterate (refs #6213)

comment:13 Changed 8 years ago by Jonathan Lange

(In [39830]) Recommend global cooperator (refs #6213)

comment:14 Changed 8 years ago by Jonathan Lange

Keywords: review added
Owner: Jonathan Lange deleted

Thanks for the feedback!

dreid:

  • have outlined cooperate more
  • fixed docstring formatting
  • link everywhere (fwiw, I was following the practice used by wikipedia & others of only linking the first reference)

exarkun:

  • fixed docstring formatting
  • have recommended cooperate over coiterate (I thought it was the other way around, and that CooperativeTask existed more for internal use only. Glad to learn otherwise.)
  • have recommended the global cooperator, specifically by recommending task.cooperate.

I've changed the ordering of the paragraphs and made a couple of spelling fixes along the way.

comment:15 Changed 8 years ago by hawkowl

Keywords: documentation,reviewdocumentation review

comment:16 Changed 8 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:17 Changed 8 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Jonathan Lange
Status: assignednew

Thanks jml.

The new docstrings are very helpful.

Notes:

Points:

  1. Some sentences which confused me, or which I thought could be clearer (and my suggested changes):
    1. "then work will only resume after it fires" > "then the Cooperator will pause until the deferred fires"
  1. "doing one thing, waiting for a Deferred to fire, doing the next thing, repeat " > "serializing asynchronous tasks"
    1. but isn't that just a deferred callback chain?
  1. "Multiple Cooperator's do not" > "Cooperators do not"
    1. don't think the apostrophe is necessary.
    2. also what exactly does this mean?
      1. Does it mean that you *can* have multiple cooperators but you shouldn't need to unless...eg you create a Cooperator subclass with a special scheduling policy.
  1. If we want to encourage the use of the global cooperater then I'd have thought you should move (or copy) this documentation to that function (task.cooperate) so people see it clearly in the task API index and visit it before delving into task.Cooperator.
  1. Also it might be even clearer if task.cooperate was moved above task.Cooperate in the source code...which I think will cause pydoctor to present them in that order in the API docs.
  1. The ticket description also promises some howto documentation - which I think is badly needed.
    1. If you don't want to add it here please create a followup ticket for it.
  1. http://as.ynchrono.us/2006/05/limiting-parallelism_22.html
  1. That's exarkun's document that I've referred to in the past, but in it he's not using the global cooperate (maybe it didn't exist at the time)
  1. Is there a good external description of the pattern that Cooperate implements? If so, it would be nice to link to it
  1. Maybe this? https://en.wikipedia.org/wiki/Computer_multitasking#Cooperative_multitasking.2Ftime-sharing

I think it's a great improvement anyway.

So +1 from me if you address or answer the numbered points above. Or resubmit for another review if you want a another opinion.

-RichardW.

comment:18 Changed 8 years ago by Richard Wall

Hmm. My carefully numbered list got mangled...but hopefully you get the point.

comment:20 Changed 8 years ago by Jonathan Lange

(In [39855]) Respond to rwall's review comments (refs #6213)

comment:21 Changed 8 years ago by Jonathan Lange

(In [39856]) Add some 'why' documentation to cooperate (refs #6213)

comment:22 Changed 8 years ago by Jonathan Lange

Keywords: review added

Thanks Richard.

  1. Fixed
  1. Supplemented, rather than replaced.
  1. Apostrophe fixed, no clarifying text added. See below.
  1. I believe the documentation for the global cooperator is adequately suited to its purpose. See below.
  1. Interesting notion. The layout as-is makes the code very clear, so I'd prefer to keep it this way.
  1. Howto is definitely out of scope. The comment in the original ticket was there to emphasise that I'd checked everywhere, rather than to suggest that all of these places should have documentation. Nevertheless have filed #6724.
  1. That's because the global cooperator is not appropriate for that use case. As mentioned earlier in the thread, I think the right way to solve the limited parallelism confusion is just to add the helper (see #6693)
  1. I'd defer to exarkun on that one, and would rather omit a potentially useful link than include a potentially misleading one.

In general, the more I learn about cooperator, the more it seems that most users will not need to use it directly: they'll want cooperate and eventually parallel. Thus it's appropriate for the docstring of the class to go into a bit of detail about its power and the approach it takes, and likewise appropriate for the global cooperate function to have high-level documentation, focusing on its most common use case.

comment:23 Changed 8 years ago by Richard Wall

Ok. I scanned through the changes and noticed one mistake.

  • r39856 "Just break each task up into so" -- there's something missing.

Fix that, add a .doc news file and please merge.

comment:24 Changed 8 years ago by Jonathan Lange

(In [39857]) Fix thinko in cooperate docstring (refs #6213)

comment:25 Changed 8 years ago by Jonathan Lange

Resolution: fixed
Status: newclosed

(In [39860]) API documentation for twisted.internet.task.Cooperator

  • Author: jml
  • Reviewers: Alex_Gaynor, dreid, exarkun, rwall
  • Fixes: #6213

Extends existing API documentation by providing some context on the class as a whole and suggesting some reasons to use it.

comment:26 Changed 5 years ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.