-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Avoid importing IPython
if notebook cells do not contain magics
#3782
Conversation
528c64f
to
9c403c1
Compare
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.
Hmm, I thought about this, but when this returns true we'll end up importing these anyway right? In which case this introduces overhead
I guess we can save if there isn't a cell magic in the notebook |
TBH I didn't look at how the function was integrated in the rest of the control flow. From an external point of view I thought it might be preferable to decouple introspection and actual imports, especially if the added overhead is 3 orders of magnitude bellow the import cost, but your call ! :) |
9c403c1
to
0f8bd82
Compare
diff-shades reports zero changes comparing this PR (0afa13c) to main (0e26ada).%0A%0A---%0A%0AWhat is this? | Workflow run | diff-shades documentation |
IPython
if notebook cells do not contain magics
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 idea sounds reasonable and the changes look good. Please address failing lint CI though.
Yeah, sorry, lint is failing because of click stuff again that's unrelated to these changes, #3791 to fix |
Thanks @neutrinoceros and @hauntsaninja! |
Description
follow up to #3748: completely avoid import overhead in
jupyter_dependencies_are_installed
. This function now takes <0.2ms regardless of the result, while the current implementation takes >200ms on first call when the result isTrue
.Checklist - did you ...
CHANGES.md
if necessary?