Faster imports #23
Faster imports #23
Conversation
with sorted the files will be sort by the timestamp, not the file names
| ignore = "E402,C901,C0103,E203,R0912,R0915,R901,R901,R0914,C0413,C0206,R1716,W1514,R0902,R0913,W0707,W0102,W0107,R0903" | ||
| # W1201: Use % formatting in logging functions | ||
| # W1203: Use %s formatting in logging functions | ||
| ignore = "E402,C901,C0103,E203,R0912,R0915,R901,R901,R0914,C0413,C0206,R1716,W1514,R0902,R0913,W0707,W0102,W0107,R0903,W1201,W1203" |
There was a problem hiding this comment.
Instead of ignoring W1203 in pylama, for deliberate style choices (and looking at the pylint issues this was a pretty controversial lint) it's a good idea to disable it in pylint directly using a new section:
[tool.pylint.logging]
disable="logging-fstring-interpolation"Pylama will pick up this config anyway, and this way I can run pylint directly too without the noise.
On the other hand, W1201 seems like something worth fixing directly. That's warning about using concatenation instead of an fstring, the three instances can be upgraded to fstrings.
| @@ -32,6 +33,9 @@ | |||
| except ImportError: | |||
| __version__ = "0.0.0" # package is not installed | |||
|
|
|||
| logger = logging.getLogger(__file__) | |||
| LOGGER_FORMAT = "%(asctime)s %(levelname)s [%(filename)s:%(lineno)s] %(message)s" | |||
There was a problem hiding this comment.
This format string gets pretty close to the old loguru one: %(asctime)s | %(levelname)-9s | %(name)s:%(funcName)s:%(lineno)s - %(message)s.
Importantly this includes name (the logger name) and funcName. Just using filename lead to some confusion because it doesn't show the path to the filename. The logs were getting spammed from base.py and it took a hot second to figure out where base.py was.
| @@ -32,6 +33,9 @@ | |||
| except ImportError: | |||
| __version__ = "0.0.0" # package is not installed | |||
|
|
|||
| logger = logging.getLogger(__file__) | |||
| LOGGER_FORMAT = "%(asctime)s %(levelname)s [%(filename)s:%(lineno)s] %(message)s" | |||
|
|
|||
There was a problem hiding this comment.
On the topic of base.py, it comes from canopen. It's logging a bunch of stuff about PDO creation(?) to loglevel INFO that we don't need to see. Either here or near a canopen import it'd be nice to set it to a more reasonable logging.getLogger("canopen").setLevel(logging.ERROR).
|
|
||
| logger_tmp_file_setup(level) | ||
| logging.basicConfig(format=LOGGER_FORMAT, level=level, handlers=handlers) |
There was a problem hiding this comment.
Setting the format argument will automatically set the formatter for all Handlers passed to handlers (it's the only thing that the format arg does). You can save a few lines above by removing the formatter bits.
| if args.log: | ||
| logger.add(SysLogHandler(address="/dev/log"), level=level, backtrace=True) | ||
| syslog_handler = SysLogHandler() |
There was a problem hiding this comment.
Is there still a reason to have a dedicated SysLogHandler? The two running environments I'm aware of are dev in which case I always want stdio, and on the cards where they're being run as systemd units and stdio is redirected to the journal anyway.
| else: | ||
| logger.add(sys.stdout, level=level, backtrace=True) | ||
| stream_handler = logging.StreamHandler() |
There was a problem hiding this comment.
I don't know if using stdout previously was deliberate, but if it was StreamHandler defaults to stderr.
|
|
||
| logger_tmp_file_setup(level) | ||
| logging.basicConfig(format=LOGGER_FORMAT, level=level, handlers=handlers) |
There was a problem hiding this comment.
Configuring logging here, during the application start, means that not all log entries may be handled by the appropriate handlers. In the C3 app for example there's a line about the RTC that occurs before olaf_setup(). If basicConfig() were moved up into the global scope then it would reasonably configure the loggers before any of our code gets run.
The only catch is that it needs to be configured here because the list of handlers depends on the result of argparse, but does it really need to? If SysLogHandler was dropped then that would free this up to be moved.
Tested with
python -X importtime run.py gps -b can0on C3 card at max CPU frequencyImport time before changes: 5.9s
Import time after changes: 4.1s
Changes:
natsortwith a custom function (removed 300ms)loguruwith build-inloggingmodule (removed 260ms)python-canto 4.3.0 (release fixed internalpkg_resourceslowdown, removed 1s)Could remove 1.5s more if removed or split out the
Flaskapp, but that could make the projects harder to modify for new students.