DM-39294: Refresh directed graph colors #238

leeskelvin · 2023-05-19T19:43:24Z

Checklist

ran Jenkins
added a release note for user-visible changes to doc/changes

timj · 2023-05-19T19:49:09Z

Can you please add some motivation for this change to the commit message?

codecov · 2023-05-19T20:06:11Z

Codecov Report

Patch coverage: 80.00% and project coverage change: -0.29 ⚠️

Comparison is base (3e79529) 85.71% compared to head (610f934) 85.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
- Coverage   85.71%   85.43%   -0.29%     
==========================================
  Files          47       47              
  Lines        4200     4215      +15     
  Branches      722      724       +2     
==========================================
+ Hits         3600     3601       +1     
- Misses        448      461      +13     
- Partials      152      153       +1

Impacted Files	Coverage Δ
python/lsst/ctrl/mpexec/dotTools.py	`52.28% <75.00%> (+2.28%)`	⬆️
tests/test_dotTools.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

leeskelvin · 2023-05-19T20:17:59Z

Added this commit message text:

The current pipeline directed graph nodes use dark gray as their background color. It has been reported that it is difficult to read the black text on the dark gray background. This ticket updates the directed graph color scheme with an aim towards making node text easier to read. In the process of exploring what color schemes would be optimal to satisfy the aim of this ticket, it emerged that making use of the Rubin visual identity colors may be desirable. This will help to make LSST pipeline graphs more instantly recognizable as Rubin-associated products. Colors: https://rubin.canto.com/g/RubinVisualIdentity

andy-slac

Looks good, a couple of comments.

andy-slac · 2023-06-12T19:00:40Z

python/lsst/ctrl/mpexec/dotTools.py

