mca: fix deprecated variable source warning

[francdoc]

Linked to #12437.

This PR fixes deprecated MCA synonym warning source reporting.

Previously, if mca_base_component_show_load_errors was set in an MCA params file and the deprecated mca_component_show_load_errors name was set from the environment, the warning could incorrectly blame the params file.

Validation:

• rebuilt build-12437
• git diff --check
• manually checked the env, file, COMMAND_LINE, and non-deprecated cases

[rhc54]

Just FWIW: I ported these changes over to PMIx and found that the output isn't quite correct - it winds up with the deprecated and "new" variables being reversed.

--------------------------------------------------------------------------
A deprecated MCA variable value was specified in the environment or
on the command line.  Deprecated MCA variables should be avoided;
they may disappear in future releases.

Deprecated variable: mca_base_component_show_load_errors
New variable:        mca_component_show_load_errors
--------------------------------------------------------------------------

when in fact it is the other way around. I could correct it by simply changing the order in the show_help calls, but I'm not sure if that would always be correct.

It also wasn't clear if these changes impacted that output, or if this output was always incorrect. So I tried it with the current code (i.e., without changes) and got this:

--------------------------------------------------------------------------
A deprecated MCA variable value was specified in an MCA variable
file.  Deprecated MCA variables should be avoided; they may disappear
in future releases.

Deprecated variable: mca_component_show_load_errors
Source file:         /Users/rhc/.pmix/mca-params.conf
New variable:        mca_component_show_load_errors

The file is correct, but the deprecated variable is actually in the environment and not the file - so the bug report here is correct. However, it also shows that the deprecated and new variable names are being reported as the same.

So I think you need the rest of the patch to make this correct:

diff --git a/src/mca/base/pmix_mca_base_var.c b/src/mca/base/pmix_mca_base_var.c
index b1c2d00e..bb5da3c7 100644
--- a/src/mca/base/pmix_mca_base_var.c
+++ b/src/mca/base/pmix_mca_base_var.c
@@ -1435,13 +1435,14 @@ static int var_set_from_env(pmix_mca_base_var_t *var, pmix_mca_base_var_t *origi
     if (NULL != source_env) {
         if (0 == strncasecmp(source_env, "file:", 5)) {
             original->mbv_source_file = append_filename_to_list(source_env + 5);
-            if (0 == strcmp(var->mbv_source_file, pmix_mca_base_var_override_file)) {
+            if (0 == strcmp(original->mbv_source_file, pmix_mca_base_var_override_file)) {
                 original->mbv_source = PMIX_MCA_BASE_VAR_SOURCE_OVERRIDE;
             } else {
                 original->mbv_source = PMIX_MCA_BASE_VAR_SOURCE_FILE;
             }
-        } else if (0 == strcasecmp(source_env, "command")) {
-            var->mbv_source = PMIX_MCA_BASE_VAR_SOURCE_COMMAND_LINE;
+        } else if (0 == strcasecmp(source_env, "command") ||
+                   0 == strcasecmp(source_env, "command_line")) {
+            original->mbv_source = PMIX_MCA_BASE_VAR_SOURCE_COMMAND_LINE;
         }
     }

@@ -1452,19 +1453,20 @@ static int var_set_from_env(pmix_mca_base_var_t *var, pmix_mca_base_var_t *origi
             new_variable = var->mbv_full_name;
         }

-        switch (var->mbv_source) {
+        switch (original->mbv_source) {
         case PMIX_MCA_BASE_VAR_SOURCE_ENV:
-            pmix_show_help("help-pmix-mca-var.txt", "deprecated-mca-env", true, var_full_name,
-                           new_variable);
+            pmix_show_help("help-pmix-mca-var.txt", "deprecated-mca-env", true,
+                           new_variable, original->mbv_full_name);
             break;
         case PMIX_MCA_BASE_VAR_SOURCE_COMMAND_LINE:
-            pmix_show_help("help-pmix-mca-var.txt", "deprecated-mca-cli", true, var_full_name,
-                           new_variable);
+            pmix_show_help("help-pmix-mca-var.txt", "deprecated-mca-cli", true,
+                           new_variable, original->mbv_full_name);
             break;
         case PMIX_MCA_BASE_VAR_SOURCE_FILE:
         case PMIX_MCA_BASE_VAR_SOURCE_OVERRIDE:
-            pmix_show_help("help-pmix-mca-var.txt", "deprecated-mca-file", true, var_full_name,
-                           pmix_mca_base_var_source_file(var), new_variable);
+            pmix_show_help("help-pmix-mca-var.txt", "deprecated-mca-file", true,
+            new_variable, pmix_mca_base_var_source_file(original),
+            original->mbv_full_name);
             break;

         case PMIX_MCA_BASE_VAR_SOURCE_DEFAULT:

Note that our line numbers are different as I don't have all the MPI_T stuff. You'd also need to check that this doesn't create an error for some other use-case.

[francdoc]

Thanks for the review Ralph. I’ll re-check this as soon as I can. My testing was a bit too manual, so I’ll review the relevant paths again and update the PR if I understood your point correctly.

[francdoc]

Sorry for the delay. This PR is still in progress, I’m finishing a postgraduate presentation next week and will resume the review afterward.

[jsquyres]

@francdoc Would you mind using rebase instead of merging from main? We tend to prefer that in this repo.

Thank you!

[francdoc]

@francdoc Would you mind using rebase instead of merging from main? We tend to prefer that in this repo.

Thank you!

Hi Jeff (@jsquyres), absolutely. Sorry for the confusion. I will fix this right away.

[francdoc]

I’ll redo it with a rebase and force-push the cleaned branch.

[francdoc]

Hi Ralph (@rhc54) and Jeff (@jsquyres). I am going to handle this in two parts so the PMIx changes go through the right upstream project.

Plan:

1. Reconfirm the OPAL behavior in this PR.
2. Reproduce the PMIx warning issue separately.
3. Submit the PMIx-side fix upstream to openpmix/openpmix.
4. Link the PMIx PR back here once it exists.

Since 3rd-party/openpmix is a submodule in Open MPI, this should keep the OPAL fix scoped here while still addressing the PMIx issue Ralph pointed out.

[francdoc]

Hi. I opened the PMIx-side fix here: openpmix/openpmix#3933

That PR handles the deprecated MCA variable warning issue in standalone OpenPMIx. I am leaving this Open MPI PR scoped to the OPAL-side change unless you prefer a different approach.

[francdoc]

Hi Jeff (@jsquyres). Based on what happened in openpmix/openpmix#3933, I’ll update this PR to match the OPAL side:

1. Add an OPAL MCA variable to suppress deprecated MCA warnings by default, like Ralph (@rhc54) did in PMIx.
2. Only show deprecated MCA warnings when the user explicitly asks for them.

I'll check this conversation again before the new push just in case I need to do some change or adjustment.

[rhc54]

Just to be clear, I really don't like this suppress by default behavior. IMO, it's the opposite of what we should do. However, if we do that, we'd have to modify the mpi4py CI to set the MCA param to suppress the warnings before running the test.

I guess we could do that - if folks over here agree that this is the behavior they want. I think I'll update the PMIx side to work that way as I, at least, certainly think that would make more sense.

[francdoc]

Ralph (@rhc54), ok. I won’t make suppression the default here.

My understanding then is that for the OPAL side I can add the same control and keep deprecated warnings shown by default unless the new MCA param is set.

I think I’ll wait until tomorrow to see if there are any new comments and then I can try coding that version in this PR.

[rhc54]

See openpmix/openpmix#3941 (apologies for the .gitignore update in the middle).

[francdoc]

See openpmix/openpmix#3941 (apologies for the .gitignore update in the middle).

Great, thank you. I’ll use this as the reference for the fix and try to work on it in a couple of hours when I can take a proper look.

[francdoc]

Hi Jeff (@jsquyres) and Ralph (@rhc54), I pushed an update to this PR based on the PMIx follow-up work.

Does this approach look reasonable?

Thank you.

[rhc54]

LGTM! Thanks!

[francdoc]

LGTM! Thanks!

Thanks Ralph! I appreciate the review.