Skip to content

⚡️ Lazy-load rich_utils to reduce startup time #1128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

oefe
Copy link

@oefe oefe commented Jan 19, 2025

Lazy-load rich_utils (and rich.traceback) to reduce import time.

Fixes #744

There was some discussion in that issue about using lazyasd and if TYPE_CHECKING: in order to make the import known to type checkers and build tools that perform import analysis.

To keep it simple, I didn't go that route.
There are already a few lazy imports, so I think this doesn't change the big picture.
And lazyasd appears to be no longer maintained.

Let me know if you rather would use lazyasd and/or if TYPE_CHECKING:.

Benefit

This cuts down startup overhead by 45%. Below are some measurements (best of 5 runs).
Measured on a Mac mini (M4 pro).

Before

~/R/typer►time uv run docs_src/first_steps/tutorial001.py
Hello World

________________________________________________________
Executed in   94.19 millis    fish           external
   usr time   71.72 millis    0.15 millis   71.57 millis
   sys time   19.54 millis    1.76 millis   17.77 millis

before

After

~/R/typer►time uv run docs_src/first_steps/tutorial001.py
Hello World

________________________________________________________
Executed in   51.99 millis    fish           external
   usr time   37.36 millis    0.10 millis   37.26 millis
   sys time   12.77 millis    1.37 millis   11.39 millis

after

@svlandeg svlandeg added the feature New feature, enhancement or request label Jan 20, 2025
@bonastreyair
Copy link

I am also looking forward for this quick win, CLI should be faster than it is right now...

@svlandeg svlandeg self-assigned this Feb 17, 2025
@svlandeg svlandeg changed the title Lazy-load rich_utils to reduce startup time ⚡️ Lazy-load rich_utils to reduce startup time Feb 20, 2025
@N-Demir
Copy link

N-Demir commented Feb 25, 2025

+1

Copy link

@stinovlas stinovlas left a comment

Choose a reason for hiding this comment

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

Looks good to me (I'm not a maintainer though).

I did follow the review process, downloaded and ran the code. Everything seems to be fine. This PR doesn't add any tests and I think that's OK, because the code is already tested and it's not changing behavior, it's just a performance optimization.

@oefe
Copy link
Author

oefe commented Mar 27, 2025

Originally, I thought the same. While the speedup is significant, it would be difficult to test reliably in a unit test, and I didn't want to introduce a flaky test.

But thinking about it again, I realize that it would be possible to assert that rich is not imported. This is deterministic and should be testable.

@oefe
Copy link
Author

oefe commented Mar 29, 2025

OK, added a test

Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Thanks for the clear analysis and PR proposal @oefe!

I can definitely see the appeal of doing the lazy load to improve startup time when rich isn't used. It clutters the code a little bit, but the trade-off may be worth it.

One question I have is for when you do have Rich installed. Do the multiple import statements at different points in the code base result in an efficiency loss somehow? Because I can imagine that in the background, it is being checked what is already imported and what isn't. I'm not sure if that check leaves a performance trace or not.

for module in modules
if module not in ACCEPTED_MODULES and module.startswith("rich")
]
assert not modules
Copy link
Member

Choose a reason for hiding this comment

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

This is a matter of personal preference, but for clarity of code I would prefer

Suggested change
assert not modules
assert len(modules) == 0

Copy link
Author

Choose a reason for hiding this comment

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

This is arguably the most idiomatic style, see also PEP8:

For sequences, (strings, lists, tuples), use the fact that empty sequences are false:

# Correct:
if not seq:
if seq:
# Wrong:
if len(seq):
if not len(seq):

But I can change it if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

I'll leave the final decision with Tiangolo 🙏

@svlandeg svlandeg removed their assignment Apr 17, 2025
@oefe
Copy link
Author

oefe commented Apr 17, 2025

The performance impact is negligible. Essentially, it boils down to one dictionary lookup (to test whether the module is already in says.modules) and one dictionary insertion (add the module to the local namespace). These are both very quick.

Also, most of the affected lines are executed at most once per run.

Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

The performance impact is negligible.

Thanks for double checking!

This PR looks good to merge to me, I'll leave it to Tiangolo for a final review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature, enhancement or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants