Skip to content

Commit

Permalink
Logging: remove override_log_formatter* utilities
Browse files Browse the repository at this point in the history
The `override_log_formatter` and `override_log_formatter_context`
utilities of the `aiida.common.log` module were used in certain CLI
commands to temporarily change the formatter of the logger. This was
done because they used the logging system as the way to output messages
to the terminal, but the default logger prefixed messages with extensive
information such as timestamps and the name of the logger, where only
the message was required.

These utilities are no longer needed since this custom formatting for
the CLI is now done ensured by configuring the `CliFormatter` custom
formatter which takes care of only including the log level as prefix.
  • Loading branch information
sphuber committed Aug 24, 2021
1 parent b0f6331 commit 74d8b6c
Show file tree
Hide file tree
Showing 12 changed files with 198 additions and 354 deletions.
20 changes: 7 additions & 13 deletions aiida/cmdline/commands/cmd_archive.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ def create(
You can modify some of those rules using options of this command.
"""
# pylint: disable=too-many-branches
from aiida.common.log import override_log_formatter_context
from aiida.common.progress_reporter import set_progress_bar_tqdm, set_progress_reporter
from aiida.tools.importexport import export, ExportFileFormat
from aiida.tools.importexport.common.exceptions import ArchiveExportError
Expand Down Expand Up @@ -165,14 +164,13 @@ def create(
elif archive_format == 'null':
export_format = 'null'

if AIIDA_LOGGER.level <= logging.INFO:
if AIIDA_LOGGER.level <= logging.REPORT: # pylint: disable=no-member
set_progress_bar_tqdm(leave=(AIIDA_LOGGER.level == logging.DEBUG))
else:
set_progress_reporter(None)

try:
with override_log_formatter_context('%(message)s'):
export(entities, filename=output_file, file_format=export_format, **kwargs)
export(entities, filename=output_file, file_format=export_format, **kwargs)
except ArchiveExportError as exception:
echo.echo_critical(f'failed to write the archive file. Exception: {exception}')
else:
Expand All @@ -198,7 +196,6 @@ def create(
)
def migrate(input_file, output_file, force, in_place, archive_format, version):
"""Migrate an export archive to a more recent format version."""
from aiida.common.log import override_log_formatter_context
from aiida.common.progress_reporter import set_progress_bar_tqdm, set_progress_reporter
from aiida.tools.importexport import detect_archive_type, EXPORT_VERSION
from aiida.tools.importexport.archive.migrators import get_migrator
Expand All @@ -213,7 +210,7 @@ def migrate(input_file, output_file, force, in_place, archive_format, version):
'no output file specified. Please add --in-place flag if you would like to migrate in place.'
)

if AIIDA_LOGGER.level <= logging.INFO:
if AIIDA_LOGGER.level <= logging.REPORT: # pylint: disable=no-member
set_progress_bar_tqdm(leave=(AIIDA_LOGGER.level == logging.DEBUG))
else:
set_progress_reporter(None)
Expand All @@ -225,8 +222,7 @@ def migrate(input_file, output_file, force, in_place, archive_format, version):
migrator = migrator_cls(input_file)

try:
with override_log_formatter_context('%(message)s'):
migrator.migrate(version, output_file, force=force, out_compression=archive_format)
migrator.migrate(version, output_file, force=force, out_compression=archive_format)
except Exception as error: # pylint: disable=broad-except
if AIIDA_LOGGER.level <= logging.DEBUG:
raise
Expand Down Expand Up @@ -310,10 +306,9 @@ def import_archive(
The archive can be specified by its relative or absolute file path, or its HTTP URL.
"""
# pylint: disable=unused-argument
from aiida.common.log import override_log_formatter_context
from aiida.common.progress_reporter import set_progress_bar_tqdm, set_progress_reporter

if AIIDA_LOGGER.level <= logging.INFO:
if AIIDA_LOGGER.level <= logging.REPORT: # pylint: disable=no-member
set_progress_bar_tqdm(leave=(AIIDA_LOGGER.level == logging.DEBUG))
else:
set_progress_reporter(None)
Expand All @@ -332,9 +327,8 @@ def import_archive(
'comment_mode': comment_mode,
}

with override_log_formatter_context('%(message)s'):
for archive, web_based in all_archives:
_import_archive(archive, web_based, import_kwargs, migration)
for archive, web_based in all_archives:
_import_archive(archive, web_based, import_kwargs, migration)


def _echo_exception(msg: str, exception, warn_only: bool = False):
Expand Down
4 changes: 1 addition & 3 deletions aiida/cmdline/commands/cmd_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ def delete(codes, dry_run, force):
Note that codes are part of the data provenance, and deleting a code will delete all calculations using it.
"""
from aiida.common.log import override_log_formatter_context
from aiida.tools import delete_nodes

node_pks_to_delete = [code.pk for code in codes]
Expand All @@ -200,8 +199,7 @@ def _dry_run_callback(pks):
echo.echo_warning(f'YOU ARE ABOUT TO DELETE {len(pks)} NODES! THIS CANNOT BE UNDONE!')
return not click.confirm('Shall I continue?', abort=True)

with override_log_formatter_context('%(message)s'):
_, was_deleted = delete_nodes(node_pks_to_delete, dry_run=dry_run or _dry_run_callback)
was_deleted = delete_nodes(node_pks_to_delete, dry_run=dry_run or _dry_run_callback)

if was_deleted:
echo.echo_success('Finished deletion.')
Expand Down
4 changes: 1 addition & 3 deletions aiida/cmdline/commands/cmd_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ def group_remove_nodes(group, nodes, clear, force):
@with_dbenv()
def group_delete(group, delete_nodes, dry_run, force, **traversal_rules):
"""Delete a group and (optionally) the nodes it contains."""
from aiida.common.log import override_log_formatter_context
from aiida.tools import delete_group_nodes
from aiida import orm

Expand All @@ -112,8 +111,7 @@ def _dry_run_callback(pks):
echo.echo_warning(f'YOU ARE ABOUT TO DELETE {len(pks)} NODES! THIS CANNOT BE UNDONE!')
return not click.confirm('Do you want to continue?', abort=True)

with override_log_formatter_context('%(message)s'):
_, nodes_deleted = delete_group_nodes([group.pk], dry_run=dry_run or _dry_run_callback, **traversal_rules)
_, nodes_deleted = delete_group_nodes([group.pk], dry_run=dry_run or _dry_run_callback, **traversal_rules)
if not nodes_deleted:
# don't delete the group if the nodes were not deleted
return
Expand Down
4 changes: 1 addition & 3 deletions aiida/cmdline/commands/cmd_node.py
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,6 @@ def node_delete(identifier, dry_run, force, **traversal_rules):
the nodes necessary to keep a consistent graph, according to the rules outlined in the documentation.
You can modify some of those rules using options of this command.
"""
from aiida.common.log import override_log_formatter_context
from aiida.orm.utils.loaders import NodeEntityLoader
from aiida.tools import delete_nodes

Expand All @@ -309,8 +308,7 @@ def _dry_run_callback(pks):
echo.echo_warning(f'YOU ARE ABOUT TO DELETE {len(pks)} NODES! THIS CANNOT BE UNDONE!')
return not click.confirm('Shall I continue?', abort=True)

with override_log_formatter_context('%(message)s'):
_, was_deleted = delete_nodes(pks, dry_run=dry_run or _dry_run_callback, **traversal_rules)
_, was_deleted = delete_nodes(pks, dry_run=dry_run or _dry_run_callback, **traversal_rules)

if was_deleted:
echo.echo_success('Finished deletion.')
Expand Down
8 changes: 7 additions & 1 deletion aiida/cmdline/utils/echo.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,20 @@ def echo_report(message: str, bold: bool = False, nl: bool = True, err: bool = F
def echo_success(message: str, bold: bool = False, nl: bool = True, err: bool = False, prefix: bool = True) -> None:
"""Log a success message to the cmdline logger.
.. note:: The message will be logged at the ``REPORT`` level and always with the ``Success:`` prefix.
:param message: the message to log.
:param bold: whether to format the message in bold.
:param nl: whether to add a newline at the end of the message.
:param err: whether to log to stderr.
:param prefix: whether the message should be prefixed with a colored version of the log level.
"""
message = click.style(message, bold=bold)
CMDLINE_LOGGER.report(message, extra=dict(nl=nl, err=err, prefix=prefix))

if prefix:
message = click.style('Success: ', bold=True, fg=COLORS['success']) + message

CMDLINE_LOGGER.report(message, extra=dict(nl=nl, err=err, prefix=False))


def echo_warning(message: str, bold: bool = False, nl: bool = True, err: bool = False, prefix: bool = True) -> None:
Expand Down
1 change: 0 additions & 1 deletion aiida/common/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@
'ValidationError',
'create_callback',
'get_progress_reporter',
'override_log_formatter',
'override_log_level',
'set_progress_bar_tqdm',
'set_progress_reporter',
Expand Down
43 changes: 3 additions & 40 deletions aiida/common/log.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,11 @@
###########################################################################
"""Module for all logging methods/classes that don't need the ORM."""
import collections
import copy
import contextlib
import logging
import types
from contextlib import contextmanager
from wrapt import decorator

__all__ = ('AIIDA_LOGGER', 'override_log_level', 'override_log_formatter')
__all__ = ('AIIDA_LOGGER', 'override_log_level')

# Custom logging level, intended specifically for informative log messages reported during WorkChains.
# We want the level between INFO(20) and WARNING(30) such that it will be logged for the default loglevel, however
Expand Down Expand Up @@ -214,46 +212,11 @@ def configure_logging(with_orm=False, daemon=False, daemon_log_file=None, cli=Fa
dictConfig(config)


@contextmanager
@contextlib.contextmanager
def override_log_level(level=logging.CRITICAL):
"""Temporarily adjust the log-level of logger."""
logging.disable(level=level)
try:
yield
finally:
logging.disable(level=logging.NOTSET)


@contextmanager
def override_log_formatter_context(fmt: str):
"""Temporarily use a different formatter for all handlers.
NOTE: One can _only_ set `fmt` (not `datefmt` or `style`).
"""
temp_formatter = logging.Formatter(fmt=fmt)
cached_formatters = {}

for handler in AIIDA_LOGGER.handlers:
# Need a copy here so we keep the original one should the handler's formatter be changed during the yield
cached_formatters[handler] = copy.copy(handler.formatter)
handler.setFormatter(temp_formatter)

yield

for handler, formatter in cached_formatters.items():
handler.setFormatter(formatter)


def override_log_formatter(fmt: str):
"""Temporarily use a different formatter for all handlers.
NOTE: One can _only_ set `fmt` (not `datefmt` or `style`).
Be aware! This may fail if the number of handlers is changed within the decorated function/method.
"""

@decorator
def wrapper(wrapped, instance, args, kwargs): # pylint: disable=unused-argument
with override_log_formatter_context(fmt=fmt):
return wrapped(*args, **kwargs)

return wrapper
2 changes: 1 addition & 1 deletion setup.json
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@
"aiida-export-migration-tests==0.9.0",
"pg8000~=1.13",
"pgtest~=1.3,>=1.3.1",
"pytest~=6.0",
"pytest~=6.2",
"pytest-asyncio~=0.12",
"pytest-timeout~=1.3",
"pytest-cov~=2.7,<2.11",
Expand Down
8 changes: 7 additions & 1 deletion tests/cmdline/commands/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
# For further information please visit http://www.aiida.net #
###########################################################################
"""Pytest fixtures for command line interface tests."""
import pathlib

import click
import pytest

Expand All @@ -33,13 +35,17 @@ def _run_cli_command(command: click.Command, options: list = None, raises: bool
"""
import traceback

# Convert any ``pathlib.Path`` objects in the ``options`` to their absolute filepath string representation.
# This is necessary because the ``invoke`` command does not support these path objects.
options = [str(option) if isinstance(option, pathlib.Path) else option for option in options or []]

# We need to apply the ``VERBOSITY`` option. When invoked through the command line, this is done by the logic
# of the ``VerdiCommandGroup``, but when testing commands, the command is retrieved directly from the module
# which circumvents this machinery.
command = VerdiCommandGroup.add_verbosity_option(command)

runner = click.testing.CliRunner()
result = runner.invoke(command, options or [])
result = runner.invoke(command, options)

if raises:
assert result.exception is not None, result.output
Expand Down
Loading

0 comments on commit 74d8b6c

Please sign in to comment.