Framework: Prohibit MPI_COMM_WORLD usages #12111

sebrowne · 2023-08-07T20:18:27Z

Disallow MPI_COMM_WORLD in new/changed code unless a specific comment identifier is added.

Motivation

Helps clean up usages of MPI_COMM_WORLD.

…4_175808 Automatically Merged using Trilinos Master Merge AutoTester PR Title: Trilinos Master Merge PR Generator: Auto PR created to promote from master_merge_20230804_175808 branch to master PR Author: trilinos-autotester

github-actions · 2023-08-07T20:18:41Z

You seem to have created a PR on master. This is not allowed behavior, so we've blocked your PR. Please switch your PR to target the develop branch and remove the AT: WIP label.

sebrowne · 2023-08-07T21:20:35Z

@ccober6 @jwillenbring I think this PR does what we were discussing with respect to MPI_COMM_WORLD

.github/workflows/probited-strings.yml

stmcgovern · 2023-08-10T16:20:16Z

@ccober6 @jwillenbring I think this PR does what we were discussing with respect to MPI_COMM_WORLD

Hi @sebrowne , in discussion with @ccober6 and @jwillenbring, I suggest that the workflow here NexGenAnalytics#73 might be useful.

It looks line by line through modifications.
It excludes test and example directories.
It does not rely on a 3rd party action.

@JacobDomagala has said that he could put up a PR to trilinos/develop, or if you prefer Sam, to incorporate the workflow in your PR here.

sebrowne · 2023-08-10T17:00:53Z

I'm perfectly happy with the other approach. @ccober6 voiced concern over per-file vs. per-line. If Trilinos leadership is okay with this action premise, I'm on-board with adding this to the main repository.

sebrowne · 2023-08-24T19:42:48Z

That does assume that all work is targeting develop, which is probably the most frequent case. Do we still want to add a ctest test that calls that script then? I can't shake the feeling that that is not right. Do people normally add tests for clang-format, include-what-you-use, etc? Every software project I've been a part of does that through hook-based approaches or separate CI checks from the main tests.

bartlettroscoe · 2023-08-24T19:58:41Z

I can't shake the feeling that that is not right. Do people normally add tests for clang-format, include-what-you-use, etc?

Typically, it is a git commit hook to run clang-format checks and the such.

Every software project I've been a part of does that through hook-based approaches or separate CI checks from the main tests.

Right, but Trilinos developers have objected to mandatory git commit hooks.

In my view, running these checks along with other tests with CTest is a reasonable compromise if developers are not willing to use git commit hooks (which need to be installed and maintained automatically if they are going to be used consistently).

bartlettroscoe · 2023-08-24T20:04:40Z

The python script can be used locally without specifying commits. By default it will compare your last commit with origin/develop. Uncommited changes should also be detected.

@stmcgovern, as long as you could specify a different reference version (I call the main Trilinos GitHub repo github instead of origin because I don't lots of multi-repo development workflows), this would work fine. So add and argument to the Python script (e.g. --reference-version=<version-spec>) and a CMake cache var that sets that for the CTest test (e.g. Trilinos_GIT_UPSTREAM_REFERENCE_SPEC) would do the trick and be about perfect I guess (because it is exactly the same check that would be done by the GHA job).

sebrowne · 2023-08-24T21:15:56Z

In my view, running these checks along with other tests with CTest is a reasonable compromise if developers are not willing to use git commit hooks (which need to be installed and maintained automatically if they are going to be used consistently).

Isn't the compromise that they have to wait on the PR check to run? Meaning, if they don't want to implement user-side hooks, they do not get as immediate of feedback? (What I'm implying is that it's fine to leave the hooks as optional)

bartlettroscoe · 2023-08-24T21:46:25Z

In my view, running these checks along with other tests with CTest is a reasonable compromise if developers are not willing to use git commit hooks (which need to be installed and maintained automatically if they are going to be used consistently).

Isn't the compromise that they have to wait on the PR check to run? Meaning, if they don't want to implement user-side hooks, they do not get as immediate of feedback? Meaning they are welcome to not use the hooks (don't make them mandatory), but that means feedback is delayed.

As a general rule of thumb, it should be easy for developers to reproduce locally any automated check enforced on their PR. (They can do that Trilinos PR builds using GenConfig, for example, using some simple driver scripts.) And that is in the best interest of Trilinos too. We don't want Trilinos developers pushing small tweaks to GitHub just to run this check again (and tying up huge computational resources running all of the PR builds again).

If one expects developers to use Git hooks, then the project should provide reasonable infrastructure and guidance on how to install those checks. (For example, Trilinos should provide suggested git commit hooks that developers can just install and update.) Personally, I think running more expensive checks as part of the CTest test suite makes more sense.

Technically speaking, the Trilinos test suite could also be run as a commit hook but that would really hut productivity.

The one type of check that makes the most sense to check at commit time are checks for the commit log message itself like:

$ cat .git/hooks/commit-msg
which codespell >> /dev/null
which_codespell_rtn=$?

if [[ "${which_codespell_rtn}" == "0" ]] ; then
  codespell $1
fi

I have said my piece. Let's see what the feedback from Developers is like. (It may be a while before someone inserts MPI_COMM_WORLD in their library code so this might not be a big deal.)

bartlettroscoe · 2023-08-24T21:51:04Z

@stmcgovern and @sebrowne, one should note that the tool check-mpi-comm-world-usage.py has about 95% of the code needed to check for the occurrence of any undesirable identifier in the code base. In the future, it would be easy to refactor this tool to read from a list of undesirable identifiers and flag for them (with the same ignore text CHECK: ALLOW <some-word>).

trilinos-autotester · 2023-08-24T23:49:24Z