Skip to content

gh-148823: Avoid importing _colorize when creating an ArgumentParser#148827

Merged
savannahostrowski merged 12 commits intopython:mainfrom
DavidCEllis:defer-colorize-argparse
May 6, 2026
Merged

gh-148823: Avoid importing _colorize when creating an ArgumentParser#148827
savannahostrowski merged 12 commits intopython:mainfrom
DavidCEllis:defer-colorize-argparse

Conversation

@DavidCEllis
Copy link
Copy Markdown
Contributor

@DavidCEllis DavidCEllis commented Apr 21, 2026

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 main

Note that this uses the new lazy imports, this would need significant tweaking if it were to be backported to 3.14.

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.
@DavidCEllis
Copy link
Copy Markdown
Contributor Author

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.

@brettcannon brettcannon removed their request for review April 27, 2026 21:40
Comment thread Lib/test/test_argparse.py
self.LAZY_IMPORTS,
)

def test_create_parser(self):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it probably is. I'll add one. I'm fairly sure I at least tried this...

Comment thread Lib/test/test_argparse.py Outdated
@read-the-docs-community
Copy link
Copy Markdown

read-the-docs-community Bot commented May 6, 2026

Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DavidCEllis
Copy link
Copy Markdown
Contributor Author

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?

@JelleZijlstra
Copy link
Copy Markdown
Member

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.

@DavidCEllis
Copy link
Copy Markdown
Contributor Author

The shutil usage can probably be replaced - it's an environment variable check and then a call to os.get_terminal_size

@JelleZijlstra
Copy link
Copy Markdown
Member

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.

@DavidCEllis
Copy link
Copy Markdown
Contributor Author

Ah, it's #149375

@DavidCEllis
Copy link
Copy Markdown
Contributor Author

I generally agree on avoiding the HelpFormatter, there seems to be a lot of logic that gets formatted fairly early though.

Copy link
Copy Markdown
Member

@savannahostrowski savannahostrowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ""?

@DavidCEllis
Copy link
Copy Markdown
Contributor Author

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 ""?

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

@savannahostrowski
Copy link
Copy Markdown
Member

I cannot believe I have to comment via screenshot but inline comments don't work so...
image

I think this should be safe to do since it's read only anyway?

@DavidCEllis
Copy link
Copy Markdown
Contributor Author

DavidCEllis commented May 6, 2026

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 😆

@savannahostrowski
Copy link
Copy Markdown
Member

Edit: Now how do I accept a change from a screenshot 😆

GitHub employees, if you're listening, I have a Copilot feature request.

Copy link
Copy Markdown
Member

@savannahostrowski savannahostrowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

@savannahostrowski savannahostrowski enabled auto-merge (squash) May 6, 2026 17:54
@savannahostrowski savannahostrowski merged commit 8cad740 into python:main May 6, 2026
52 checks passed
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.

4 participants