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

Remove MPI dependence in Logger #373

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

Conversation

cniethammer
Copy link
Contributor

Description

This PR removes all dependencies to MPI inside the Logger class. The functionality was replaced by a general message prefix and setting the log levels for the different MPI processes.
Note: By default, MPI processes with rank zero will use log level Info and all other MPI processes log level Error. This should help in identifying errors more quickly from logs. As a side effect, the --verbose option will now enable log level All for all MPI processes, which should be helpful for debugging.

Related Pull Requests

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

  • running examples
  • running unit tests

By default set output level for MPI process with rank zero to Info, and for
all other MPI processes to Error.

Signed-off-by: Christoph Niethammer <[email protected]>
Added message prefix to Logger. The prefix can be set via the new
Logger::set_msg_prefix() method. This allows to get rid entirely of
any MPI dependence inside the Logger class.

Signed-off-by: Christoph Niethammer <[email protected]>
@cniethammer cniethammer added enhancement New feature or request clean-up related to the clean-up of the code and tech dept labels Jan 16, 2025
@HomesGH
Copy link
Contributor

HomesGH commented Jan 30, 2025

To fix issue #374, we should perform a case-insensitive comparison here. This could be done by changing these lines:

logLevel set_log_level(std::string l) {
for (const auto& [lvl, name] : logLevelNames) {
if (name == l) {
_log_level = lvl;
return _log_level;
}
}

to:

	logLevel set_log_level(std::string l) {
	// Compare case-insensitive
	std::string l_lower = l;
	std::transform(l_lower.begin(), l_lower.end(), l_lower.begin(), ::tolower);
	for (const auto& [lvl, name] : logLevelNames) {
		std::string name_lower = name;
		std::transform(name_lower.begin(), name_lower.end(), name_lower.begin(), ::tolower);
		if (name_lower == l_lower) { 
			_log_level = lvl; 
			return _log_level; 
		} 
	}

@amartyads
Copy link
Contributor

If we do toUpper instead, we can avoid multiple conversions:

	logLevel set_log_level(std::string l) {
	// Compare case-insensitive
	std::string l_upper = l;
	std::transform(l_upper.begin(), l_upper.end(), l_upper.begin(), [](unsigned char c){ return std::toupper(c);});
	for (const auto& [lvl, name] : logLevelNames) {
		if (name == l_upper) { 
			_log_level = lvl; 
			return _log_level; 
		} 
	}

The lambda function is to avoid undefined behaviour (https://en.cppreference.com/w/cpp/string/byte/toupper)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up related to the clean-up of the code and tech dept enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants