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

Improve documentation of message-bytes-limit #7077

Merged
merged 4 commits into from
Sep 28, 2022

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Sep 27, 2022

Addresses

Note
It just occurred to me that it might be even better to rename message-bytes-limit to gather-bytes-limit and transfer_message_bytes_limit to transfer_gather_bytes_limit to highlight their intention, but I don't want to keep changing names all the time.

  • Tests added / passed
  • Passes pre-commit run --all-files

cc @wence-, @crusaderky

#: :meth:`BaseWorker.gather_dep`. Multiple small tasks that can be fetched from the
#: same worker will be clustered in a single instruction as long as their combined
#: size doesn't exceed this value.
#: Number of bytes to gather from the same worker in a single call to
Copy link
Member Author

Choose a reason for hiding this comment

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

Driveby: Highlight the fine print here as well.

@hendrikmakait hendrikmakait self-assigned this Sep 27, 2022
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Thanks!

@github-actions
Copy link
Contributor

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   6h 29m 10s ⏱️ + 5m 54s
  3 116 tests ±0    3 029 ✔️  - 1    86 💤 ±0  1 +1 
23 064 runs  ±0  22 155 ✔️ ±0  908 💤  - 1  1 +1 

For more details on these failures, see this check.

Results for commit a5e5f57. ± Comparison against base commit 8c4133c.

Tasks are gathered in batches, and if the first task in a batch is larger than this value,
the task will still be gathered to ensure progress. Hence, this limit is not absolute.
Note that this limit applies to a single ``GatherDep`` and a worker may gather data from
multiple workers in parallel.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The GatherDep class is an implementation detail and shouldn't appear in what is effectively functional documentation. Could you rephrase? e.g. replace ``GatherDep``` with "data transfer between workers"?

Copy link
Member Author

@hendrikmakait hendrikmakait Sep 28, 2022

Choose a reason for hiding this comment

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

I've replaced GatherDep with gathering operation. This maps to the nomenclature we use in our documentation for the worker state machine. A data transfer may be falsely interpreted as a transfer on the comm level, i.e., the transfer of an individual frame, which in turn has its own configuration variable. This is also why I'm not a huge fan of the message- prefix of this configuration variable (as noted in the PR description).

@crusaderky crusaderky merged commit 8f36aa5 into dask:main Sep 28, 2022
gjoseph92 pushed a commit to gjoseph92/distributed that referenced this pull request Oct 31, 2022
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.

3 participants