Improve differentiation between incoming/outgoing connections and transfers by hendrikmakait · Pull Request #6933 · dask/distributed

hendrikmakait · 2022-08-23T10:20:44Z

The unclear differentiation between incoming/outgoing connections and transfers has made it difficult for me to understand what we are doing. In addition, there were two mistakes in docstrings/messages that we fix.

Following #6936 (comment) while assuming some small liberties, this PR now implements the following changes:

main	new	notes
WorkerState.comm_nbytes	WorkerState.transfer_incoming_bytes
(new property)	WorkerState.transfer_incoming_count	len(self.in_flight_workers)
WorkerState.total_out_connections	WorkerState.transfer_incoming_count_limit
WorkerState.comm_threshold_bytes	WorkerState.transfer_incoming_bytes_throttle_threshold
WorkerState.target_message_size	WorkerState.transfer_message_target_bytes
Worker.outgoing_current_count	Worker.transfer_outgoing_count
(new attribute)	Worker.transfer_outgoing_bytes
Worker.incoming_count	WorkerState.transfer_incoming_count_total	Moved to WorkerState
Worker.outgoing_count	Worker.transfer_outgoing_count_total
Worker.total_in_connections	Worker.transfer_outgoing_count_limit
Worker.incoming_transfer_log	Worker.transfer_incoming_log
Worker.outgoing_transfer_log	Worker.transfer_outgoing_log

Notes:

Skip current suffix
Use total suffix to highlight cumulative sums.
- shorter than cumulative
- if it's not total, it's current
The abundance transfer_-prefixed variables might indicate a missing abstraction

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

… and transfers

hendrikmakait · 2022-08-23T10:24:52Z

distributed/worker.py

-    incoming_count: int
-    outgoing_count: int
-    outgoing_current_count: int
+    incoming_transfer_count: int


Instead of talking about incoming_transfer/outgoing_transfer, which we already had through the {incoming|outgoing}_transfer_log, we could also talk about send/recv to further avoid confusion.

This stuff is "public". we should add a deprecation warning (e.g. by using a property). I'm not sure if any downstream project rely on these attributes

[EDIT] let's keep them but rename with a deprecation. Discussion on #6936 (comment)

hendrikmakait · 2022-08-23T10:34:25Z

distributed/worker_state_machine.py

    comm_nbytes: int

-    #: The maximum number of concurrent incoming requests for data.
+    #: The maximum number of concurrent outgoing requests for data.


Fixing docstring.

hendrikmakait · 2022-08-23T10:34:50Z

distributed/worker.py

        if self.status == Status.paused:
            max_connections = 1
-            throttle_msg = " Throttling outgoing connections because worker is paused."
+            throttle_msg = " Throttling incoming connections because worker is paused."


Fixing throttle_msg.

These are inbound connections for outbound data. Maybe change it to "Throttling outgoing data transfers"?

github-actions · 2022-08-23T12:22:44Z

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 48m 11s ⏱️ + 4m 58s
  3 053 tests +  2   2 969 ✔️ +  1   84 💤 +1 0 ❌ ±0
22 585 runs +16 21 603 ✔️ +10 982 💤 +6 0 ❌ ±0

Results for commit a0faa5f. ± Comparison against base commit c083790.

♻️ This comment has been updated with latest results.

hendrikmakait · 2022-08-23T12:37:19Z

CI flakes: #6896, #6934, #6935

fjetter · 2022-08-23T13:27:23Z

distributed/worker.py

-    incoming_count: int
-    outgoing_count: int
-    outgoing_current_count: int
+    incoming_transfer_count: int


This stuff is "public". we should add a deprecation warning (e.g. by using a property). I'm not sure if any downstream project rely on these attributes

hendrikmakait · 2022-08-23T14:04:55Z

distributed/worker.py

