-
-
Notifications
You must be signed in to change notification settings - Fork 487
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
Update ipykernel to 6.13.0 - reintroduces psutil #33772
Comments
Commit: |
New commits:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:4
Well it depends on a python package named psutils because it mimics that functionality, but the python package in question do not require the C package psutil. https://github.com/giampaolo/psutil https://pypi.org/project/psutil/ So the plot twist is that we can really dump the old "psutil" package in favor of something designed to be more cross platform from the start. |
comment:5
Hm? No, it's the same Python package that was dumped in #32656 but is now needed again for ipykernel. |
comment:6
Upstream psutil still doesn't support Cygwin (as far as I can see) |
comment:7
My mistake, I always thought it was another package that was being discussed. In fact I may have had a wrong dependency for a while :( |
comment:8
Well, I know when I'm beaten. It looks like ipykernel is making the same mistakes that sage did; (a) reckless introduction of dependencies to obtain (b) meaningless memory metrics. I don't have the energy to fight it, I'm going outside. |
comment:9
After some fresh air, we can try to get a simple patch into (The last attempt to get Cygwin support into |
comment:10
I doubt We'd need a drop-in replacement on Cygwin :-( |
comment:11
IMHO less and less projects are willing to mess with Cygwin, (rightly) considering it to be a dying, obsolete, semi-broken, and dodgy. |
comment:12
I don't think that's a trend. In the past, no project even had a Cygwin test infrastructure. This has changed with the availability of GitHub Actions - in particular https://github.com/cygwin/cygwin-install-action |
comment:13
Replying to @mkoeppe:
but the upstream (Jupyter & Co) don't even try to support Cygwin. |
comment:14
I think it's OK to make ipython and jupyter optional. After all, although I'm not 100% sure, it ought to be possible to use a native Windows Jupyter to connect to the cygwin Sage jupyter kernel. |
comment:15
#30306 lists a number of tickets to try out and document such setups, but nobody has done the work |
After switching the doctests to report cputime instead of walltime, the running-time measurements for tests using pexpect interfaces are off because the time spent in subprocesses is not accounted for. Fixing this turns out not to be easy: the cputime() function in sage.misc.timing can try to account for them, but it is essentially asking the subprocess to compute its own cputime. This has faults: 1. Not every pexpect interface implemented (or can implement) such a method. 2. The pexpect interface needs to be in good working condition before we can ask the subprocess to do anything. That's often not the case at the end of a "sage: ..." line in a doctest. Particularly it's not true in all of the tests for low-level details of our pexpect interface itself. Instead, this commit parses /proc/<pid>/stat to obtain the cputime numbers for pexpext processes. This works well with one caveat: that information is only available on Linux and BSD. Having a solution that only works on some platforms is not actually too problematic. The motivation for measuring these times in the doctest framework is that someone should notice an outrageously long test and fix or report it. So, it's enough that the measurements be accurate on only some platforms -- particularly where the CI is run. Furthermore, the information is only both missing and potentially useful for long-running pexpect tests, which are relatively rare. It would be possible to implement the same thing for macOS using its own platform-specific interface if anyone is motivated to do so. It would also make sense to use psutil to obtain this information once we have upgraded to a version of ipykernel that reintroduces it as a dependency (see sagemathGH-33772). Issue: sagemathGH-32981
After switching the doctests to report cputime instead of walltime, the running-time measurements for tests using pexpect interfaces are off because the time spent in subprocesses is not accounted for. Fixing this turns out not to be easy: the cputime() function in sage.misc.timing can try to account for them, but it is essentially asking the subprocess to compute its own cputime. This has faults: 1. Not every pexpect interface implemented (or can implement) such a method. 2. The pexpect interface needs to be in good working condition before we can ask the subprocess to do anything. That's often not the case at the end of a "sage: ..." line in a doctest. Particularly it's not true in all of the tests for low-level details of our pexpect interface itself. Instead, this commit parses /proc/<pid>/stat to obtain the cputime numbers for pexpext processes. This works well with one caveat: that information is only available on Linux and BSD. Having a solution that only works on some platforms is not actually too problematic. The motivation for measuring these times in the doctest framework is that someone should notice an outrageously long test and fix or report it. So, it's enough that the measurements be accurate on only some platforms -- particularly where the CI is run. Furthermore, the information is only both missing and potentially useful for long-running pexpect tests, which are relatively rare. It would be possible to implement the same thing for macOS using its own platform-specific interface if anyone is motivated to do so. It would also make sense to use psutil to obtain this information once we have upgraded to a version of ipykernel that reintroduces it as a dependency (see sagemathGH-33772). Issue: sagemathGH-32981
After switching the doctests to report cputime instead of walltime, the running-time measurements for tests using pexpect interfaces are off because the time spent in subprocesses is not accounted for. Fixing this turns out not to be easy: the cputime() function in sage.misc.timing can try to account for them, but it is essentially asking the subprocess to compute its own cputime. This has faults: 1. Not every pexpect interface implemented (or can implement) such a method. 2. The pexpect interface needs to be in good working condition before we can ask the subprocess to do anything. That's often not the case at the end of a "sage: ..." line in a doctest. Particularly it's not true in all of the tests for low-level details of our pexpect interface itself. Instead, this commit parses /proc/<pid>/stat to obtain the cputime numbers for pexpext processes. This works well with one caveat: that information is only available on Linux and BSD. Having a solution that only works on some platforms is not actually too problematic. The motivation for measuring these times in the doctest framework is that someone should notice an outrageously long test and fix or report it. So, it's enough that the measurements be accurate on only some platforms -- particularly where the CI is run. Furthermore, the information is only both missing and potentially useful for long-running pexpect tests, which are relatively rare. It would be possible to implement the same thing for macOS using its own platform-specific interface if anyone is motivated to do so. It would also make sense to use psutil to obtain this information once we have upgraded to a version of ipykernel that reintroduces it as a dependency (see sagemathGH-33772). Issue: sagemathGH-32981
<!-- ^^^^^ 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
<!-- ^^^^^ 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
After switching the doctests to report cputime instead of walltime, the running-time measurements for tests using pexpect interfaces are off because the time spent in subprocesses is not accounted for. Fixing this turns out not to be easy: the cputime() function in sage.misc.timing can try to account for them, but it is essentially asking the subprocess to compute its own cputime. This has faults: 1. Not every pexpect interface implemented (or can implement) such a method. 2. The pexpect interface needs to be in good working condition before we can ask the subprocess to do anything. That's often not the case at the end of a "sage: ..." line in a doctest. Particularly it's not true in all of the tests for low-level details of our pexpect interface itself. Instead, this commit parses /proc/<pid>/stat to obtain the cputime numbers for pexpext processes. This works well with one caveat: that information is only available on Linux and BSD. Having a solution that only works on some platforms is not actually too problematic. The motivation for measuring these times in the doctest framework is that someone should notice an outrageously long test and fix or report it. So, it's enough that the measurements be accurate on only some platforms -- particularly where the CI is run. Furthermore, the information is only both missing and potentially useful for long-running pexpect tests, which are relatively rare. It would be possible to implement the same thing for macOS using its own platform-specific interface if anyone is motivated to do so. It would also make sense to use psutil to obtain this information once we have upgraded to a version of ipykernel that reintroduces it as a dependency (see sagemathGH-33772). Issue: sagemathGH-32981
After switching the doctests to report cputime instead of walltime, the running-time measurements for tests using pexpect interfaces are off because the time spent in subprocesses is not accounted for. Fixing this turns out not to be easy: the cputime() function in sage.misc.timing can try to account for them, but it is essentially asking the subprocess to compute its own cputime. This has faults: 1. Not every pexpect interface implemented (or can implement) such a method. 2. The pexpect interface needs to be in good working condition before we can ask the subprocess to do anything. That's often not the case at the end of a "sage: ..." line in a doctest. Particularly it's not true in all of the tests for low-level details of our pexpect interface itself. Instead, this commit parses /proc/<pid>/stat to obtain the cputime numbers for pexpext processes. This works well with one caveat: that information is only available on Linux and BSD. Having a solution that only works on some platforms is not actually too problematic. The motivation for measuring these times in the doctest framework is that someone should notice an outrageously long test and fix or report it. So, it's enough that the measurements be accurate on only some platforms -- particularly where the CI is run. Furthermore, the information is only both missing and potentially useful for long-running pexpect tests, which are relatively rare. It would be possible to implement the same thing for macOS using its own platform-specific interface if anyone is motivated to do so. It would also make sense to use psutil to obtain this information once we have upgraded to a version of ipykernel that reintroduces it as a dependency (see sagemathGH-33772). Issue: sagemathGH-32981
After switching the doctests to report cputime instead of walltime, the running-time measurements for tests using pexpect interfaces are off because the time spent in subprocesses is not accounted for. Fixing this turns out not to be easy: the cputime() function in sage.misc.timing can try to account for them, but it is essentially asking the subprocess to compute its own cputime. This has faults: 1. Not every pexpect interface implemented (or can implement) such a method. 2. The pexpect interface needs to be in good working condition before we can ask the subprocess to do anything. That's often not the case at the end of a "sage: ..." line in a doctest. Particularly it's not true in all of the tests for low-level details of our pexpect interface itself. Instead, this commit parses /proc/<pid>/stat to obtain the cputime numbers for pexpext processes. This works well with one caveat: that information is only available on Linux and BSD. Having a solution that only works on some platforms is not actually too problematic. The motivation for measuring these times in the doctest framework is that someone should notice an outrageously long test and fix or report it. So, it's enough that the measurements be accurate on only some platforms -- particularly where the CI is run. Furthermore, the information is only both missing and potentially useful for long-running pexpect tests, which are relatively rare. It would be possible to implement the same thing for macOS using its own platform-specific interface if anyone is motivated to do so. It would also make sense to use psutil to obtain this information once we have upgraded to a version of ipykernel that reintroduces it as a dependency (see sagemathGH-33772). Issue: sagemathGH-32981
ipykernel >= 6.7.0 now requires psutil - https://ipykernel.readthedocs.io/en/stable/changelog.html#id33 - removed in #32656
Previous update of ipykernel was done in #33020.
CC: @orlitzky @williamstein @tornaria
Component: packages: standard
Branch/Commit: u/mkoeppe/update_ipykernel_to_6_13_0___reintroduces_psutil @
6bc0e43
Issue created by migration from https://trac.sagemath.org/ticket/33772
The text was updated successfully, but these errors were encountered: