Draft: ompi_info color coding

[gkatev]

This adds colors to ompi_info, per #10813. It's not yet complete but I'm putting it up for any comments thus far.

Todo:

• ~Detect tty for --color auto~
• ~Fix the escape characters counting towards the line limit~
• Allow customization of colors, probably via MCA param, if possible + try out some other color combinations

Fixes #10813 Signed-off-by: George Katevenis [email protected]

[ompiteam-bot]

Can one of the admins verify this patch?

[rhc54]

I really like the idea - I only wonder if we might encounter pushback from those users who suffer from color blindness? Perhaps some effort should be made to identify suitable colors that they can also see?

[gkatev]

Yes of course, if someone has experience and can chime in about more safe/universal colors I can change them up. And if we also implement the customization, the weight of our choice becomes lighter.

[rhc54]

I wouldn't say I have experience, but a quick search offered up the following that seem helpful:

https://davidmathlogic.com/colorblind/#%23D81B60-%231E88E5-%23FFC107-%23004D40 had a really good explanation and info on the problem, along with suggestions

https://www.nature.com/articles/nmeth.1618 has a useful chart:

[gkatev]

This site offers a way to visualize what the colors look like under different color blindeness conditions: https://laura.rochaprado.com/color-blind-simulator/

The param name and param value already seem to be OK. The valid values color in my cases above in some cases blends with the blue in param name. Some alternatives below.

valid_values=cyan

valid_values=red

valid_values=yellow

I'm open to suggestions about the colors (which ones, and also perhaps what things to color). Various cases could be made about whether we want the colors to stand out, or how they look under different terminal emulator themes and more.

[gkatev]

This is supposedly ready for final review.

I wasn't really sure how to name the MCA param, and where to place it. Also not fully sure about the location of the parsing/assisting code, any suggestions welcome.

Edit: force-pushed a comment fix

[gkatev]

I addressed the thus-far proposed changes

[jsquyres]

This is looking pretty good. I have one question. Check out this example output from ompi_info --all:

• The parameter name is colored: good
• The parameter value is colored: good
• For the enum values, the phrase "Valid values" is colored -- not the permissible values. Was that intentional?

[gkatev]

Yes indeed it was intentional, the rationale being something along the lines of not overwhelming the output with too much color. But of course we could change it, all styling suggestions are welcome.

(One idea, if we wanted more color there but not too much, would be to also color the actual possible values, but not what they represent. The spirit of the color coding is too make finding info faster, so highlighting the possible values sounds in line with that)

[jsquyres]

@gkatev Gotcha. I am notoriously horrible at stylistic kinda of decisions, but it does strike me that it could be good to be consistent: color all the values / possible values.

Also, could you rebase this PR to the current main HEAD? We made some changes to the github CI, and we now require a github action CI test that is apparently present at the HEAD but not from where you originally branched. Thanks!

[gkatev]

Alright yes I can do a quick test tomorrow and show how it would look with the possible values colored, and we can pick whichever seems better.

(I rebased to the latest HEAD)

[gkatev]

Here is with only the valid values colored:

Dark

Light

I think it looks okay, and beyond the stylistic aspect, we highlight the core points: the parameter, its value, and its possible values (or rather, the values' keys...). This one will entail a small code rearrangement, as it also requires support in mca_base_var_enum.c, but it shouldn't be a problem.

Diff for this test:

diff --git a/opal/mca/base/mca_base_var.c b/opal/mca/base/mca_base_var.c
index 28658fe55b..204f5883ea 100644
--- a/opal/mca/base/mca_base_var.c
+++ b/opal/mca/base/mca_base_var.c
@@ -2297,7 +2297,8 @@ int mca_base_var_dump(int vari, char ***out, mca_base_var_dump_type_t output_typ
 
             color_var_name = var_dump_colors[VAR_DUMP_COLOR_VAR_NAME];
             color_var_value = var_dump_colors[VAR_DUMP_COLOR_VAR_VALUE];
-            color_valid_values = var_dump_colors[VAR_DUMP_COLOR_VALID_VALUES];
+            // color_valid_values = var_dump_colors[VAR_DUMP_COLOR_VALID_VALUES];
+            color_valid_values = "\033[0m";
             color_reset = "\033[0m";
         }
 
diff --git a/opal/mca/base/mca_base_var_enum.c b/opal/mca/base/mca_base_var_enum.c
index 532b828a5d..914049ee6e 100644
--- a/opal/mca/base/mca_base_var_enum.c
+++ b/opal/mca/base/mca_base_var_enum.c
@@ -421,7 +421,7 @@ static int enum_dump(mca_base_var_enum_t *self, char **out)
 
     tmp = NULL;
     for (i = 0; i < self->enum_value_count && self->enum_values[i].string; ++i) {
-        ret = opal_asprintf(out, "%s%s%d:\"%s\"", tmp ? tmp : "", tmp ? ", " : "",
+        ret = opal_asprintf(out, "%s%s\033[36m%d\033[0m:\"%s\"", tmp ? tmp : "", tmp ? ", " : "",
                             self->enum_values[i].value, self->enum_values[i].string);
         if (tmp) {
             free(tmp);

Full image collection: ompi_info_color.zip

[jsquyres]

Isn't the string also a valid value for an MCA enum param? I.e., I thought both the int and the string were valid values. Meaning: shouldn't both be colored?

I don't quite understand the patch you included in your last comment -- why hard-code color_valid_values to "\033[0m" instead of var_dump_colors[VAR_DUMP_COLOR_VALID_VALUES]? And in mca_base_var_enum.c, why hard-code the colors into the string -- shouldn't those be run-time settable, too?

[rhc54]

My impression was that George was just going to do a quick prototype to see how it would look, and then once you decide which you like, THEN he would do a more permanent coding.

[bwbarrett]

bot:aws:retest

[gkatev]

Yes exactly, it is a qucik&dirty change to see which style we prefer, and the diff is there for anyone that also wants to experiment.

Indeed it does sound right to also color the string! Below are examples with int+string colored. The difference of "vva-1" vs "vva-2" is whether the colon between int and string is colored.

vva-1:

vva-2:

[github-actions[bot]]

Hello! The Git Commit Checker CI bot found a few problems with this PR:

35e41044: TMP COMMIT: Color the actual valid values

• check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

[gkatev]

I've addeed proper coloring for the actual valid values, but it's not fully there yet. I initially considered using the mca_base_var_dump_type_t enum also for the functions in mca_base_var_enum.h/c, but there seems to be a circular dependency situation going on with the typedefs and the two header files, not fully sure of the best approach yet.

I'm also considering doing the parsing for the colors at an appropriate time instead of on demand when necessary, as it bloats the code everywhere they are needed. I can't do it at mca_base_var_init() because the color mca param is not yet available. Would it be "legal" if I attempted to get the value of the opal_var_dump_colors MCA right after registering it, or is that too early?

[rhc54]

The param value is available immediately after registration, so feel free to use it.

[gkatev]

This is again ready for review/feedback. I moved the parsing and the color-settings array to opal_params_core. I also renamed the MCA for customizing the color coding to opal_var_dump_color (color, instead of colors).

How it looks now:

Useful commands to see all possible coloring scenarios:

ompi_info --all --level 9 # general, enum
ompi_info --param opal all --level 9 # bool enum
ompi_info --param btl self --level 9 # flag enum

[gkatev]

Thanks for the feedback, I committed the recommended changes. I think I also lean towards "vva-1" -- I applied the patch (it does make the code a little weird with all the printf parameters, but a small price to pay if we like it!). And I also called the raise and applied those changes as well (quotes etc). Theoretically, we could also remove the quotes from the parameter name and value (iff color support is present), but those don't stand out as much; could as well leave them.

[gkatev]

I added help texts for the recommended user-error cases, feel free to suggest alternate forms if you have inspiration regarding them.

Another small thing we might want to adjust is the token separator; initially I set it to : following the spirit of LS_COLORS/GCC_COLORS and similar. But perhaps using comma instead might be more intuitive and more in line with other stuff in OMPI?

[gkatev]

I switched to , separation, added the valid keys to the help text, and reworked/improved error cleanup, please review

[jsquyres]

bot:ompi:retest

[jsquyres]

bot:aws-v2:retest

[jsquyres]

@gkatev I don't know why this PR keeps falling off my radar; thanks for the poke. I rebased this PR to the HEAD of main so that we can clear some of the false CI failures (we made some changes to CI recently, which accidentally caused some false failures).

[jsquyres]

@gkatev Thanks for you patience with this PR!

Could you cherry-pick it to v5.0.x?