clarified Nursery.start vs Nursery.start_soon #972

rotu · 2019-03-07T09:22:16Z

Clarified the difference between start_soon and start. Mentioned that start_soon does not guarantee order.

codecov · 2019-03-07T09:29:22Z

Codecov Report

Merging #972 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #972   +/-   ##
=======================================
  Coverage   99.52%   99.52%           
=======================================
  Files         102      102           
  Lines       12192    12192           
  Branches      904      904           
=======================================
  Hits        12134    12134           
  Misses         36       36           
  Partials       22       22

codecov · 2019-03-07T09:29:22Z

Codecov Report

Merging #972 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #972   +/-   ##
=======================================
  Coverage   99.52%   99.52%           
=======================================
  Files         102      102           
  Lines       12192    12192           
  Branches      904      904           
=======================================
  Hits        12134    12134           
  Misses         36       36           
  Partials       22       22

oremanj · 2019-03-08T05:40:00Z

docs/source/history.rst

@@ -5,6 +5,42 @@ Release history

 .. towncrier release notes start

+Trio 0.11.0+dev (2019-03-07)


The towncrier generated release notes shouldn't get checked in until we actually tag the release; the only change you would make is adding the new newsfragment file.

docs/source/reference-core.rst

oremanj · 2019-03-08T05:47:34Z

docs/source/reference-core.rst

-      actually get a chance to do anything until some later point when
-      you execute a checkpoint and the scheduler decides to run it.
-      If you want to run a function and immediately wait for its result,
+      immediately, *before* it has a chance to run. At the next


I think this is a more stringent guarantee than we actually want to make -- #32 has some discussion of scheduling models that might want us to not switch tasks at every checkpoint.

Rewrote to not guarantee that tasks will launch at the next checkpoint.

oremanj · 2019-03-08T05:50:34Z

newsfragments/970.doc.rst

@@ -0,0 +1 @@
+Clarified documentation around Nursery.start_soon and Nursery.start, that start_soon does not guarantee task ordering, and when you should use each.


@njsmith - do we usually add newsfragments for documentation updates?

@oremanj @njsmith Any opinions? I don't care whether we leave it in or take it out, but I'd like this pull request resolved

oremanj · 2019-03-08T05:53:46Z

docs/source/reference-core.rst

@@ -878,9 +881,9 @@ Nursery objects provide the following interface:
      The conventional way to define ``async_fn`` is like::

         async def async_fn(arg1, arg2, *, task_status=trio.TASK_STATUS_IGNORED):
-             ...
+             ... # some work before the parent may proceed


A task with task_status=trio.TASK_STATUS_IGNORED can also be started with start_soon(), in which case the parts before and after task_status.started() behave the same as each other. Is there a different way to word this comment that might be clearer about that?

reworded to "some work the parent might wait for"

This reverts commit 82807c7.

…le code

rotu · 2019-03-09T05:55:23Z

Thank you, @oremanj for your thorough and patient feedback. I'm not used to contributing to open source, and I appreciate your help greatly. Aside from the newsfragment note (which I'd be happy to go either way on) I think I addressed all your concerns.

oremanj

Thanks for your contribution and for bearing with me on these changes! Here are a few more things I noticed on a second read -- sorry for being so nitpicky about this, but it's a pretty core aspect of the documentation and discusses a way in which Trio is unusual compared to other concurrency libraries, so I think it makes sense to polish extra-hard.

oremanj · 2019-03-09T06:26:33Z

docs/source/reference-core.rst

-      finished initializing itself, and optionally returns some
-      information from it.
+      Like :meth:`start_soon`, but blocks until the new child task
+      signals that it may unblock, optionally returning additional


"signals" is a slightly overloaded word to use here due to Unix signals. Maybe "until the new child task indicates that it's finished starting up"? And I think the original "and optionally returns some information" is better than "optionally returning additional information" -- the more active phrasing tends to communicate more clearly IMO.

changing to "indicates"

oremanj · 2019-03-09T06:28:17Z

docs/source/reference-core.rst

+      later), all tasks requested with :meth:`start_soon` will begin,
+      in no particular order. If you want to run a function to
+      completion, then you don't need a nursery; just use
+      ``await async_fn(*args)``. If you want to run some part of the


maybe "If you want to wait for the child task to finish starting up before the parent is allowed to proceed"

No, because "finish starting up" is not a meaningful concept until you've already bought into the abstraction of Nursery.start(). A task scheduled with start_soon might never call a task_status.started() function - but any programmer would say that task starts and ends and infers that it "finishes starting" somewhere in between. There needs to be some concept of the child signaling to the parent, even if we don't use the word "signal".

oremanj · 2019-03-09T06:29:55Z

docs/source/reference-core.rst

+      time, (which might be closing of the nursery context, but no
+      later), all tasks requested with :meth:`start_soon` will begin,
+      in no particular order. If you want to run a function to
+      completion, then you don't need a nursery; just use


The function always runs "to completion" (as opposed to being abandoned in the middle somewhere) regardless of whether you use await or nursery.start_soon. What are you trying to communicate here that you didn't feel was adequately communicated with "and immediately wait for its result"?

The start function runs a function and immediately waits for a result too. It just so happens that the result might be returned by an intermediate result from a .started call in the middle.. I wanted to convey the difference. How about "If you want to run a function and block until it finishes" ?

oremanj · 2019-03-09T06:34:04Z

docs/source/reference-core.rst

-      continuing, see :meth:`start()`.
+      immediately, *before* it has a chance to run. At some future
+      time, (which might be closing of the nursery context, but no
+      later), all tasks requested with :meth:`start_soon` will begin,


This still could be read to imply that all of the tasks will be started at once, which isn't a guarantee we want to make. What if anything do you think is unclear about taking the previous wording ("won't actually get a chance to do anything ... the scheduler decides to run it") and adding a sentence like "Tasks might start running in a different order than the calls to start_soon()"?

You're right - it does imply a single time. Rephrased to "Tasks requested with :meth:start_soon will begin, at no particular time, in no particular order (but no later than the closing of the nursery context)."
I wanted to avoid mention of the scheduler here. The important thing is that ordering and timing get arbitrary when you call start_soon - I don't think the reader will get that unless they have a good grasp of what the scheduler does and does not promise.

rotu · 2019-03-09T08:30:05Z

sorry for being so nitpicky about this, but it's a pretty core aspect of the documentation

No problem! This documentation cost me personally hours of frustration and confusion. I'm more than happy to hammer it to death.

rotu · 2019-05-10T19:51:11Z

What's left to fix for this PR?

pquentin · 2019-07-05T09:33:24Z

@rotu I'm sorry that you had a bad experience here despite a very promising start. I personally don't feel qualified enough to look at this, but hopefully @oremanj will be able to look at this again at some point.

rotu added 2 commits March 7, 2019 03:19

clarified documentation around Nursery.start vs start_soon

975f55c

Clarified the difference between start_soon and start. Mentioned that start_soon does not guarantee order.

Update history.rst

82807c7

oremanj reviewed Mar 8, 2019

View reviewed changes

rotu added 4 commits March 8, 2019 23:28

Revert "Update history.rst"

1d4c098

This reverts commit 82807c7.

Rewrote changes with some VERY helpful feedback from oremanj

c235dee

reworded what the code before task_status.started() is for in examp…

d5ae8ec

…le code

reverted stray `

7ddfddd

oremanj reviewed Mar 9, 2019

View reviewed changes

Polish locution

d4cd696

Removed extraneous "all"

57cd417

rotu closed this Jul 4, 2019

Zac-HD mentioned this pull request Mar 5, 2023

Document that new tasks from nursery.start_soon() may start in any order #2596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clarified Nursery.start vs Nursery.start_soon #972

clarified Nursery.start vs Nursery.start_soon #972

rotu commented Mar 7, 2019

codecov bot commented Mar 7, 2019

codecov bot commented Mar 7, 2019

oremanj Mar 8, 2019

rotu Mar 9, 2019

oremanj Mar 8, 2019

rotu Mar 9, 2019

oremanj Mar 8, 2019

rotu Mar 12, 2019

oremanj Mar 8, 2019

rotu Mar 9, 2019

rotu commented Mar 9, 2019

oremanj left a comment

oremanj Mar 9, 2019

rotu Mar 9, 2019

oremanj Mar 9, 2019

rotu Mar 9, 2019

oremanj Mar 9, 2019

rotu Mar 9, 2019

oremanj Mar 9, 2019

rotu Mar 9, 2019 •

edited

Loading

rotu commented Mar 9, 2019

rotu commented May 10, 2019

pquentin commented Jul 5, 2019

		@@ -5,6 +5,42 @@ Release history

		.. towncrier release notes start

		Trio 0.11.0+dev (2019-03-07)

		@@ -0,0 +1 @@
		Clarified documentation around Nursery.start_soon and Nursery.start, that start_soon does not guarantee task ordering, and when you should use each.

clarified Nursery.start vs Nursery.start_soon #972

clarified Nursery.start vs Nursery.start_soon #972

Conversation

rotu commented Mar 7, 2019

codecov bot commented Mar 7, 2019

Codecov Report

codecov bot commented Mar 7, 2019

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rotu commented Mar 9, 2019

oremanj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rotu Mar 9, 2019 • edited Loading

Choose a reason for hiding this comment

rotu commented Mar 9, 2019

rotu commented May 10, 2019

pquentin commented Jul 5, 2019

rotu Mar 9, 2019 •

edited

Loading