@@ -91,9 +108,10 @@ def _renderQuantumNode(

 def _renderDSTypeNode(name: str, dimensions: list[str], file: io.TextIOBase) -> None:
    """Render GV node for a dataset type"""
-    labels = [name]
+    pointSize = int(_ATTRIBS["defaultNode"].get("fontsize", 12)) + 4


Hard-coded values are usually not welcome, could you add "fontsize" attribute to _ATTRIBS["task"] instead?

There already is a fontsize attribute being set in defaultNode, which will be applied to all nodes (including task nodes). The line you've highlighted here is a safety net that will try to get fontsize from defaultNode, but, if someone were to remove fontsize down the line for some reason, it will fall back to a default value of 12.

If you're not happy with setting this default here, I can remove the 12 and simply allow the code to fail at some future date if/when someone were to remove the fontsize attribute from defaultNode. Perhaps failing then is safer in any case?

I was mostly thinking about + 4, but now I see that fontsize to task attributes also changes size of all labels, while intent here is to change the size of task name only. Maybe a separate "task-name" key can be added to _ATTRIBS to hold that value? Default 12 does not do much here, indeed, may be worth dropping it or defining a separate module-level constant for it instead?

How about I replace this logic entirely with an externally-set _NODELABELPOINTSIZE attribute, set to 18? It's not just tasks that are impacted, it also impacts dataset type label names too. The intent behind the +4 was that, if the user did change fontsize, the labels would still be somewhat larger. However, I'm happy to forego that convenience if hardcoding it makes it easier to read/maintain?

👍 to _NODELABELPOINTSIZE.

andy-slac · 2023-06-12T19:07:18Z

python/lsst/ctrl/mpexec/dotTools.py

-    label = r"\n".join(labels)
-    attrib_dict = dict(_STYLES[style], label=label)
-    attrib = ", ".join([f'{key}="{val}"' for key, val in attrib_dict.items()])
+    label = r"</TD></TR><TR><TD>".join(labels)


I'm not very familiar with how graphviz handles HTML, do you need to escape special HTML characters here (imagine someone adds an ampersand to task name)?

According to the GraphViz docs (https://graphviz.org/doc/info/shapes.html#html), yes, HTML strings will be processed like HTML input. Do we think this will be a problem for us?

I'm using HTML styling to achieve two things here: custom line-spacing and setting the label font size to be somewhat larger than the remaining body text. I could live without the font resizing, but the line-spacing is an issue (using \n only allows me integer line spacing, which is limiting).

I'm OK with HTML, but I think we need to do proper escaping of strings, just in case people start using fancy names. There is a html.escape() method that should take care of that (but you need some care not to escape actual tags, e.g. <I> that you pass to _renderNode.

The current pipeline directed graph nodes use dark gray as their background color. It has been reported that it is difficult to read the black text on the dark gray background. This ticket updates the directed graph color scheme with an aim towards making node text easier to read. In the process of exploring what color schemes would be optimal to satisfy the aim of this ticket, it emerged that making use of the Rubin visual identity colors may be desirable. This will help to make LSST pipeline graphs more instantly recognizable as Rubin-associated products. Colors: https://rubin.canto.com/g/RubinVisualIdentity

leeskelvin · 2023-06-14T20:16:23Z

Thanks @andy-slac. I've added a global _NODELABELPOINTSIZE variable, set to 18, which sets the font size for node labels. I've also added html.escape wrappers around all label text which now lives inside an HTML table. Currently running Jenkins.

leeskelvin force-pushed the tickets/DM-39294 branch from de05a8d to 86e6e6c Compare May 19, 2023 19:46

leeskelvin force-pushed the tickets/DM-39294 branch from 86e6e6c to 04f4f6e Compare May 19, 2023 19:58

leeskelvin force-pushed the tickets/DM-39294 branch from 04f4f6e to 31d67f5 Compare May 19, 2023 20:17

leeskelvin force-pushed the tickets/DM-39294 branch from 31d67f5 to 5eb8abd Compare May 19, 2023 20:20

leeskelvin force-pushed the tickets/DM-39294 branch 19 times, most recently from af388e7 to b70df01 Compare June 12, 2023 16:06

andy-slac approved these changes Jun 12, 2023

View reviewed changes

leeskelvin force-pushed the tickets/DM-39294 branch 2 times, most recently from 59a9075 to a76e6e1 Compare June 14, 2023 13:18

leeskelvin force-pushed the tickets/DM-39294 branch from a76e6e1 to 610f934 Compare June 14, 2023 14:30

leeskelvin merged commit 71390b7 into main Jun 15, 2023
9 of 11 checks passed

leeskelvin deleted the tickets/DM-39294 branch June 15, 2023 03:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-39294: Refresh directed graph colors #238

DM-39294: Refresh directed graph colors #238

leeskelvin commented May 19, 2023 •

edited

timj commented May 19, 2023

codecov bot commented May 19, 2023 •

edited

leeskelvin commented May 19, 2023

andy-slac left a comment

andy-slac Jun 12, 2023

leeskelvin Jun 12, 2023

leeskelvin Jun 12, 2023

andy-slac Jun 12, 2023

leeskelvin Jun 12, 2023

andy-slac Jun 12, 2023

andy-slac Jun 12, 2023

leeskelvin Jun 12, 2023

leeskelvin Jun 12, 2023

andy-slac Jun 12, 2023 •

edited

leeskelvin commented Jun 14, 2023

DM-39294: Refresh directed graph colors #238

DM-39294: Refresh directed graph colors #238

Conversation

leeskelvin commented May 19, 2023 • edited

Checklist

timj commented May 19, 2023

codecov bot commented May 19, 2023 • edited

Codecov Report

leeskelvin commented May 19, 2023

andy-slac 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-slac Jun 12, 2023 • edited

Choose a reason for hiding this comment

leeskelvin commented Jun 14, 2023

leeskelvin commented May 19, 2023 •

edited

codecov bot commented May 19, 2023 •

edited

andy-slac Jun 12, 2023 •

edited