-
Notifications
You must be signed in to change notification settings - Fork 65
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
Unittest IPython extension + Automatic loading #478
Conversation
Thanks this looks good - could you resolve the merge conflicts please so CI can run? |
I was very confused about that error... It's actually up-to-date with |
Okay, all suggestions implemented. I made it work for IPython as well. |
My one other question is I fear the current warning is too verbose and a bit annoying:
Imagine getting hit with this every time you load the package... I propose we instead just make this:
and include the other information in the docs. Users can google how to turn it off if they want (my guess is that 99% of users want it turned on), and will land on the docs page with more details. |
648084a
to
dfe5b84
Compare
All suggestions implemented and I cleaned up the commit history |
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 mechanism looks great, just a couple of small comments to address then it's good to go.
@@ -248,8 +248,34 @@ def jlstr(x): | |||
"PYTHON_JULIACALL_HANDLE_SIGNALS=no." | |||
) | |||
|
|||
init() | |||
# Next, automatically load the juliacall extension if we are in IPython or Jupyter | |||
CONFIG['autoload_ipython_extension'] = choice('autoload_ipython_extension', ['yes', 'no'])[0] |
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.
Could you put this near the top of init? Just so if it's set incorrectly the error gets raised early.
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.
Don't we want it come after Julia is initialized? Otherwise we would see the printout for installing Julia, and the extension autoload message would be lost above it.
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.
I meant literally just the CONFIG['autoload_ipython_extension'] = ...
line.
|
||
if CONFIG['autoload_ipython_extension'] is None: | ||
# Only let the user know if it was not explicitly set | ||
print( |
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.
Should maybe be a warning, so can be disabled using Python's warnings mechanisms?
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.
I'm ambivalent about this.
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.
I tried this. If it is a warning, it feels like you need to set the environment variable (one way or the other), and ends up being a pain. When I have this set up I just want it to run automatically and not worry about it. A single-line print seems nice though.
I also tried logger.info
but it's also a pain because you need to reconfigure logging defaults to have it print. I guess I think this is really what print
is made for.
The user can always set that env variable to turn off printing. The printing only happens when it's unset.
Okay I took the advice into account and added docs on both the environment variables page and the IPython page. The automatic loading now links to the IPython page (https://juliapy.github.io/PythonCall.jl/stable/compat/#IPython) which seems nice as it points the user to the API. It's great for people who learn by trying. |
Thanks muchly :) |
This adds unittesting for the IPython extension, using nbval: https://nbval.readthedocs.io/
As discussed in issue () with @cjdoris, I have also set up the IPython extension to automatically load when it detects the user has loaded the package from inside Jupyter. I've done this with PySR and it has been really nice as you can basically just do
import pysr
and then start typing out%%julia
blocks of code as if you were using a native Julia notebook.This can be switched off with a new
"autoload_ipython_extension"
configuration parameter. The default behavior will be to assume"yes"
but will also print a helpful message to the user letting them know how to disable it if they desire. Setting this parameter explicitly to"yes"
will disable this message.