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

clean up logging configuration #919

Merged
merged 3 commits into from
Jul 28, 2022
Merged

clean up logging configuration #919

merged 3 commits into from
Jul 28, 2022

Conversation

anthrotype
Copy link
Member

@anthrotype anthrotype commented Jul 28, 2022

logging configuration is global, does not belong to a specific fontmake.FontProject instance, so it should only be done inside main(). This PR deprecates the verbose and timing parameters to FontProject constructor and moves the logging configuration to the CLI module (fontmake.__main__).
The old parameter continue to work but a deprecation warning is issued (category is actually UserWarning as DeprecationWarning proper are usually muted -- just to shout it out more louder)

It also enables logging configuration for a logger named "ufo2ft.timer" (which reports timing info inside ufo2ft) which doesn't exist yet but will soon (googlefonts/ufo2ft#641)
This and fontmake's own timing logger are controlled by the fontmake --timing flag, independently from the --verbose LEVEL flag which control the root logger configuration.

…() instance constructor

and deprecate the 'verbose' and 'timing' parameter to FontProject constructor.
This always bugged me, I've been wanting to do this for ages.
@anthrotype
Copy link
Member Author

anthrotype commented Jul 28, 2022

I think this is fairly uncontroversial so I'll go on and merge. We can decide to decorate more methods with @timer or add context-manager with timer(...): to blocks of code that we deem useful. @simoncozens is doing that in googlefonts/ufo2ft#641 and fontmake (with this PR) is already ready to handle messages from the "ufo2ft.timer" logger.

@anthrotype anthrotype merged commit 8f18f73 into main Jul 28, 2022
@anthrotype anthrotype deleted the logging branch July 28, 2022 16:24
@simoncozens
Copy link
Contributor

One important thing now (since we do not have hierarchical timers...) is that we need to be careful to avoid decorating an operation and a constituent part of the same operation, or else we lose the ability to add the timings together. What I mean is:

Took 5.00s to run MarkFeatureWriter
Took 7.00s to run KernFeatureWriter
Took 12.00s to run feature writers

Unless you teach your analysis code that MarkFeatureWriter and KernFeatureWriter are inside feature writers, then it will seem like that part of the run took 24 seconds.

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.

2 participants