gh-144384: Lazily import _colorize#149318
Conversation
hugovk
requested review from
erlend-aasland,
lysnikolaou and
pablogsal
as code owners
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
Can we have a convention for lazy imports
| # All Rights Reserved | ||
| """Colorful tab completion for Python prompt""" | ||
| from _colorize import ANSIColors, get_colors, get_theme | ||
| lazy from _colorize import ANSIColors, get_colors, get_theme |
There was a problem hiding this comment.
Can we add a blank line before the lazy import please to improve readability? You could also move the lazy from after the import list as you did for other modules.
| def _safe_get_theme(*, force_color=False, force_no_color=False): | ||
| try: | ||
| return _colorize.get_theme( | ||
| force_color=force_color, force_no_color=force_no_color | ||
| ) | ||
| except ImportError: | ||
| return _shutdown_theme | ||
|
|
There was a problem hiding this comment.
Stupid question here, but can't we just use a local import then instead of a lazy one? or would we still have an issue with real imports?
There was a problem hiding this comment.
Yeah, test_print_traceback_at_exit fails with a local import. Things are very much in shutdown there!
hugovk
left a comment
hugovk
left a comment
There was a problem hiding this comment.
Can we have a convention for lazy imports
Yes, given we don't have a convention, and the SC left it the details to linters and auto-formatters, let's generally follow the Ruff/isort convention:
import json
import os
import subprocess
from collections import defaultdict
from pathlib import Path
from typing import Final
lazy import ast
lazy import shutil
lazy from dataclasses import dataclass| def _safe_get_theme(*, force_color=False, force_no_color=False): | ||
| try: | ||
| return _colorize.get_theme( | ||
| force_color=force_color, force_no_color=force_no_color | ||
| ) | ||
| except ImportError: | ||
| return _shutdown_theme | ||
|
|
There was a problem hiding this comment.
Yeah, test_print_traceback_at_exit fails with a local import. Things are very much in shutdown there!
| lazy import _colorize | ||
|
|
||
|
|
||
| class _ShutdownTheme: |
There was a problem hiding this comment.
The shutdown logic could be moved after the _missing_stdlib_info actually line because _missing_stdlib_info is still imported (so probably after the __all__). Or is it also because we want to have those objects as soon as possible at interpreter's finalization?
There was a problem hiding this comment.
Yep it can move, I put it below __all__.
|
Seeing #149321, I think this should wait till that is somewhat figured out. |
3 participants
Lazily import
_colorizeto avoid slow construction/import when not needed.The easily testable:
Some indirect improvements:
tracebackneeds some extra handling: add a shutdown theme, so if it attempts to reify_colorizeduring shutdown when the import machinery is no longer around, it has a no-op fallback.Also add
ensure_lazy_importstests._colorizemodule is slow to import, affectingtracebackandloggingmodules #144384