Suppress permission warnings

[mikekryjak]

There are warnings when permissions are too loose (#467). They are very numerous and hard to fix because they don't identify the source component. We need to add the capability for components to know their name to have a realistic chance of resolving this (#429).

The warnings are so large that the logs quickly grow to multi-MB in size, making it impractical to run the code or see other checks. Because of this, I think we should disable the warnings until we have a chance of fixing them.

This PR does this by requiring a DCHECK=4 (which doesn't exist).

Permissions being too open will not be a problem until the topological sort algorithm is finished (#428), at which point they will inform the component order. We can resolve this when we work on the algorithm.

[cmacmackin]

This will be enough to suppress the warnings messages, but if we're turning those off there are some other bits it would make sense to turn off too. See oparry-ukaea@5e66efb.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 29.24%. Comparing base (1911a59) to head (900b435).
⚠️ Report is 400 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
- Coverage   29.52%   29.24%   -0.29%     
==========================================
  Files          94       94              
  Lines        8768     8755      -13     
  Branches     1228     1225       -3     
==========================================
- Hits         2589     2560      -29     
- Misses       5914     5934      +20     
+ Partials      265      261       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dschwoerer]

This PR does this by requiring a DCHECK=4 (which doesn't exist).

I always run with CHECK=4. If you think this should currently be disabled, use something like 999 to make the intent clearer, or even better remove it. You can then later revert it, if this becomes useful.

[cmacmackin]

I always run with CHECK=4. If you think this should currently be disabled, use something like 999 to make the intent clearer, or even better remove it. You can then later revert it, if this becomes useful.

I don't think -DCHECK=4 is meant to do anything. The BOUT++ docs only mention check levels up to 3 and there are only assertion macros up to that level too.

[dschwoerer]

It is clearly supported and used in BOUT++:
https://github.com/boutproject/BOUT-dev/blob/master/CMakeLists.txt#L535
https://github.com/boutproject/BOUT-dev/blob/master/include/bout/region.hxx#L283

Please do not abuse it for commenting out code.

Maybe fix the docs then?

[dschwoerer]

They are still printed with debug level 4.

You could also print them to output_debug instead of output_warn - if you want the output of by default.

Anyway, this should not be merged as it decreases test coverage 😇

[bendudson]

An alternative is to only print once at low check levels, and print every time at high check level:

#if CHECKLEVEL >= 1
  static bool print_warnings {true};                                                                                     
  if (print_warnings) {
    for (auto& [varname, region] : guarded.unreadItems()) {
      output_warn.write("Did not read from state variable {} in region(s) {}\n", varname,
                        Permissions::regionNames(region));
    }
    for (auto& [varname, region] : guarded.unwrittenItems()) {
      output_warn.write("Did not write to state variable {} in region(s) {}\n", varname,
                        Permissions::regionNames(region));
    }
  }
#if CHECKLEVEL < 4
  print_warnings = false; // Disable printing next time
#endif
#endif

[mikekryjak]

I don't think the warnings should be present for the default check level, because we're not achieving anything by spamming the console of users - most of whom probably haven't even heard of check levels in the first place.

I also don't think the warnings should be present at other check levels because:

• It makes those check levels unusable (if printing all the time) or annoying (if printing once)
• The underlying issue has no impact until Perform topological sort to order components #428 is merged.
• We will get around to fixing it once @cmacmackin returns in the new FY.

@dschwoerer sorry, we believed the docs and didn't realise DCHECK=4 exists.

As for the codecov thing, what's the best practice here? Do we need to add unit tests which specifically target what we changed in this PR, or can we just add more test coverage somewhere else? @ZedThree

[bendudson]

I think this is ok for now, to make master usable. Setting CHECK to 999 allows testing of this feature before merging.