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

Improve logging #339

Merged
merged 14 commits into from
Jan 15, 2024
Merged

Improve logging #339

merged 14 commits into from
Jan 15, 2024

Conversation

teutoburg
Copy link
Contributor

  • Use hierarchical logger as outlined in Properly do logging #278
  • Better configuration of logging (more to come).
  • Set the default level for console logging to INFO to display messages of normal operation to the user, this should fix Logging to console #257
  • Any astar loggers from outside (e.g. skycalc_ipy) will default to WARNING.
  • Reduce INFO -> DEBUG in a few places to avoid spam.
  • Import logger factory from astar-utils to have a single place where these are defined.

@teutoburg teutoburg added dependencies Related to or updating any dependencies refactor Implementation improvement API How users interact with the software labels Jan 12, 2024
@teutoburg teutoburg self-assigned this Jan 12, 2024
@teutoburg
Copy link
Contributor Author

Oops, seems I didn't push the change to pyproject.toml where I added astar-utils 🤦

@hugobuddel
Copy link
Collaborator

FWIW, I still run the tests with python-randomly installed, and occasionally the tests fail with

tests/test_utils_functions.py .WARNING - Couldn't convert <table>.meta['comments'] to dict
.............F
>>>>>>>>>>>>> traceback >>>>>>>>>>>>>

self = <scopesim.tests.test_utils_functions.TestSetupLoggers object at 0x7f2143a04f50>

    def test_log_file_exist_when_file_logging_turned_on(self):
        utils.setup_loggers(log_to_file=True)
        filepath = Path(rc.__config__["!SIM.logging.file_path"])
        level = rc.__config__["!SIM.logging.file_level"]
        f_handler = logging.getLogger().handlers[-1]
    
        assert filepath.exists()
>       assert f_handler.level == logging._checkLevel(level)
E       AssertionError: assert 30 == 10
E        +  where 30 = <StreamHandler <stdout> (WARNING)>.level
E        +  and   10 = <function _checkLevel at 0x7f2161eb51c0>('DEBUG')
E        +    where <function _checkLevel at 0x7f2161eb51c0> = logging._checkLevel

tests/test_utils_functions.py:215: AssertionError
>>>>>>>>>>> entering PDB >>>>>>>>>>>>

>>>>>>>>>>> PDB post_mortem >>>>>>>>>>>
> /[...]/ScopeSim/scopesim/tests/test_utils_functions.py(215)test_log_file_exist_when_file_logging_turned_on()
-> assert f_handler.level == logging._checkLevel(level)

I've done 0 effort to figure out what causes this error, but was wondering whether this would fix it?

@hugobuddel
Copy link
Collaborator

FWIW, I still run the tests with python-randomly installed, and occasionally the tests fail with

>>>>>>>>>>>>> traceback >>>>>>>>>>>>>

self = <scopesim.tests.test_utils_functions.TestSetupLoggers object at 0x7f2143a04f50>

I've done 0 effort to figure out what causes this error, but was wondering whether this would fix it?

Uhm, yes this PR fixes that problem, because it removes TestSetupLoggers from the repo entirely! 🍷

@teutoburg
Copy link
Contributor Author

Uhm, yes this PR fixes that problem, because it removes TestSetupLoggers from the repo entirely! 🍷

Indeed, I'm planning to add new tests from scratch...

Copy link

codecov bot commented Jan 13, 2024

Codecov Report

Attention: 73 lines in your changes are missing coverage. Please review.

Comparison is base (f187c6d) 77.06% compared to head (1b6ba70) 77.04%.

Files Patch % Lines
scopesim/effects/spectral_trace_list_utils.py 33.33% 10 Missing ⚠️
scopesim/optics/image_plane_utils.py 77.41% 7 Missing ⚠️
scopesim/source/source.py 66.66% 7 Missing ⚠️
scopesim/optics/fov_utils.py 53.84% 6 Missing ⚠️
scopesim/effects/spectral_trace_list.py 28.57% 5 Missing ⚠️
scopesim/utils.py 66.66% 5 Missing ⚠️
scopesim/effects/electronic.py 75.00% 4 Missing ⚠️
scopesim/server/download_utils.py 42.85% 4 Missing ⚠️
scopesim/__init__.py 78.57% 3 Missing ⚠️
scopesim/commands/user_commands.py 40.00% 3 Missing ⚠️
... and 13 more
Additional details and impacted files
@@              Coverage Diff               @@
##           dev_master     #339      +/-   ##
==============================================
- Coverage       77.06%   77.04%   -0.03%     
==============================================
  Files              57       57              
  Lines            7714     7693      -21     
==============================================
- Hits             5945     5927      -18     
+ Misses           1769     1766       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@teutoburg
Copy link
Contributor Author

Coverage seems bad because many of the logging calls are warnings / errors in some rare condition, that is not tested...

@teutoburg teutoburg marked this pull request as ready for review January 14, 2024 14:34
Copy link
Collaborator

@hugobuddel hugobuddel left a comment

Choose a reason for hiding this comment

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

Thanks, another step in best-practices

@teutoburg teutoburg merged commit d97dfaa into dev_master Jan 15, 2024
15 of 16 checks passed
@teutoburg teutoburg deleted the fh/logging branch January 15, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API How users interact with the software dependencies Related to or updating any dependencies refactor Implementation improvement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Logging to console
2 participants