Skip to content
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

Merged
merged 9 commits into from Jul 17, 2023

Conversation

neutrinoceros
Copy link
Contributor

@neutrinoceros neutrinoceros commented Jul 11, 2023

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 is True.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@neutrinoceros neutrinoceros marked this pull request as ready for review July 11, 2023 08:12
Copy link
Collaborator

@hauntsaninja hauntsaninja left a 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

@hauntsaninja
Copy link
Collaborator

I guess we can save if there isn't a cell magic in the notebook

@neutrinoceros
Copy link
Contributor Author

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 ! :)

@github-actions
Copy link

github-actions bot commented Jul 11, 2023

diff-shades reports zero changes comparing this PR (0afa13c) to main (0e26ada).%0A%0A---%0A%0AWhat is this? | Workflow run | diff-shades documentation

@hauntsaninja hauntsaninja changed the title PERF: minimize cost of introspection to detect jupyter dependencies Avoid importing IPython if notebook cells do not contain magics Jul 16, 2023
Copy link
Collaborator

@ichard26 ichard26 left a 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.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jul 17, 2023

Yeah, sorry, lint is failing because of click stuff again that's unrelated to these changes, #3791 to fix

@ichard26 ichard26 merged commit 92e0f5b into psf:main Jul 17, 2023
35 checks passed
@ichard26
Copy link
Collaborator

Thanks @neutrinoceros and @hauntsaninja!

@neutrinoceros neutrinoceros deleted the effortless_introspection branch July 17, 2023 05:10
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.

None yet

4 participants