-
Notifications
You must be signed in to change notification settings - Fork 251
[query/ggplot] runs plotting notebook setup on import #13610
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
e5720cc
to
c8491c7
Compare
c8491c7
to
2a2fa74
Compare
hail/python/hail/__init__.py
Outdated
@@ -11,6 +11,18 @@ | |||
del pkg_resources | |||
del sys | |||
|
|||
try: | |||
is_notebook = get_ipython().__class__.__name__ == 'ZMQInteractiveShell' |
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.
One query, one comment:
Query: how quick is time python3 -c 'import hail as hl'
on this branch and on main
?
Comment: we should re-use this variable instead of calling the same code in hailtop.utils.rich_progress_bar
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.
in zsh
on my machine with n = 5
, the difference between the execution time on this branch and main
was -0.14s
; no reason it should be faster than main
, so i assume that's just due to natural variation
in a jupyter notebook with the same parameters the difference was 0.12s
good catch, didn't realize we already checked whether we were in a notebook somewhere; updated! i'm not super clear on how hailtop.utils.rich_progress_bar.is_notebook
is currently used, though. what would be a good example to try out both locally and in a notebook to make sure that it still works as expected with the change?
2a2fa74
to
727cdc4
Compare
727cdc4
to
7e0f841
Compare
7e0f841
to
e0ad48e
Compare
hail/python/hail/__init__.py
Outdated
from IPython.core.getipython import get_ipython | ||
IS_NOTEBOOK = get_ipython().__class__.__name__ == 'ZMQInteractiveShell' | ||
except (NameError, ModuleNotFoundError): | ||
IS_NOTEBOOK = False |
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.
@iris-garden Put this in hailtop b/c hailtop isn't allowed to depend on hail (circularity!)
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.
moved!
0c88c7f
to
e1de458
Compare
e1de458
to
8d52b70
Compare
#13610 added the setup for bokeh and plotly into `hailtop/__init__.py`, which causes `IPython` to be imported when invoking `hailctl`, adding unnecessary startup time to it (observed between 0.210s and 0.400s). This change moves the setup into the `__init__.py` files for the plotting modules, with the resulting state stored in a global variable in `hailtop/__init__.py`. The plotting setup is still run upon invoking `import hail`, as this imports all submodules of hail as well, but the `IPython` import no longer happens when invoking `hailctl`.
CHANGELOG: Additional setup is no longer required when using
hail.plot
orhail.ggplot
in a Jupyter notebook (callingbokeh.io.output_notebook
orhail.plot.output_notebook
and/or settingplotly.io.renderers.default = 'iframe'
is no longer necessary).I double-checked locally that
import hail as hl
andfrom hail.ggplot import ggplot
both cause the added code to run, and that running them in a notebook does the setup we expect it to, but running it in iPython or the Python interpreter does not.