Skip to content

Worker dashboard: More tooltip information on peer comms#5230

Open
gjoseph92 wants to merge 3 commits intodask:mainfrom
gjoseph92:worker_dashboard/peer_comms_tooltips
Open

Worker dashboard: More tooltip information on peer comms#5230
gjoseph92 wants to merge 3 commits intodask:mainfrom
gjoseph92:worker_dashboard/peer_comms_tooltips

Conversation

@gjoseph92
Copy link
Copy Markdown
Collaborator

Show the name(s) of the keys being transferred, and their sizes, in the tooltop. Also show the transfer direction—knowing what blue vs read means is unintuitive at first.

After:
Screen Shot 2021-08-18 at 2 39 03 PM
Before:
Screen Shot 2021-08-18 at 2 45 21 PM

  • Closes #xxxx
  • Tests added / passed
  • Passes black distributed / flake8 distributed / isort distributed

Show the name(s) of the keys being transferred, and their sizes, in the tooltop. Also show the transfer direction—knowing what blue vs read means is unintuitive at first.
Copy link
Copy Markdown
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks for the update and screenshots @gjoseph92! cc @ncclementi if you get a chance to take a look at this

@ncclementi
Copy link
Copy Markdown
Member

Thanks for the update @gjoseph92 I have a few comments and some suggestions. I noticed that in some cases the hovers overlap, for example here. I'm not sure what's the best way of handling this but we might be missing some info due to the overlap.

overlap_1

)

hover = HoverTool(point_policy="follow_mouse", tooltips="""@hover""")
hover = HoverTool(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Most of the hovers now have a different format, which looks like this:

formatted_hover

This are the changes to make it consistent : )

diff --git a/distributed/dashboard/components/worker.py b/distributed/dashboard/components/worker.py
index e3e879c7..1840b3a7 100644
--- a/distributed/dashboard/components/worker.py
+++ b/distributed/dashboard/components/worker.py
@@ -157,12 +157,22 @@ class CommunicatingStream(DashboardComponent):
 
             hover = HoverTool(
                 point_policy="follow_mouse",
-                tooltips=[
-                    ("Transfer", "@transfer"),
-                    ("Keys", "@keys{safe}"),
-                    ("Direction", "$name"),
-                ],
+                tooltips="""
+                <div>
+                    <span style="font-size: 12px; font-weight: bold;">Transfer:</span>&nbsp;
+                    <span style="font-size: 10px; font-family: Monaco, monospace;">@transfer</span>
+                </div>
+                <div>
+                    <span style="font-size: 12px; font-weight: bold;">Keys:</span>&nbsp;
+                    <span style="font-size: 10px; font-family: Monaco, monospace;">@keys{safe}</span>
+                </div>
+                <div>
+                    <span style="font-size: 12px; font-weight: bold;">Direction:</span>&nbsp;
+                    <span style="font-size: 10px; font-family: Monaco, monospace;">$name</span>
+                </div>
+                """,
             )
+
             fig.add_tools(
                 hover,
                 ResetTool(),

@ncclementi
Copy link
Copy Markdown
Member

@gjoseph92 Since we are already modifying the worker's status page, I noticed that the timeseries plot doesn't have labels. If the colors indicate "Incoming" and "Outgoing" too, would you mind adding the labels here? (please note that I assumed that red is incoming and blue outgoing, but correct me if I'm wrong.)

fig.line(source=self.source, x="x", y="in", color="red")
fig.line(source=self.source, x="x", y="out", color="blue")

In a diff, this would look like (label set to the left, to prevent blocking most recent readings)

diff --git a/distributed/dashboard/components/worker.py b/distributed/dashboard/components/worker.py
index e3e879c7..c830bc03 100644
--- a/distributed/dashboard/components/worker.py
+++ b/distributed/dashboard/components/worker.py
@@ -246,9 +246,10 @@ class CommunicatingTimeSeries(DashboardComponent):
             x_range=x_range,
             **kwargs,
         )
-        fig.line(source=self.source, x="x", y="in", color="red")
-        fig.line(source=self.source, x="x", y="out", color="blue")
+        fig.line(source=self.source, x="x", y="in", color="red", legend_label="Incoming")
+        fig.line(source=self.source, x="x", y="out", color="blue", legend_label="Outgoing")
 
+        fig.legend.location = "top_left"
         fig.add_tools(
             ResetTool(), PanTool(dimensions="width"), WheelZoomTool(dimensions="width")
         )

@ncclementi
Copy link
Copy Markdown
Member

@gjoseph92 checking in here. Is this something you are still looking into?

@gjoseph92
Copy link
Copy Markdown
Collaborator Author

Hey thanks for checking in. Thanks for the suggestions, never got around to it since I stopped using this dashboard. If I get the time to this week maybe I can follow up.

Thanks for the example; I didn't know how to do this
@gjoseph92
Copy link
Copy Markdown
Collaborator Author

@ncclementi I made your changes here, thanks!

@ncclementi
Copy link
Copy Markdown
Member

@gjoseph92 thanks for adding those. This is looking good to me!
I have only one question Ithat it's not strictly to a change you made but I was wondering if we should add something.

@ncclementi
Copy link
Copy Markdown
Member

Separate note, we have a problem with hovers overlapping but this problem existed before the changes of this PR so I would leave it for a separate PR. But I'm not sure how we can fix this, it seems to be on bokeh's end. @bryevdv do you have any ideas on how to go about this?

Screen Shot 2021-11-04 at 3 29 38 PM

@bryevdv
Copy link
Copy Markdown
Contributor

bryevdv commented Nov 4, 2021

@ncclementi ability to filter down the number of tooltips dispayed is still an open issue bokeh/bokeh#9087 I seem to recall someone on SO or another forum may have found some hacky CSS workaround to just have one visible, but I have not been able to find the reference.

I've re-triaged the issue to 3.0 but I am going to be disengaged from OSS somewhat for the remainder of the year, so if anyone wants to try to look at it sooner I will happily offer any guidance I can.

A separate issue would be some kind of visual dodge for tooltips but that is probably more work.

Offhand, the immediate workaround would be not not use hover tooltip at all. Instead use a hover callback to update an informational Div outside the plot (or a Label or text glyph in the plot) with the information of your choosing.

@gjoseph92 gjoseph92 requested a review from fjetter as a code owner January 23, 2024 10:57
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.

4 participants