-
-
Notifications
You must be signed in to change notification settings - Fork 770
⚡️ 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
Conversation
I am also looking forward for this quick win, CLI should be faster than it is right now... |
rich_utils
to reduce startup time
+1 |
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.
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.
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. |
OK, added a test |
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.
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.
The performance impact is negligible. Essentially, it boils down to one dictionary lookup (to test whether the module is already in Also, most of the affected lines are executed at most once per run. |
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.
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.
I just tested these changes, it makes a noticeable difference for my CLI. https://gist.github.com/onyx-and-iris/987ad066cb9baa03b58f5f446f7ea837 Thank you for investigating this issue. |
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.
Awesome! Came here after I found out that Typer is the one package that is the slowest part of (my CLI).
@tiangolo, it seems like this PR is blocked by your review, would you mind giving it a few minutes of your time? In my application ![]() |
+1 on this |
+1 !!! |
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.
@tiangolo There is still one additional performance improvement missing, not on CLI execution but for CLI help text rendering. Specifically-- lazy-loading There are many libraries in the wild which take a long time to load their CLIs (for reasons unrelated to Typer) and so every fraction of a second shaved off can make things feel snappier basically for free. |
@dwreeves: thanks for the note. Please start a new discussion for this, as this PR has been merged and closed. When opening the discussion, it'd be great if you can quantify the improvement you get from lazy loading |
rich.markup is used to escape strings when rendering help texts. This fixes a regression introduced by fastapi#1128
Typer 0.9.4 is not compatible with click 8.2.1 due to a missing positional argument in rich_utils.py:370, so update Typer to the latest version. This introduced a breaking change, as the rich_utils module imported to override the dim text in help messages is now lazy-loaded [1]. The solution is to directly import rich_utils from typer. Typer was not updated due to using the caret requirement [2]. The constraint stays for now, considering potential breaking changes introduced in new versions. However, the constraint on lief changes ~=0.16, a compatible release [3] allowing >=0.16.0 and <1.0.0. [1]: fastapi/typer#1128 [2]: https://python-poetry.org/docs/dependency-specification/#caret-requirements
Typer 0.9.4 is not compatible with click 8.2.1 due to a missing positional argument in rich_utils.py:370, so update Typer to the latest version. This introduced a breaking change, as the rich_utils module imported to override the dim text in help messages is now lazy-loaded [1]. The solution is to directly import rich_utils from typer. Typer was not updated due to using the caret requirement [2]. The constraint stays for now, considering potential breaking changes introduced in new versions. However, the constraint on lief changes ~=0.16, a compatible release [3] allowing >=0.16.0 and <1.0.0. [1]: fastapi/typer#1128 [2]: https://python-poetry.org/docs/dependency-specification/#caret-requirements
Lazy-load
rich_utils
(andrich.traceback
) to reduce import time.Fixes #744
There was some discussion in that issue about using
lazyasd
andif 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/orif 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
After