+    @property
+    def incoming_count(self):
+        warnings.warn(
+            "The `Worker.incoming_count` attribute has been renamed to "
+            "`Worker.incoming_transfer_count`",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+        return self.incoming_transfer_count
+
+    @property
+    def outgoing_count(self):
+        warnings.warn(
+            "The `Worker.outgoing_count` attribute has been renamed to "
+            "`Worker.outgoing_transfer_count`",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+        return self.outgoing_transfer_count
+
+    @property
+    def outgoing_current_count(self):
+        warnings.warn(
+            "The `Worker.outgoing_current_count` attribute has been renamed to "
+            "`Worker.current_outgoing_transfer_count`",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+        return self.current_outgoing_transfer_count


Added DeprecationWarnings for the renamed attributes via properties. Setters have been skipped. IIUC, these attributes should not be modified by others. cc @fjetter

hendrikmakait · 2022-08-23T14:30:19Z

distributed/worker.py

-    outgoing_count: int
-    outgoing_current_count: int
+    incoming_transfer_count: int
+    outgoing_transfer_count: int


While we're at it, would total_outgoing_transfer_count be clearer for this?

How about

outgoing_transfer_count_current

outgoing_transfer_count_total

Pros:

differentiation is very clear

can be sorted alphabetically and related stuff is grouped

code autocompletion is straight forward and a developer can search for outgoing_stuff_*

not a strong opinion

discussion on #6936 (comment)

hendrikmakait

Copying @crusaderky's comment for visibility:

@hendrikmakait

To recap our meeting:

rename WorkerState.comm_nbytes to comm_incoming_bytes.
No deprecation in WorkerState needed; just change the already existing DeprecatedWorkerStateAttribute in Worker

new property WorkerState.comm_incoming_count: return len(self.in_flight_workers)

rename Worker.outgoing_current_count to comm_outgoing_count. Add a deprecated property (read-only).

new attribute Worker.comm_outgoing_bytes

rename Worker.incoming_count to comm_incoming_cumulative_count with a deprecated accessor

rename Worker.outgoing_count to comm_outgoing_cumulative_count with a deprecated accessor

heartbeat will now return
comm:
      incoming_bytes: #
      incoming_count: #
      incoming_cumulative_count: #
      outgoing_bytes: #
      outgoing_count: #
      outgoing_cumulative_count: #

update #6933 to match. Prefer "comm" prefix to "transfers" suffix everywhere.

#6936 (comment)

In addition to the suggested renamings, I have renamed Worker.{incoming|outgoing}_transfer_log to Worker.comm_{incoming|outgoing}_log for consistency.

I'm wondering if instead of stripping the word transfer out of names, we should explicitly in for anything that's a data transfer, i.e., a connection but going the other way.

hendrikmakait · 2022-08-29T14:41:12Z

distributed/worker_state_machine.py

            if (
-                len(self.in_flight_workers) >= self.total_out_connections
-                and self.comm_nbytes >= self.comm_threshold_bytes
+                self.comm_incoming_count >= self.total_out_connections


After implementing those changes, I am not a huge fan of comm_incoming_count. This line illustrates why:
Why do we compare comm_incoming_count to total_out_connections? How are they related?

comm_incoming_count still feels vaguely defined to me. This might be because comm_ feels like a namespacing prefix for everything communications-related, so what are we counting exactly in that namespace that's incoming? I might just need to get more used to the fact that when we're talking about comm_, we tend to mean Comm object. Then again, that Comm object feels equivalent to a connection, so why is the comm incoming while the connection is outgoing? Should we get transfer back into the naming here?

Happy to rename comm to transfer everywhere.
comm_incoming_count -> transfer_incoming_count_current
total_out_connections -> transfer_incoming_count_max

crusaderky · 2022-08-29T16:09:27Z

Updated naming on #6936. Please let's keep all naming discussion there from now on.

hendrikmakait · 2022-08-30T09:29:02Z

distributed/dashboard/components/worker.py

            "Ready": [len(w.state.ready)],
            "Waiting": [w.state.waiting_for_data_count],
-            "Connections": [len(w.state.in_flight_workers)],
+            "Connections": [w.state.transfer_incoming_count],


For now, I don't want to touch "Connections".

hendrikmakait · 2022-08-30T09:33:08Z

distributed/tests/test_worker_state_machine.py

 @gen_cluster()
 async def test_deprecated_worker_attributes(s, a, b):
-    n = a.state.comm_threshold_bytes
+    n = a.state.target_message_size


This tests the use of a deprecated attribute with the same name on the WorkerState. Since we renamed comm_threshold_bytes, I had to pick another variable.

fjetter

The abundance transfer_-prefixed variables might indicate a missing abstraction

This is just about instrumentation. We could introduce a dataclass holding these things but I think that'd be overkill.

hendrikmakait · 2022-08-30T13:01:01Z

This is just about instrumentation. We could introduce a dataclass holding these things but I think that'd be overkill.

I don't want to do this now, it might be worth taking a closer look at this once/if we are diving into things like dynamically limiting comms. Currently, the comms-related logic is spread around Worker and WorkerState and I'm not sure the WorkerState should contain any logic relating to memory-aware throttling.

fjetter · 2022-08-30T13:25:28Z

I'm not sure the WorkerState should contain any logic relating to memory-aware throttling.

No, it should not rely on dynamic memory measurements. We're not actually measuring any memory here. We're running off of data provided by the scheduler TaskState.nbytes so this is a static attribute which makes it suitable for the worker state.

hendrikmakait · 2022-08-30T13:30:32Z

No, it should not rely on dynamic memory measurements. We're not actually measuring any memory here. We're running off of data provided by the scheduler TaskState.nbytes so this is a static attribute which makes it suitable for the worker state.

Agreed, as I said, this note was merely meant to be kept in mind for future work such as the dynamic limiting ideas mentioned in #6212 or #6208 (comment).

distributed/utils_test.py

distributed/worker_state_machine.py

crusaderky · 2022-08-30T16:01:58Z

Thank you!

…nsfers (dask#6933)

Improve naming to differentiate between incoming/outgoing connections…

bbe7e18

… and transfers

hendrikmakait commented Aug 23, 2022

View reviewed changes

Typo

485e72f

hendrikmakait marked this pull request as ready for review August 23, 2022 12:37

hendrikmakait requested review from crusaderky, fjetter, gjoseph92 and graingert August 23, 2022 12:37

hendrikmakait self-assigned this Aug 23, 2022

fjetter requested changes Aug 23, 2022

View reviewed changes

Add DeprecationWarnings

cc83764

hendrikmakait commented Aug 23, 2022

View reviewed changes

Add test for deprecation warning

f70cceb

hendrikmakait requested a review from fjetter August 23, 2022 14:19

hendrikmakait commented Aug 23, 2022

View reviewed changes

crusaderky mentioned this pull request Aug 26, 2022

Expose transfer-related metrics in Worker.get_metrics and WorkerMetricCollector #6936

Merged

2 tasks

hendrikmakait added 3 commits August 29, 2022 12:39

Review comments

074707d

Improve docs

2bfdc2e

Rename comm logs

aea0210

hendrikmakait commented Aug 29, 2022

View reviewed changes

hendrikmakait added 2 commits August 29, 2022 18:34

Rename total connections

0742765

comm -> transfer

4b073aa

hendrikmakait force-pushed the naming-for-incoming-outgoing branch from 8bdf7a0 to 4b073aa Compare August 29, 2022 17:20

hendrikmakait added 4 commits August 29, 2022 19:21

Update docs

87599ed

Move transfer_incoming_count_total

3afc878

Adjust test

4fb8b7b

Add deprecated property

1b2034d

hendrikmakait added 5 commits August 30, 2022 11:13

Fix naming

4218493

Fix tests

07c96db

Minor

ce1d051

Fix test

abdc243

Rename test

d28ff03

hendrikmakait commented Aug 30, 2022

View reviewed changes

hendrikmakait added 2 commits August 30, 2022 11:56

rename target_message_size

5d64743

Minor

a0faa5f

fjetter approved these changes Aug 30, 2022

View reviewed changes

crusaderky approved these changes Aug 30, 2022

View reviewed changes

distributed/utils_test.py Outdated Show resolved Hide resolved

distributed/utils_test.py Outdated Show resolved Hide resolved

distributed/worker_state_machine.py Outdated Show resolved Hide resolved

crusaderky added 3 commits August 30, 2022 17:01

Update distributed/utils_test.py

bdfc87c

Update distributed/utils_test.py

1f3dc4f

Update distributed/worker_state_machine.py

b10985a

crusaderky merged commit 34b18a9 into dask:main Aug 30, 2022

crusaderky added a commit to crusaderky/distributed that referenced this pull request Aug 30, 2022

merge dask#6933

de126f7

crusaderky mentioned this pull request Aug 30, 2022

Rename network-related settings in config #6977

Open

gjoseph92 pushed a commit to gjoseph92/distributed that referenced this pull request Oct 31, 2022

Improve differentiation between incoming/outgoing connections and tra…

03823dc

…nsfers (dask#6933)

Uh oh!

Conversation

hendrikmakait commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusaderky Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

hendrikmakait commented Aug 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikmakait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusaderky commented Aug 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjetter left a comment

Choose a reason for hiding this comment

Uh oh!

hendrikmakait commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjetter commented Aug 30, 2022

Uh oh!

hendrikmakait commented Aug 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crusaderky commented Aug 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hendrikmakait commented Aug 23, 2022 •

edited

Loading

crusaderky Aug 26, 2022 •

edited

Loading

github-actions bot commented Aug 23, 2022 •

edited

Loading

hendrikmakait commented Aug 30, 2022 •

edited

Loading