Opened 3 years ago

Closed 2 months ago

#26253 closed enhancement (invalid)

Upgrade: psutil 5.8.0

Reported by: dimpase Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: packages: standard Keywords: upgrade, psutil
Cc: embray, slelievre, mjo, mkoeppe, fbissey, arojas, isuruf Merged in:
Authors: Reviewers: Michael Orlitzky
Report Upstream: N/A Work issues: Cygwin
Branch: public/ticket/26253 (Commits, GitHub, GitLab) Commit: 0fc3c304287e728be0700c526b7dde4cd847c4e9
Dependencies: Stopgaps:

Status badges

Description (last modified by slelievre)

Cygwin support needs a rebase.

Change History (52)

comment:1 Changed 3 years ago by dimpase

The cygwin-specific patch does not apply, needs a rebase. It's removed for the sake of being able to test on other system (needed on FreeBSD, older package does not work there).

Last edited 3 years ago by dimpase (previous) (diff)

comment:2 Changed 3 years ago by dimpase

  • Branch set to u/dimpase/psutils547
  • Cc embray added
  • Commit set to eee119432df73a72e802f92f6518408c21e42f6d
  • Description modified (diff)

New commits:

eee1194updated and removed cygwin patch (needs a rebase)

comment:3 Changed 3 years ago by dimpase

  • Summary changed from update psutil to version 5.4.7 to update psutil to version 5.4.8

meanwhile 5.4.8 is out.

comment:4 Changed 3 years ago by dimpase

  • Branch u/dimpase/psutils547 deleted
  • Commit eee119432df73a72e802f92f6518408c21e42f6d deleted
  • Description modified (diff)

comment:5 Changed 3 years ago by embray

  • Milestone changed from sage-8.4 to sage-pending

I keep starting and stopping on reworking my Cygwin stuff for psutil. It needs to be refactored pretty badly and every time I start to work on it I quickly get pulled away to other things.

Is there some specific reason to update the psutil version?

comment:6 Changed 3 years ago by dimpase

One reason is that the current version does not work on FreeBSD 12.0. How hard is that refactoring? I have a Windows laptop around, which BSODs now and then due to some hardware issue, but probably is good enough for mild Cygwin work.

A related Cygwin issue is on #26052 - any idea about that?

comment:7 Changed 3 years ago by embray

It's very non-trivial, unfortunately, and requires upstream cooperation (where in this case the psutil maintainer has been helpful and amenable, but that doesn't mean I can just go and do what I want).

Last edited 3 years ago by embray (previous) (diff)

comment:8 Changed 3 years ago by dimpase

  • Description modified (diff)
  • Summary changed from update psutil to version 5.4.8 to update psutil to version 5.5.0

meanwhile psutils 5.5.0 is out

comment:9 Changed 3 years ago by chapoton

  • Keywords upgrade added

comment:10 Changed 3 years ago by embray

I'm working on a new (simpler) version of the Cygwin patch for psutil. Hopefully it will be ready soon.

comment:11 Changed 3 years ago by dimpase

the latest psutil is 5.6.2.

comment:12 Changed 3 years ago by fbissey

It would be good to update. Distros likely use a more recent psutil (I do). It causes at least one optional doctest failure (when combined with fricas) due to changes to output

sage -t --long /usr/lib64/python2.7/site-packages/sage/interfaces/fricas.py
**********************************************************************
File "/usr/lib64/python2.7/site-packages/sage/interfaces/fricas.py", line 351, in sage.interfaces.fricas.FriCAS._quit_string
Failed example:
    p = fricas.pid(); pr = psutil.Process(p); pr                  # optional - fricas
Expected:
    <psutil.Process(pid=..., name='AXIOMsys') at ...>
Got:
    psutil.Process(pid=7277, name='AXIOMsys', started='19:52:17')
**********************************************************************

This is with psutil-5.5.0.

comment:13 Changed 3 years ago by chapoton

  • Branch set to public/ticket/26253
  • Commit set to 4d7ce5119935ea51a8316f8d477c59b488a7b78f
  • Description modified (diff)

Here is a branch, not tested. I have not removed the patches.


New commits:

4d7ce51update psutil to 5.6.2

comment:14 Changed 3 years ago by git

  • Commit changed from 4d7ce5119935ea51a8316f8d477c59b488a7b78f to 291ca8643de692d63aede3107104a9257b523efc

Branch pushed to git repo; I updated commit sha1. New commits:

291ca86fix psutil fricas doctest

comment:15 Changed 21 months ago by slelievre

New release (2020-02): psutil 5.7.0.

comment:16 Changed 19 months ago by git

  • Commit changed from 291ca8643de692d63aede3107104a9257b523efc to 844fa71cce390793267dbfb37af19d260ea683cc

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4c042dcupdate psutil to 5.6.2
891175cfix psutil fricas doctest
844fa71build/pkgs/psutil: Update to 5.7.0

comment:17 Changed 19 months ago by mkoeppe

The psutil upstream issue on cygwin support is still open: https://github.com/giampaolo/psutil/issues/82

Erik, your help is needed

comment:18 Changed 19 months ago by mkoeppe

  • Summary changed from update psutil to version 5.5.0 to update psutil to version 5.7.0
  • Work issues set to Cygwin!!

comment:19 Changed 15 months ago by mkoeppe

  • Milestone changed from sage-pending to sage-9.3

comment:20 Changed 15 months ago by dimpase

perhaps we should look into removing psutils (on cygwin, or perhaps totally)

comment:21 Changed 15 months ago by embray

I have been thinking lately of spending a couple days updating the cygwin version of psutils. In particular, I think I will start maintaining my own fork of psutils with cygwin support, since last time I tried to upstream these changes it did not go well. If the psutil developer wants to port over any work from my fork he'd still be free to.

I was thinking about this lately because there was yet another issue on the psutils github asking for cygwin support and pointing to my incomplete pull requests...

comment:22 Changed 15 months ago by embray

If nothing else, I'll try rebasing my existing patches again...

comment:23 Changed 14 months ago by embray

I've rebased the latest version of my Cygwin patch onto psutil master. Once we update this ticket and decide which psutil version to upgrade to I can update the patch as well.

I might also try submitting this upstream again. The patch is incomplete (many features are missing) but good enough for Sage's purposes for now.

comment:24 Changed 14 months ago by dimpase

  • Description modified (diff)

5.7.3 is close to master, so hopefully one can apply your patches on top of 5.7.3

comment:25 Changed 14 months ago by git

  • Commit changed from 844fa71cce390793267dbfb37af19d260ea683cc to bb604c64c65a02e199cf43e954f365cba8157aed

Branch pushed to git repo; I updated commit sha1. New commits:

bb604c6update to 5.7.3

comment:26 Changed 14 months ago by git

  • Commit changed from bb604c64c65a02e199cf43e954f365cba8157aed to ec6c4a6a602f07b19c5fa4bdb336886fc9c13182

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3e84ad2update psutil to 5.6.2
79624d6build/pkgs/psutil: Update to 5.7.0
ec6c4a6update to 5.7.3

comment:27 Changed 14 months ago by git

  • Commit changed from ec6c4a6a602f07b19c5fa4bdb336886fc9c13182 to 46ba2ee06666eb7504e44dfb8e9b036c415442f0

Branch pushed to git repo; I updated commit sha1. New commits:

46ba2eeremove old Cygwin-specific patch

comment:28 Changed 14 months ago by dimpase

wrong comment - confused by the timeline, which I didn't update. It's fine.

Last edited 14 months ago by dimpase (previous) (diff)

comment:29 Changed 14 months ago by dimpase

  • Status changed from new to needs_review

this should be tested everywhere, except Cygwin, where Erik has promised a patch.

comment:30 Changed 14 months ago by dimpase

  • Summary changed from update psutil to version 5.7.0 to update psutil to version 5.7.3

comment:31 Changed 14 months ago by embray

I can generate a patch from my latest Cygwin branch at https://github.com/embray/psutil/tree/cygwin/v3

But longer term if we want psutil to support Cygwin someone other than me will have to try to talk sense into its maintainer, who apparently does not have time to work on the project, but also says that he and only he can work on the Cygwin port which I've already spent days bringing to around 90% completion, with frankly a pretty clean patch not requiring a lot of changes. Seems pretty unreasonable, and disappointing.

I continued to work on my branch and he blocked me from the project bc I unthinkingly used his @ in a commit message (perhaps in unfounded optimism that he'd be willing to give the work another look later): https://github.com/giampaolo/psutil/pull/1884#issuecomment-733165286

comment:32 Changed 14 months ago by dimpase

one cannot even open issues on the main psutil repo, I don't think there is any hope that patches would be accepted there.

it's not the 1st case we have where our patches are not welcome, I would not be too upset about it (cf. e.g. csdp, cliquer...)

comment:33 follow-up: Changed 14 months ago by mkoeppe

We should not maintain patches that add functionality in Sage. This will create problems later when switch to using standard Python package dependency information for Python packages.

Instead I see two options:

  • Fork psutil, making it available on PyPI under a new name. Only makes sense if you are willing to maintain it, obviously.
  • Just implement Cygwin version of the few things needed from psutil in src/sage/interfaces/gap.py and src/sage/misc/getusage.py

comment:34 in reply to: ↑ 33 Changed 14 months ago by embray

Replying to mkoeppe:

We should not maintain patches that add functionality in Sage. This will create problems later when switch to using standard Python package dependency information for Python packages.

Normally I would agree but that's not really the case here. We're not talking about adding new functionality or bug fixes required for Sage to function in such a way that would be at odds with using system packages.

This is just adding support to the package for a platform it doesn't otherwise support: Which is to say, there is no "standard Python package dependency information" for psutil on Cygwin. In fact, PyPI does not even support cygwin wheels yet (something I plan to lobby on, but it's slightly complicated). There is also no psutil package for Cygwin, again because support for it does not even exist modulo my patch.

Instead I see two options:

  • Fork psutil, making it available on PyPI under a new name. Only makes sense if you are willing to maintain it, obviously.
  • Just implement Cygwin version of the few things needed from psutil in src/sage/interfaces/gap.py and src/sage/misc/getusage.py

What I plan to do is, once my Cygwin port of psutil reaches 100% feature parity then I will add a Cygwin package for it to cygwinports based on my fork. Then there will at least be a standard Cygwin package for psutil and we can drop the patch and/or use my fork as the standard installation source for psutil on all platforms.

I don't think it will require a lot of maintenance since he says he's planning on taking a break from making new psutil releases for a while.

comment:35 Changed 11 months ago by git

  • Commit changed from 46ba2ee06666eb7504e44dfb8e9b036c415442f0 to 0fc3c304287e728be0700c526b7dde4cd847c4e9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6417214update psutil to 5.6.2
3c9ff5dbuild/pkgs/psutil: Update to 5.7.0
b5b584bupdate to 5.7.3
2c2e6dfremove old Cygwin-specific patch
0fc3c30psutil 5.8.0

comment:36 Changed 11 months ago by dimpase

  • Description modified (diff)
  • Summary changed from update psutil to version 5.7.3 to update psutil to version 5.8.0

comment:37 Changed 11 months ago by mkoeppe

Is there any news on Cygwin support?

comment:38 Changed 11 months ago by embray

I don't know. It would be great if someone would try to talk sense to its maintainer at https://github.com/giampaolo/psutil/pull/1884#issuecomment-733165286.

Otherwise we can still just generate a patch from my existing work in the meantime...

comment:39 Changed 11 months ago by dimpase

It sounds as if psutil is ripe for a fork :-)

comment:40 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

comment:41 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

Setting a new milestone for this ticket based on a cursory review.

comment:42 Changed 5 months ago by slelievre

  • Cc slelievre added
  • Description modified (diff)
  • Keywords psutil added
  • Summary changed from update psutil to version 5.8.0 to Upgrade: psutil 5.8.0
  • Work issues changed from Cygwin!! to Cygwin

Currently our spkg is at psutil 5.2.0, from 2017-03 (with patches).

Python 3 deprecation warnings in psutil were resolved in psutil itself by

With a recent psutil, the first start of Sage on Cygwin could be warnings-free.

Some user reports mentioning the deprecation warnings:

Last edited 5 months ago by slelievre (previous) (diff)

comment:43 Changed 5 months ago by dimpase

psutil 5.8 does not support Cygwin, as far as I know.

Last edited 5 months ago by dimpase (previous) (diff)

comment:44 Changed 4 months ago by mjo

  • Cc mjo mkoeppe fbissey arojas isuruf added

Do we need psutil at all? I think we can delete it. It's used for three things:

  1. To guess at the initial GAP memory pool size, passed to gap via -o and -s. I don't think we need these flags, though: https://www.gap-system.org/Manuals/doc/ref/chap3.html
  2. To set the maximum stack size during PARI initialization. Again, we can just set it to a big number; it's a limit, and we're already guessing.
  3. Two doctests that call get_memory_usage() to check for leaks.

Those two doctests are not worth keeping it around, even as an optional package, considering it won't work on one of our supported platforms. So I say we,

  1. Drop the -o and -s flags to GAP. If the absence of -s causes problems in some cases, we can always pass -s <big_number> since that memory should not actually be allocated. And if that assumption is violated on some platform, then we could special-case that platform.
  2. Pass a big value to PARI in libs/pari/__init__.py for its stack size limit. 1024 times the initial size, or something like that.
  3. Delete those two doctests.
  4. Delete psutil, and sleep easier knowing that we won't be responsible for a fork of it for the rest of our lives.

comment:45 Changed 4 months ago by dimpase

+1 to removing psutils

comment:46 Changed 4 months ago by mkoeppe

+1 if it can be done without breaking stuff

comment:47 Changed 4 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:48 Changed 4 months ago by slelievre

  • Status changed from needs_work to needs_info

Consider setting to invalid/wontfix after removal of psutil in #32656.

comment:49 Changed 3 months ago by dimpase

  • Milestone changed from sage-9.5 to sage-wishlist

comment:50 Changed 3 months ago by slelievre

  • Authors Dima Pasechnik deleted
  • Milestone changed from sage-wishlist to sage-duplicate/invalid/wontfix
  • Status changed from needs_info to needs_review

Ticket #32656 was merged in Sage 9.5.beta5.

comment:51 Changed 2 months ago by mjo

  • Reviewers set to Michael Orlitzky
  • Status changed from needs_review to positive_review

comment:52 Changed 2 months ago by mkoeppe

  • Resolution set to invalid
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.