Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle ASTE and preCICE logging #136

Merged
merged 11 commits into from
Nov 18, 2022
Merged

Handle ASTE and preCICE logging #136

merged 11 commits into from
Nov 18, 2022

Conversation

fsimonis
Copy link
Member

@fsimonis fsimonis commented Aug 23, 2022

This PR changes the ASTE logger to:

  • Tags log entries from ASTE using the constant boolean attribute ASTE.
  • Output only ASTE log entries by filtering ... and %ASTE% in the sink.
  • Adds additional attributes for Participant and Rank. This allows to distinguish log entries when running ASTE in parallel.

After this, preCICE will still output ASTE logs by default. A similar change will soon be merged in preCICE itself.
Until then, we can configure a log filter in preCICE ... and not %ASTE% to remove ASTE log entries from the preCICE log.

edit: ASTE will now handle logs from both preCICE and ASTE.

preCICE configs should disable all sinks by defining <log enabled="false"></log>

Related to #73

@davidscn
Copy link
Member

The preciceMap file was renamed. You can easily resolve the conflict here by renaming your preciceMap file locally and rebasing on the current develop. Please let me know if you want me to do that.

@fsimonis
Copy link
Member Author

@davidscn I happily accept your offer 😉

Copy link
Member

@davidscn davidscn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the changes now with the corresponding precice branch. First of all, the PRs resolve the logger ambiguity, thanks!
However, we have in ASTE in addition to ASTE_INFO and ASTE_DEBUG ASTE_WARN and ASTE_ERROR, which were colorized and prepended by a corresponding keyword (e.g. ERROR). The colorization and the keyword are gone. See below for some other comments.

src/precice-aste-run.cpp Outdated Show resolved Hide resolved
src/logger.cpp Outdated Show resolved Hide resolved
src/logger.cpp Outdated Show resolved Hide resolved
src/logger.cpp Outdated

void addLogSink(bool verbose)
{
std::string filter = "( %Severity% >= ";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to filter according to the primary rank by default?

Copy link
Member Author

@fsimonis fsimonis Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially in ASTE, it can be useful to see how the various ranks are processing the mesh data. I found this very valuable while improving this part of the application.

@fsimonis
Copy link
Member Author

I got more info from the boost team and will change the implementation slightly.

@fsimonis fsimonis changed the title Limit local logging to application log Handle ASTE and preCICE logging Aug 24, 2022
@fsimonis
Copy link
Member Author

I had a new go at this as I just learned formatters can be conditional (boostorg/log#193).

ASTE now handles all the logging. preCICE logs (aka sinks) should be turned off.

@davidscn
Copy link
Member

With this approach, the preCICE logger won't be configurable any more, right?

@fsimonis
Copy link
Member Author

fsimonis commented Aug 26, 2022

With this approach, the preCICE logger won't be configurable any more, right?

Yes and no.

ASTE enforces a format and filter for everything. You can still use --verbose to enable debug in both ASTE and preCICE (if available).

In addition, you can still use the preCICE config for additionally logging detailed information. Say debug of a specific module. You could then set the sink to a file and have only your required information in a format that is most useful for you.
Example: filter="( %Participant% == A ) & ( %Severity% == debug ) & ( %Module% endwith Mapping )"

@davidscn
Copy link
Member

davidscn commented Aug 31, 2022

In addition, you can still use the preCICE config for additionally logging detailed information. Say debug of a specific module. You could then set the sink to a file and have only your required information in a format that is most useful for you.
Example: filter="( %Participant% == A ) & ( %Severity% == debug ) & ( %Module% endwith Mapping )"

I see, this will potentially lead to duplication in the logging, but probably be inevitably when handling the logging this way.

Could you please rebase? The mapping tester example using this logging configuration fails for me and the CI back then didn't fail (but will do now).

<log enabled="true">
<sink type="stream" output="stdout" filter="%Severity% > debug" format='%TimeStamp(format="%H:%M:%S.%f")%|%Participant%|%Rank%|%Module%|l%Line%|%Function%|%Severity%%Message%' enabled="true" />
<sink type="file" output="debug.log" filter="" format='%TimeStamp(format="%H:%M:%S.%f")%|%Participant%|%Rank%|%Module%|l%Line%|%Function%|%Severity%%Message%' enabled="true"/>
</log>

EDIT: I think the fail here is related to the preCICE logging branch. Which of the configurations here is invalid?

@fsimonis fsimonis force-pushed the extend-local-logging branch from bed3464 to 78df340 Compare September 7, 2022 10:07
@fsimonis
Copy link
Member Author

fsimonis commented Sep 7, 2022

According to the documentation of boost log, the expression templates for filters and logs perform better than the parsed filters and formatters. So, ASTE using a fixed format should reduce logging overhead too.

@fsimonis fsimonis requested a review from davidscn September 7, 2022 10:19
@davidscn
Copy link
Member

Remaining points to discuss:

  1. The config logger settings are now mostly useless, unless someone wants additional loggings. We should remove the logger settings from all config files.

  2. Do we want to go with this logger format?

---[preCICE:A:0] (impl::SolverInterfaceImpl in configure) I am participant "A"

Considering that the verbose flag refers to a debug log in preCICE, we could add the function only in a verbose mode. I don't see much advantage for having the function name printed by default.

@fsimonis
Copy link
Member Author

fsimonis commented Sep 19, 2022

Today, we talked about colorizing log levels as well as participant names differently.

We could add some more options to handle this as well as #136 (comment).

We need verbosity, colorization, and a filter for all vs primary only/no secondaries.

Suggestion: use non-verbose, colorized output from all ranks by default

Options to implement:

  • --verbose (which we already have) enables the output of debug information and enabled the %Module% in %Function% part of the log
  • --no-color disables the output of colors
  • --only-primary prints only logs from rank 0

Problem now is which colours to use for the Participants? Are they allowed to clash with the severity levels? Would a bold font be enough?

There are also no that many colors available if we stick to ASNI color codes.

       31        set red foreground
       32        set green foreground
       33        set brown foreground
       34        set blue foreground
       35        set magenta foreground
       36        set cyan foreground
       37        set white foreground

There are more options though https://chrisyeh96.github.io/2020/03/28/terminal-colors.html

@davidscn
Copy link
Member

Suggestion: use non-verbose, colorized output from all ranks by default

To avoid complications, I would propose to use the non-verbose, non-colorized output by default. Maybe even use only the primary rank logs by default (no strong opinion here). However, this is somewhat the default behavior when working with preCICE/adapters/tutorials. We could then still add the CL arguments.

Problem now is which colours to use for the Participants? Are they allowed to clash with the severity levels? Would a bold font be enough?

I guess one would then execute one solver using the colorization and the other without colorization? Or different colors for different participants? I think aste supports only two participants right now. I would just decide for a color, maybe green or blue? I don't think that it will lead to conflicts with other colorization, as the messages are always white (just the severity is colorized).

@fsimonis fsimonis force-pushed the extend-local-logging branch from b5865d8 to 438c4eb Compare September 19, 2022 12:49
@fsimonis
Copy link
Member Author

Current conclusion is to handle colorization in a future PR.

@davidscn
Copy link
Member

Last point would be to disable (remove) all logger tags from the preCICE config, right?

@fsimonis
Copy link
Member Author

Sure, will do

@davidscn
Copy link
Member

Considering that the changes here are related to the logging in preCICE, I'm not sure weather we want to add them to the distribution, as the distribution is referring to preCICE v 2.5.0

@fsimonis
Copy link
Member Author

That's actually a good point. This PR will not work properly until precice/precice#1420 is merged.

@davidscn davidscn merged commit 97e2a23 into develop Nov 18, 2022
@davidscn davidscn deleted the extend-local-logging branch November 18, 2022 13:47
davidscn added a commit to davidscn/aste that referenced this pull request Dec 9, 2022
* Limit local logging to application log
* Indentation using clang-format
* Handle log entries from ASTE and preCICE
* Hide preCICE log location in non-verbose runs
* Add severity and rank filter control
* Fix filter expression
* Make mapping tester tests verbose
* Indentation using clang-format
* Remove log sinks from ASTE configuration files
* Update flag documentation

Co-authored-by: David Schneider <david.schneider@ipvs.uni-stuttgart.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants