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

Upgrade ipython to 8.17.x #35251

Closed
wants to merge 18 commits into from
Closed

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Mar 9, 2023

📚 Description

📝 Checklist

This is the upgrade of ipython to 8.17 + upgrades of dependencies.

As the following issue is still open:

⌛ Dependencies

@tornaria
Copy link
Contributor

tornaria commented May 5, 2023

@mkoeppe see prompt-toolkit/python-prompt-toolkit#1576 (comment)

Doctest errors with new ipython are already dealt with in #35235 (ipython 8.11) and #35423 (ipython 8.12), both already merged.

I'm currently using ipython 8.13.1, no new changes needed.

Unless upstream approves my proposal, your way forward is to either (a) patch the updated prompt_toolkit and never allow the version from the system or (b) do the saving-restoring signals in sagemath around the call to prompt_toolkit (poc here: #33428 (comment)).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 5, 2023

Patching prompt_toolkit is not really a long-term option because we need it to be a pip-installable dependency of sagemath-repl.

Let's wait a little bit for a reaction from prompt-toolkit upstream, but your solution in #33428 (comment) looks promising

@tornaria
Copy link
Contributor

tornaria commented May 5, 2023

Let's wait a little bit for a reaction from prompt-toolkit upstream, but your solution in #33428 (comment) looks promising

Only caveat is that eventually we will be saving-restoring signals twice (once in the fixed prompt-toolkit and once in sagemath) but it seems a reasonable workaround.

The solution I proposed can be adjusted as well to use the python stable API instead of cysignals as in my new patch for prompt-toolkit. Maybe that's useful (I don't know if sagemath-repl will depend on cysignals).

@orlitzky
Copy link
Contributor

Let's wait a little bit for a reaction from prompt-toolkit upstream, but your solution in #33428 (comment) looks promising

The solution I proposed can be adjusted as well to use the python stable API instead of cysignals as in my new patch for prompt-toolkit. Maybe that's useful (I don't know if sagemath-repl will depend on cysignals).

I think we should give up waiting on upstream and go ahead with this. Distros are adopting the new prompt-toolkit (https://repology.org/project/python:prompt-toolkit/versions), and patching it with a fix not accepted by upstream is a hard sell. The corresponding distro sage will just suffer from the bug until something changes.

@orlitzky
Copy link
Contributor

orlitzky commented Nov 4, 2023

@tornaria is this the solution you had in mind?

diff --git a/src/sage/repl/interpreter.py b/src/sage/repl/interpreter.py
index b9a222c12ef..4ddb8aa94a3 100644
--- a/src/sage/repl/interpreter.py
+++ b/src/sage/repl/interpreter.py
@@ -153,6 +153,18 @@ from IPython.terminal.embed import InteractiveShellEmbed
 from IPython.terminal.ipapp import TerminalIPythonApp, IPAppCrashHandler
 from IPython.core.crashhandler import CrashHandler
 
+from ctypes import pythonapi, c_int, c_void_p
+# The following functions are part of the stable ABI since python 3.2
+# See: https://docs.python.org/3/c-api/sys.html#c.PyOS_getsig
+
+# PyOS_sighandler_t PyOS_getsig(int i)
+pythonapi.PyOS_getsig.restype = c_void_p
+pythonapi.PyOS_getsig.argtypes = c_int,
+
+# PyOS_sighandler_t PyOS_setsig(int i, PyOS_sighandler_t h)
+pythonapi.PyOS_setsig.restype = c_void_p
+pythonapi.PyOS_setsig.argtypes = c_int, c_void_p,
+
 
 # TODO: This global variable _do_preparse should be associated with an
 # IPython InteractiveShell as opposed to a global variable in this
@@ -287,6 +299,18 @@ class SageTerminalInteractiveShell(SageShellOverride, TerminalInteractiveShell):
         backend = BackendIPythonCommandline()
         backend.get_display_manager().switch_backend(backend, shell=self)
 
+    def prompt_for_code(self):
+        # save sigint handlers (python and os level)
+        # https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576
+        # https://github.com/sagemath/sage/issues/33428
+        # https://github.com/sagemath/sage/pull/35251
+        import signal
+        sigint = signal.getsignal(signal.SIGINT)
+        sigint_os = pythonapi.PyOS_getsig(signal.SIGINT)
+        text = TerminalInteractiveShell.prompt_for_code(self)
+        signal.signal(signal.SIGINT, sigint)
+        pythonapi.PyOS_setsig(signal.SIGINT, sigint_os)
+        return text

(It seems to work for me but I haven't had the energy to read through the documentation for everything yet.)

If so, can we add it to this PR? It'd be nice to have a modern version of ipython in 10.2.

@mkoeppe mkoeppe added this to the sage-10.3 milestone Nov 8, 2023
@mkoeppe mkoeppe changed the title Upgrade ipython to 8.11 Upgrade ipython to 8.17.x Nov 9, 2023
@dimpase
Copy link
Member

dimpase commented Nov 9, 2023

I've added the workaround in #35251 (comment) to the branch. Is there a quick way to test it?

@tornaria
Copy link
Contributor

tornaria commented Nov 9, 2023

I've added the workaround in #35251 (comment) to the branch. Is there a quick way to test it?

See #33428 for the original way to reproduce it.

There are other ways to reproduce it directly in ipython in prompt-toolkit/python-prompt-toolkit#1576 and void-linux/void-packages#35712.

@orlitzky: your diff is what I had in mind. My actual solution is to patch prompt_toolkit to default handle_sigint to False (https://github.com/void-linux/void-packages/blob/master/srcpkgs/python3-prompt_toolkit/patches/dont-handle-sigint.patch).

An alternative would be to somehow "fix" ipython to pass handle_sigint=False to prompt_toolkit.

@dimpase
Copy link
Member

dimpase commented Nov 9, 2023

OK, so forever test, as in prompt-toolkit/python-prompt-toolkit#1576, passes just fine.

sage: import forever
sage: forever.run()
^C---------------------------------------------------------------------------
KeyboardInterrupt                         Traceback (most recent call last)
Cell In[2], line 1
----> 1 forever.run()

File /mnt/opt/Sage/sage-dev/forever.pyx:4, in forever.run()
      2 
      3 def run():
----> 4     sig_on()
      5     while True:
      6         pass

KeyboardInterrupt: 
sage: 

@dimpase
Copy link
Member

dimpase commented Nov 10, 2023

it goes without saying that Sage's ipython does not behave as sage, i.e. forever.run() cannot be interrupted by Ctrl-C, as opposed to being run in Sage's CLI.

@orlitzky
Copy link
Contributor

I finally read the python docs and now understand what that code is doing. It should do the right thing.

@dimpase dimpase marked this pull request as ready for review November 10, 2023 15:44
@vbraun vbraun force-pushed the develop branch 2 times, most recently from 883e05f to e349b00 Compare November 12, 2023 16:25
@orlitzky
Copy link
Contributor

The comments in ipython/install-requires.txt and prompt_toolkit/install-requires.txt are no longer needed, otherwise LGTM. Everything is working with system packages on Gentoo.

@orlitzky
Copy link
Contributor

Feel free to re-positive-review it on my behalf if you push those two comment fixes.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2023

Also the constraint added in #36659 needs to be reverted

Copy link

Documentation preview for this PR (built with commit 563a31a; changes) is ready! 🎉

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 24, 2023

Doctest failures. Same that are fixed already in #36129.

@dimpase
Copy link
Member

dimpase commented Nov 24, 2023

what I see are

2023-11-24T17:08:48.2168346Z sage -t --random-seed=122800131133740979597912118662376657835 src/sage/interacts/test_jupyter.rst  # 2 doctests failed
2023-11-24T17:08:48.2168996Z sage -t --random-seed=122800131133740979597912118662376657835 src/sage/repl/ipython_kernel/interact.py  # 2 doctests failed
2023-11-24T17:08:48.2169517Z sage -t --random-seed=122800131133740979597912118662376657835 src/sage/repl/ipython_kernel/widgets.py  # 2 doctests failed
2023-11-24T17:08:48.2170067Z sage -t --random-seed=122800131133740979597912118662376657835 src/sage/repl/ipython_kernel/widgets_sagenb.py  # 2 doctests failed

and this is all boiling down to AttributeError: module 'ipykernel.comm.comm' has no attribute 'BaseComm'
(no idea what it is though)
anything else?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 24, 2023

So why not just test the PR that already works - #36129.

@dimpase
Copy link
Member

dimpase commented Nov 24, 2023

this PR has an important technical fix, enabling an update to the modern prompt_toolkit.

it's not logical to put it into a mega-update PR (which imho should be "remove stuff" PR)

@dimpase dimpase closed this Nov 25, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 4, 2023
    
<!-- ^^^^^
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 sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

This is now based on JupyterLab. There are many changes in dependencies.

We are trying to use platform-independent wheels when available, to
avoid any Node.JS activity during our build.
Hence we remove our `nodeenv`, `nodejs`, `jupyter_packaging`,
`hatch_nodejs_version` packages.

There is trouble on the horizon regarding our model of building
everything from source: The latest versions of `jsonschema` have
switched from `pyrsistent` to a Rust-based package.
We use the newest versions that don't pull in the build dependency on
the Rust compiler.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
- Resolves sagemath#24904
- Resolves sagemath#30399
- Resolves sagemath#33772

<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 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! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#35251 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36129
Reported by: Matthias Köppe
Reviewer(s): Eric Gourgoulhon, Kwankyu Lee, Matthias Köppe, Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 5, 2023
    
<!-- ^^^^^
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 sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

This is now based on JupyterLab. There are many changes in dependencies.

We are trying to use platform-independent wheels when available, to
avoid any Node.JS activity during our build.
Hence we remove our `nodeenv`, `nodejs`, `jupyter_packaging`,
`hatch_nodejs_version` packages.

There is trouble on the horizon regarding our model of building
everything from source: The latest versions of `jsonschema` have
switched from `pyrsistent` to a Rust-based package.
We use the newest versions that don't pull in the build dependency on
the Rust compiler.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
- Resolves sagemath#24904
- Resolves sagemath#30399
- Resolves sagemath#33772

<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 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! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#35251 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36129
Reported by: Matthias Köppe
Reviewer(s): Eric Gourgoulhon, Kwankyu Lee, Matthias Köppe, Michael Orlitzky
@tornaria
Copy link
Contributor

@tornaria is this the solution you had in mind?

diff --git a/src/sage/repl/interpreter.py b/src/sage/repl/interpreter.py
index b9a222c12ef..4ddb8aa94a3 100644
--- a/src/sage/repl/interpreter.py
+++ b/src/sage/repl/interpreter.py
...

(It seems to work for me but I haven't had the energy to read through the documentation for everything yet.)

If so, can we add it to this PR? It'd be nice to have a modern version of ipython in 10.2.

Just as a heads up: prompt_toolkit 3.0.42 was released today, and it fixes the issue for good. This means the workaround above will not be necessary as long as prompt_toolkit is >= 3.0.42.

@orlitzky @mkoeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 12, 2023

Excellent!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants