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

Logger update #359

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

cniethammer
Copy link
Contributor

@cniethammer cniethammer commented Dec 11, 2024

Added the possibility to change the output stream of the logger. The logger is now default initialized with output to std::cout right after MPI_init. The output is changed to a log file later if requested via command line arguments. This resolves the segfault brought up during the review of PR #355 and takes into account the comments on the initial fix in PR #358.

As the explicit constructor for logfiles is not needed any more, now, got rid of this in preparation for removing dependencies to MPI.
Also, refactored the time output code to use std::chrono where possible

Update:
Allow copying of loggers.
Fixed setting the log level by string: now case-matches instead of just looking for the leading character.

@cniethammer cniethammer added bug Something isn't working clean-up related to the clean-up of the code and tech dept labels Dec 11, 2024
Copy link
Member

@FG-TUM FG-TUM left a comment

Choose a reason for hiding this comment

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

I really like the simplification of the constructor and usage of std::chrono.

src/utils/Logger.cpp Show resolved Hide resolved
src/MarDyn.cpp Outdated Show resolved Hide resolved
src/utils/Logger.h Outdated Show resolved Hide resolved
Copy link
Contributor

@HomesGH HomesGH left a comment

Choose a reason for hiding this comment

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

Overall good PR. I do not fully understand:

got rid of this in preparation for removing dependencies to MPI.

Are those dependencies removed in a future PR? What is the roadmap for this and why not in this one?

src/MarDyn.cpp Show resolved Hide resolved
@cniethammer
Copy link
Contributor Author

I would like to remove the MPI dependencies, but I think for the moment I will put my effort into other parts of the code/PRs first ...

Consider this PR as ready after applying the suggested updates, now. :)

Copy link
Member

@FG-TUM FG-TUM left a comment

Choose a reason for hiding this comment

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

Just stumbled upon one more thing since you are refactoring the logger anyway 😇

src/utils/Logger.h Outdated Show resolved Hide resolved
@cniethammer
Copy link
Contributor Author

CI seems to have some issues - seem to be some issues with AutoPas that also break tests for other PRs ...?

@FG-TUM
Copy link
Member

FG-TUM commented Dec 13, 2024

CI seems to have some issues - seem to be some issues with AutoPas that also break tests for other PRs ...?

Seems like the update to the GitHub Ubuntu images messed up something with the dependencies. I'll fix it later today.

Working again in #360

cniethammer and others added 12 commits December 18, 2024 16:40
Implement set_log_stream to change the output stream of the logger after
logger construction.

Signed-off-by: Christoph Niethammer <[email protected]>
Construct the global logger just after MPI_init with the default output
stream (stdout).
After command line parsing, switch the output stream of the global logger
to a logfile if requested via the '--logfile' command line option.

Signed-off-by: Christoph Niethammer <[email protected]>
As now the logger output stream can be set after default construction,
remove the constructor for logfiles as it is not used any more. This
also helps removing the MPI code from the logger.

Signed-off-by: Christoph Niethammer <[email protected]>
The Logger constructor now takes a shared pointer to a std::ostream.
In this way we do not need the error prone handling of manual stream
descruction any more.
Instead the default shared pointer argument for std::cout takes now
the no-op deleter.

Signed-off-by: Christoph Niethammer <[email protected]>
Signed-off-by: Christoph Niethammer <[email protected]>
Signed-off-by: Christoph Niethammer <[email protected]>
A logger can now be copied, as it does not manage the stream it logs to
itself but keeps an shared pointer, now.

Signed-off-by: Christoph Niethammer <[email protected]>
Signed-off-by: Christoph Niethammer <[email protected]>
Just comparing a single character is not a good idea:
'do not log anything please' would enable debug output ...

Skipping uppercase conversion as we always emit an error if the provided
level name cannot be identified.

Signed-off-by: Christoph Niethammer <[email protected]>
@cniethammer
Copy link
Contributor Author

Rebased and force pushed after CI update to rerun tests

@cniethammer
Copy link
Contributor Author

@FG-TUM: CI now looks good - not sure which change requests are still open ... may have got lost due to the manual push updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working clean-up related to the clean-up of the code and tech dept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants