gh-148823: Avoid importing _colorize when creating an ArgumentParser - #148827
gh-148823: Avoid importing _colorize when creating an ArgumentParser#148827savannahostrowski merged 12 commits into
Conversation
Sorry, something went wrong.
This uses a new lazy import so it works with short circuiting alongside a `_colorless_theme` object to prevent the `_colorize` import if color is set to False. _theme and _decolor are now properties to prevent `_set_color` from performing the imports on creation of a formatter.
|
Ah this pinged more people than I had anticipated. This appears to be due to the modification to import_helper.py to allow for testing lazy imports with additional logic. |
Sorry, something went wrong.
| self.LAZY_IMPORTS, | ||
| ) | ||
|
|
||
| def test_create_parser(self): |
There was a problem hiding this comment.
Is it worth also checking .parse_args()? I think a good outcome is we only import colorize in the uncommon case where there's a parsing error or we invoke --help.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, it probably is. I'll add one. I'm fairly sure I at least tried this...
Sorry, something went wrong.
Documentation build overview97 files changed · + 2 added · ± 95 modified + Added ± Modified |
Sorry, something went wrong.
JelleZijlstra
left a comment
There was a problem hiding this comment.
This mostly looks good and I'd like to get it into 3.15, but tests are failing, we're actually still importing _colorize in one of the paths.
Sorry, something went wrong.
|
Yes, locally on this branch I'm not though which is a little confusing... All of the lazy import tests are now failing. I assume I'll have to handle the merge conflicts to figure out what's going on? |
Sorry, something went wrong.
|
Inline comments seem to disappear, but wanted to add that it would be nice to make it so parsing args doesn't trigger the lazy import of shutil. It's apparently only used in the HelpFormatter, which intuitively we shouldn't need if we're not showing help. However, that may be a more involved change than we can get in for 3.15. |
Sorry, something went wrong.
|
The shutil usage can probably be replaced - it's an environment variable check and then a call to os.get_terminal_size |
Sorry, something went wrong.
|
I feel ideally we should avoid instantiating the HelpFormatter at all but haven't looked into how hard that is. Having to inline get_terminal_size() would be a bit unfortunate. |
Sorry, something went wrong.
|
I generally agree on avoiding the HelpFormatter, there seems to be a lot of logic that gets formatted fairly early though. |
Sorry, something went wrong.
savannahostrowski
left a comment
There was a problem hiding this comment.
Overall, this looks great!
The only thing that we may want to consider is that since _colorize is private and _ColorlessTheme exists as an unenforced parallel implementation of ThemeSection.no_colors(), it may be worth adding a small test that imports _colorize, grabs get_theme(force_no_color=True).argparse, iterates its fields, and asserts each is ""?
Sorry, something went wrong.
That's fine, I can do that. I'm just trying to work out how to stop the import from being triggered due to the changes from #149375 Right now when any argument is added the help check now calls _apply_text_markup on the help string, importing _colorize due to this change https://github.com/DavidCEllis/cpython/blame/43b1c51105944aea17623e47f45294ba6b571c0c/Lib/argparse.py#L714 |
Sorry, something went wrong.
|
I cannot believe I have to comment via screenshot but inline comments don't work so... I think this should be safe to do since it's read only anyway? |
Sorry, something went wrong.
|
Yes! That's what I was looking for. I had got around to turning it off just for _check_help but this is better. Edit: Now how do I accept a change from a screenshot 😆 |
Sorry, something went wrong.
GitHub employees, if you're listening, I have a Copilot feature request. |
Sorry, something went wrong.
savannahostrowski
left a comment
There was a problem hiding this comment.
LGTM! Thank you!
Sorry, something went wrong.
This is one approach to deferring the _colorize import further.
This makes ._theme and ._decolor into properties so that _colorize is no longer imported when _set_color is called, but only when the theme is accessed.
Currently to prevent the import from subsequently being triggered when a subparser is created, this adds a 'fake' _colorless_theme which just returns empty strings for all attributes which replaces the actual colourless theme from _colorize.
Effectively this is what the colourless theme from _colorize does. I wanted to avoid duplicating all of the attribute names here or further complicating the logic, but there may be a better way to handle this.
Testing this did require an addition to the ensure_lazy_imports test helper to allow running additional code before checking for lazy imports. I think this might also be useful elsewhere when testing imports aren't being triggered earlier than intended.
Command: ./python -c 'import argparse; argparse.ArgumentParser()'
Benchmark 1: main Time (mean ± σ): 28.7 ms ± 3.3 ms [User: 24.1 ms, System: 4.3 ms] Range (min … max): 22.2 ms … 46.4 ms 200 runs Benchmark 2: lazy colorize Time (mean ± σ): 21.6 ms ± 5.8 ms [User: 17.3 ms, System: 4.0 ms] Range (min … max): 12.1 ms … 36.0 ms 200 runs Summary lazy colorize ran 1.33 ± 0.39 times faster than mainNote that this uses the new lazy imports, this would need significant tweaking if it were to be backported to 3.14.