reframe-hpc / reframe

A powerful Python framework for writing and running portable regression tests and benchmarks for HPC systems.
https://reframe-hpc.readthedocs.org
BSD 3-Clause "New" or "Revised" License
214 stars 101 forks source link

Can't set logging.handlers.file.timestamp to true #3128

Closed brandonbiggs closed 1 month ago

brandonbiggs commented 6 months ago

I'm running reframe 4.2.1 so this may have been fixed, but I went through the release notes and didn't see anything about it..

In the logging configuration documentation for file handlers it indicates I can set timestamp = True but when I tried that, I received the following error.

Example config:

    'logging': [
        {
            'level': 'debug',
            'handlers': [
                {
                    'type': 'file',
                    'name': 'test_results/reframe-debug.log',
                    'level': 'debug',
                    'format': '[%(asctime)s] %(levelname)s: %(check_info)s: %(message)s', 
                    'append': True,
                    'timestamp': True
                }
            ]
       }
    ]

Error:

Traceback (most recent call last):
  File "/path/reframe/bin/reframe", line 22, in <module>
    cli.main()
  File "/path/reframe/reframe/core/logging.py", line 968, in _fn
    return fn(*args, **kwargs)
  File "/path/reframe/reframe/frontend/cli.py", line 776, in main
    logging.configure_logging(site_config)
  File "/path/reframe/reframe/core/logging.py", line 921, in configure_logging
    _logger = _create_logger(site_config, 'handlers$', 'handlers')
  File "/path/reframe/reframe/core/logging.py", line 348, in _create_logger
    for handler in _extract_handlers(site_config, hgrp):
  File "/path/reframe/reframe/core/logging.py", line 629, in _extract_handlers
    hdlr = _create_file_handler(site_config, f'{handler_prefix}/{i}')
  File "/path/reframe/reframe/core/logging.py", line 369, in _create_file_handler
    filename = f'{basename}_{time.strftime(timestamp)}{ext}'
TypeError: strftime() argument 1 must be str, not bool

Easy enough to fix as I just set it to a timestamp string like %Y%m%d, but documentation might need to change or change code to allow for bool.

vkarak commented 6 months ago

I think that's present even in the latest. I've hit that too. Thanks for reporting. Indeed, the workaround is to set a date format. Maybe the docs are wrong here. 👀

brandonbiggs commented 6 months ago

Updating the docs would definitely be the easiest way to fix this. Do you want people to be able to set it to True and have it default to some default timestamp though?

vkarak commented 2 months ago

This is indeed a bug as even the config schema accepts boolean values for timestamp.