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

Kgn/logging revamp #2720

Merged
merged 13 commits into from
May 2, 2024
Merged

Kgn/logging revamp #2720

merged 13 commits into from
May 2, 2024

Conversation

karnigen
Copy link
Collaborator

Attempt to centralize logging and warning settings, along with their configuration, from an external file.

Some ideas can be found in: Modern Python logging

  • The goal is to set up logging from the file LOGGING.toml
  • To use the logging and warning libraries for logging purposes.
  • Using so-called root logger to control all other loggers in the application.

root logger:

  • The primary logger manages all other loggers through logging.xxx() calls.
  • It should only be utilized at the application's highest level to manage the logging of all loggers.
  • It can easily disable all loggers by invoking logging.disable() and channel all warnings to logging
    by setting logging.captureWarnings(True) with the level WARNING.
  • The configuration of all loggers can be achieved via a file, and logging.config.dictConfig(logging_dict).

module logger:

  • Instantiate the logger by invoking logger=getLogger(name).
    Avoid using __name__ as the name, as it generates numerous loggers per application.
    The logger name persists globally throughout the application.
  • Avoid configuring the module logger within the module itself;
    instead, utilize the top-level application configuration with logging.config.
    This allows users of the application to customize it according to their requirements.

suggested usage of module logger:

import logging
logger = logging.getLogger('inkstitch')  # create module logger with name 'inkstitch', but configure it at top level of app
 ...
   logger.debug('debug message')          # example of using module logger
 ...

Current status

When running inkstitch as frozen mode normally we want to ignore all warnings and logging, but we can enable it by setting environment variables before inkscape is started:

  • INKSTITCH_LOGLEVEL - logging level: 'DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL'
  • PYTHONWARNINGS, -W - warnings action controlled by python actions: 'error', 'ignore', 'always', 'default', 'module', 'once'
  • Logging is redirected to directory of input svg file input-svg-file.inkstitch.log

When running in development mode we want to use configuration from some LOGGING.toml file:

  • Name of configuration file is given by DEBUG.ini, log_config_file=LOGGING.toml
  • Template files are also available as LOGGING_template.toml and DEBUG_template.ini.
  • File debug_logging.py contains default setting for logging system if the configuration file is missing.

why TOML format

  • TOML's clear syntax and structure enhance maintainability and readability (similar to INI).
  • TOML is integrated as a standard library in newer versions of Python.
  • For older versions, a requirement for TOML is added to the requirements.

@kaalleen
Copy link
Collaborator

I do like the idea. @lexelby what do you think?

inkstitch.py Outdated Show resolved Hide resolved
@lexelby
Copy link
Member

lexelby commented Feb 18, 2024

I like the idea of centralizing everything. I wonder if we should go even further by changing lib/debug.py to use the logging system too? Right now it just prints to the log file.

With the addition of another lib/debug_*.py file, I wonder if we should make a lib/debug/ directory. I know that we need to be careful to avoid import loops, but we can potentially handle that by switching to local imports in some places.

@karnigen
Copy link
Collaborator Author

Some improvements:

  • Moving debug* files to the debug subdirectory.
  • Changing DEBUG.ini to DEBUG.toml and removing support for ini format.
  • Splitting debug.py into debugger.py to enable debugging alone.
  • Debug configuration is driven by DEBUG.toml.
  • Logging configuration is driven by LOGGING.toml.
  • Output to debug.log and debug.svg is fully driven by inkstitch.debug logger; refer to LOGGING.toml and corresponding parameters are removed from DEBUG.toml.

There are also template files for DEBUG and LOGGING.

Additionally, please ensure to test thoroughly before deployment.

@karnigen karnigen requested a review from lexelby March 22, 2024 19:10
@kaalleen
Copy link
Collaborator

Thank you for all the work you've done!
Would you mind to rebase your branch on main? It's hard to review like this.

@karnigen
Copy link
Collaborator Author

I tried to perform a rebase, but it seems like it's doing a merge instead. It transferred commits from the main branch to this branch. I'm not clear on how it works either.

@kaalleen
Copy link
Collaborator

Make sure your local main branch is up to date. Then you checkout into your branch and type git rebase main it should have no further complaints and rebase without conflict. Since upstream and local branches are now diverged you'll have to force push to your branch git push --force.

@karnigen
Copy link
Collaborator Author

Now it's exactly as it should be, thank you.

inkstitch.py Show resolved Hide resolved
lib/debug/debugger.py Outdated Show resolved Hide resolved
lib/debug/logging.py Outdated Show resolved Hide resolved
lib/debug/logging.py Outdated Show resolved Hide resolved
@lexelby
Copy link
Member

lexelby commented Mar 24, 2024

Just a few minor comments, but this is looking really good!

@karnigen
Copy link
Collaborator Author

Another attempt.

@karnigen
Copy link
Collaborator Author

Another attempt without AI.

@kaalleen
Copy link
Collaborator

Since the "print pdf revert to browser" branch has been merged this PR has conflicts in the print_pdf file. Sorry for the trouble.
Would you mind to update the branch and resolve conflicts (or do you rather want us to take over)?

@karnigen
Copy link
Collaborator Author

Thank you for letting me know about the conflicts. Of course, feel free to perform any updates at any time. I'll be happy to assist.

@kaalleen
Copy link
Collaborator

Thanks for the update! I rather leave the review to @lexelby.

@karnigen
Copy link
Collaborator Author

Is there anything else that needs to be addressed or any issues with the TOML format?

@kaalleen
Copy link
Collaborator

kaalleen commented May 1, 2024

I did some merging and again there is a merge conflict for this branch (sorry for that)
In order to get rid of electron, the file lib/api/server.py has been removed and there were changes to the file in this branch.
I thought I ask you first before I do anything, if you want to do the rebase yourself...

@karnigen
Copy link
Collaborator Author

karnigen commented May 1, 2024

Thank you for the information. At least I got to try the rebase when the file is removed. Hopefully, I did it correctly. I'm still not entirely sure if everything went smoothly, but fingers crossed.

@kaalleen
Copy link
Collaborator

kaalleen commented May 2, 2024

Thank you so much. Ink/Stitch is a good learning field - at least it has been for me (and still is).
Your rebase looks fine - at least the file is still gone ;)

@karnigen karnigen merged commit bf5c2df into main May 2, 2024
5 checks passed
@karnigen
Copy link
Collaborator Author

karnigen commented May 2, 2024

Thank you very much for the review. I hope the modifications will be beneficial.

@kaalleen kaalleen deleted the kgn/logging-revamp branch June 10, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants