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
Improve documentation for scheduler states #1498
Conversation
@@ -2,7 +2,7 @@ | |||
# | |||
|
|||
# You can set these variables from the command line. | |||
SPHINXOPTS = | |||
SPHINXOPTS = -j4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will build docs in parallel.
|
||
* ``{'task': (inc, 1)}``: a tuple satisfying the dask graph protocol. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting 'task'
there seemed to be a bug.
|
||
A key's priority is only used to break ties, when many keys are being | ||
considered for execution. The priority does *not* determine running order, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The priority does not determine running order" sounds wrong to me, otherwise what would it be used for? The goal is to start computing some tasks earlier than others...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I suspect that my intent here was to say that priority does not fully determine running order. Many other concerns can take precedence. This probably isn't necessary to say though.
docs/source/scheduling-state.rst
Outdated
processing → erred occupancy, idle, used_resources | ||
processing → released occupancy, idle, used_resources | ||
memory → released worker_bytes | ||
memory → forgotten worker_bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The task in tasks
itself is also forgotten if you want to include this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually a bit surprised that the right column of this table is not more dense. I would have expected more worker and task state to be affected. Ah, I see that you are only listing worker state, never mind.
In general this looks great to me. I agree with all of the wording changes. Thank you for going through this. |
I'm +1 on this PR and happy to merge any time. |
Cool, thank you :-) |
Thank you! This seems much clearer to me than before.
…On Thu, Oct 26, 2017 at 7:26 AM, Antoine Pitrou ***@***.***> wrote:
Merged #1498 <#1498>.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1498 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AASszG4u08xW4lLpt5wrQQS6D8AZPrN1ks5swGxagaJpZM4QEzPE>
.
|
This categorizes the state variable in three groups, adds a table of where tasks appear depending on their state, another table listing worker state changes in each task transition, and improves some wording choices in my POV :-)