Fix test failures with ipython 8.11 #35235
Conversation
|
Documentation preview for this PR is ready! 🎉 |
|
Yes, I can see those. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #35235 +/- ##
===========================================
+ Coverage 88.58% 88.61% +0.03%
===========================================
Files 2141 2148 +7
Lines 397475 398654 +1179
===========================================
+ Hits 352106 353287 +1181
+ Misses 45369 45367 -2
... and 60 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Hmm... are we sure the warning only happens on these doctests? Wouldn't it be better to either fix or ignore the warning? Someone put the warning there for some reason. Even if the warning is irrelevant, did you make sure it won't show up to a normal user? |
|
Indeed: I think we should figure out if the warning means something has to be changed, or if the warning can be safely ignored in general, and keep the doctest as is so it makes sure the warning doesn't reappear. |
|
As of fb2a1f9 the inputhook is not installed once at initialization but each time a file is attached (including when attached files are reloaded). The warning complains about that. The following seems to fix it: the The call to I still don't know how to replace the branch in a PR like we used to do in trac. Opening a new PR seems too much noise. --- a/src/sage/repl/inputhook.py
+++ b/src/sage/repl/inputhook.py
@@ -49,13 +49,23 @@ def install():
EXAMPLES::
+ Make sure ipython is running so we really test this function
+
+ sage: from sage.repl.interpreter import get_test_shell
+ sage: get_test_shell()
+ <sage.repl.interpreter.SageTestShell object at ...>
+
+ Test the function twice, to make sure it is idempotent (see :trac:`35235`)
+
sage: from sage.repl.inputhook import install
sage: install()
+ sage: install()
"""
ip = get_ipython()
if not ip:
return # Not running in ipython, e.g. doctests
- ip.enable_gui('sage')
+ if ip._inputhook != sage_inputhook:
+ ip.enable_gui('sage')
def uninstall(): |
|
I certainly do like the approach. Is it compatible with older ipython or does it require us to move to ipython 8.11? I would not have an issue with that but I would be surprised if it was needed. |
I didn't test but I don't think it is, indeed, all checks have passed here using older ipython. All we do here is to skip changing the inputhook if it is already set to our own. In this case |
|
I pushed the branch with my fix to tornaria:ipython-8.11. The commit is tornaria@502cb00, in case it helps. I don't know if it's possible to replace antonio-rojas:ipython-8.11 by tornaria:ipython-8.11 in this PR, maybe @arojas can just grab my commit. |
This reverts commit 3654f2e.
|
Done |
|
@tornaria |
|
Looking at other files I believe you need something like' |
|
Sorry, I usually don't build docs... I'll redo my commit. It turns out running Can someone link to documentation explaining the doctest formats? |
|
I have updated my branch tornaria:ipython-8.11. The new commit is tornaria@23471e2. Note that I rebased on top of beta4 and redid the commit -- you won't be able to merge, just resetting your branch to mine seems the easier way (also, I find it very noisy that we keep a commit + revert the commit). I'm still wondering: maybe the workflow should be that we close this PR and I open a separate one. It's very unfortunate that there is so much friction to work cooperatively on a PR. @mkoeppe any suggestions? |
|
Let's just open a new pull request with your branch |
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes #1234" use "Introduce new method to calculate 1+1" --> ### 📚 Description ipython 8.11 throws new warnings "UserWarning: Shell was already running a gui event loop for sage; switching to sage." when running some tests. This is because sage indeed tries to install the same gui event loop for ipython again and again. The fix is checking if the ipython input hook is already set to the sage input hook, in that case we don't install the gui event loop again. In the test, we run `install()` twice so we can verify that running it multiple times won't cause a warning anymore. <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If it resolves an open issue, please link to the issue here. For example "Closes #1337" --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] I have made sure that the title is self-explanatory and the description concisely explains the PR. - [x] I have linked an issue or discussion. - [x] I have created tests covering the changes. ### ⌛ Dependencies <!-- List all open pull requests that this PR logically depends on --> <!-- - #xyz: short description why this is a dependency - #abc: ... --> ### Replaces: #35235 Since gh doesn't allow to change the branch on a PR. URL: #35337 Reported by: Gonzalo Tornaría Reviewer(s): François Bissey
📚 Description
ipython 8.11 throws new warnings "UserWarning: Shell was already running a gui event loop for sage; switching to sage." when running some tests
📝 Checklist
⌛ Dependencies