GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-118761: improve optparse import time by delaying textwrap import by eli-schwartz · Pull Request #128899 · python/cpython

Conversation

@eli-schwartz

@eli-schwartz eli-schwartz commented Jan 16, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

The same change was made, and for the same reason, by argparse back in
2017. The textwrap module is only used when printing help text, so most invocations will never need it imported.

See: #1269
See: 8110837

  • Issue: Improve import time of various stdlib modules #118761

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot mentioned this pull request Jan 16, 2025
Improve import time of various stdlib modules #118761
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Jan 16, 2025
@eli-schwartz eli-schwartz mentioned this pull request Jan 16, 2025
Speed up small CLI tools by removing import re from the excecutable template pypa/pip#13165
Closed
1 task done
ZeroIntensity
ZeroIntensity approved these changes Jan 16, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Jan 16, 2025
AA-Turner
AA-Turner reviewed Jan 16, 2025
View reviewed changes
Comment thread Lib/optparse.py Outdated
Comment on lines +254 to +256
text_width = max(self.width - self.current_indent, 11)
indent = " "*self.current_indent
import textwrap

@AA-Turner AA-Turner Jan 16, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Style: move local imports to the top of the block

Suggested change
text_width = max(self.width - self.current_indent, 11)
indent = " "*self.current_indent
import textwrap
import textwrap
text_width = max(self.width - self.current_indent, 11)
indent = " "*self.current_indent

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@eli-schwartz eli-schwartz Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sure, makes sense!

For what it's worth the reason I did it this way was a direct consequence of having just been reading argparse.py, which breaks this style suggestion, while documenting the motivation of the change. :) I suppose it is not worth bothering with the churn there as it's just a style nit on code that isn't being modified already...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/optparse.py
help_text = self.expand_default(option)
import textwrap

@AA-Turner AA-Turner Jan 16, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Style: move local imports to the top of the block

Suggested change
help_text = self.expand_default(option)
import textwrap
import textwrap
help_text = self.expand_default(option)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@eli-schwartz
pythongh-118761: improve optparse import time by delaying textwrap im…
10cd152
…port

The same change was made, and for the same reason, by argparse back in
2017. The textwrap module is only used when printing help text, so most
invocations will never need it imported.

See: python#1269
See: 8110837
@eli-schwartz
eli-schwartz force-pushed the optparse-importtime branch from 4484efc to 10cd152 Compare January 17, 2025 00:44
@picnixz

picnixz commented Jan 17, 2025

Copy link
Copy Markdown
Member

Out of curiosity, how are the hyperfine and -X importtime benchmarks?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@eli-schwartz

eli-schwartz commented Jan 17, 2025

Copy link
Copy Markdown
Contributor Author

As mentioned in the other PR -- I'm not used to benchmarking and I may be doing something totally wrong because I can get results saying this PR is now slower to import, or results saying it's faster, and hyperfine warns that I have lots of "outliers". But here's a favorable result.

Produced by copying the old and new versions of the file so that I can import them both without switching branches:

$ LD_LIBRARY_PATH=$PWD hyperfine --warmup 8 "./python -c 'import optparse_old'" "./python -c 'import optparse_new'"
Benchmark 1: ./python -c 'import optparse_old'
  Time (mean ± σ):      23.6 ms ±   7.7 ms    [User: 18.4 ms, System: 5.1 ms]
  Range (min … max):    17.2 ms …  47.7 ms    92 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 2: ./python -c 'import optparse_new'
  Time (mean ± σ):      17.5 ms ±   2.8 ms    [User: 14.4 ms, System: 3.0 ms]
  Range (min … max):    16.3 ms …  31.1 ms    98 runs
 
  Warning: The first benchmarking run for this command was significantly slower than the rest (29.2 ms). This could be caused by (filesystem) caches that were not filled until after the first run. You are already using the '--warmup' option which helps to fill these caches before the actual benchmark. You can either try to increase the warmup count further or re-run this benchmark on a quiet system in case it was a random outlier. Alternatively, consider using the '--prepare' option to clear the caches before each timing run.
 
Summary
  ./python -c 'import optparse_new' ran
    1.35 ± 0.49 times faster than ./python -c 'import optparse_old'
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AA-Turner
AA-Turner approved these changes Jan 20, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jan 20, 2025
@AA-Turner
AA-Turner merged commit 3b6a27c into python:main Jan 20, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 20, 2025
@eli-schwartz
eli-schwartz deleted the optparse-importtime branch January 20, 2025 00:09
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
@eli-schwartz @AA-Turner @srinivasreddy
pythongh-118761: Redudce the import time of optparse (python#128899)
2197bd9
The same change was made, and for the same reason, by ``argparse`` back in
2017. The ``textwrap`` module is only used when printing help text, so most
invocations will never need it imported.

Co-authored-by: Adam Turner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AA-Turner AA-Turner AA-Turner approved these changes
@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@eli-schwartz @picnixz @AA-Turner @ZeroIntensity
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.