Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#21441 closed defect (fixed)

Old installed version of Cython is used

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-7.4
Component: build Keywords:
Cc: embray Merged in:
Authors: Erik Bray, Jeroen Demeyer Reviewers: Jeroen Demeyer, Erik Bray, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 8fd8273 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

On a machine which had an older version of Cython installed but which was upgraded recently after #20218 was merged:

$ ls -ld local/lib/python/site-packages/Cython*
drwxrwxr-x 12 jdemeyer jdemeyer 4096 Sep  6 21:23 local/lib/python/site-packages/Cython
drwxrwxr-x  5 jdemeyer jdemeyer 4096 Sep  6 21:22 local/lib/python/site-packages/Cython-0.23.3-py2.7-linux-i686.egg
drwxrwxr-x  2 jdemeyer jdemeyer 4096 Sep  6 21:14 local/lib/python/site-packages/Cython-0.24.1-py2.7.egg-info

Despite Cython-0.24.1 being available, the older version 0.23.3 is still used by default. This causes breakage when building the Sage library.

>>> import Cython
>>> Cython.__version__
'0.23.3'
>>> import sys
>>> [x for x in sys.path if 'Cython' in x]
['/home/jdemeyer/sage-git/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-i686.egg']

The problem is not limited to Cython, the same happens for other packages too.

This is a blocker ticket since it breaks upgrades from older Sage versions.

Steps to reproduce:

  1. ./sage --sh -c 'easy_install cython==0.23.3' # Install an older version of Cython.
  1. ./sage -f cython # Rebuild the Cython in Sage. This should uninstall the old version but it doesn't.

Note to the release manager: ideally, this would be merged together with #21552 since both tickets trigger recompilation of all Python packages.

Change History (60)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Old version of Cython might be used to Old installed version of Cython is used

comment:2 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 4 years ago by embray

Wow, I see the issue. When you use pip with the --ignore-installed option it also does not uninstall existing versions of the package (at the very least if it's an old .egg). I would say that's undocumented behavior.

comment:6 follow-up: Changed 4 years ago by embray

I think the easiest thing to do for now would be to make PIP_INSTALL point to a little wrapper function that also forcibly calls pip uninstall. That will also make ./sage -f work properly since it will force reinstall of the package even if the same version was already installed.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 4 years ago by jdemeyer

  • Description modified (diff)

Replying to embray:

I think the easiest thing to do for now would be to make PIP_INSTALL point to a little wrapper function that also forcibly calls pip uninstall.

That is not entirely trivial because you need to figure out which package to uninstall. You cannot do pip uninstall ., you need something like pip uninstall $PKGNAME.

Other than that, I like the idea. We could probably drop the --ignore-installed --no-deps options too.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by embray

Replying to jdemeyer:

Replying to embray:

I think the easiest thing to do for now would be to make PIP_INSTALL point to a little wrapper function that also forcibly calls pip uninstall.

That is not entirely trivial because you need to figure out which package to uninstall. You cannot do pip uninstall ., you need something like pip uninstall $PKGNAME.

Fair point. A broader idea I had is that as many of the spkg-installs (but not all!) have sections for uninstalling previous installations, that step should maybe be broken out to a separate spkg-uninstall script. Another step in making the packages a little more uniform in how they are managed. But my hope was to find a simple way to address this specific issue now.

I'm sort of of the mind that pip uninstall . should work. If there's a local setup.py it should use that to figure out the package's name, and then uninstall that.

Other than that, I like the idea. We could probably drop the --ignore-installed --no-deps options too.

I don't know what you mean here. If --ignore-installed is dropped then it's a moot point. pip will uninstall the previous version of the package without that flag. It seems that --ignore-installed is just very literal in its intent. It really means "pay no attention whatsoever to what I already have installed".

comment:9 Changed 4 years ago by embray

I think what pip really needs here is an option to explicitly ensure that conflicting packages are uninstalled, even if I said --ignore-installed.

comment:10 follow-up: Changed 4 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/sage-pip-install
  • Commit set to 015197a071c29cdd158581f230d11668f63a6e00

Here's an initial stab at a fix. Adds a new sage-pip-install script to wrap pip, since otherwise this got too unwieldy. It just ensures that any previous installations of the package are thoroughly uninstalled before the actual pip install call.

In the process I found out that Cython's setup.py is badly behaved--./setup.py --name should really only print the package's name (if it prints anything else it should be to stderr). This is easy enough to work around, but I wonder what other packages make this mistake, and I'm testing for that now...


New commits:

015197aAdd a sage-pip-install wrapper for pip which handles uninstallation better.

comment:11 Changed 4 years ago by git

  • Commit changed from 015197a071c29cdd158581f230d11668f63a6e00 to ea52ac9527aebf6ac967b6aa6cf56aed18b733d8

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

ea52ac9Make sure to consume additional flags passed to the PIP_INSTALL command

comment:12 Changed 4 years ago by git

  • Commit changed from ea52ac9527aebf6ac967b6aa6cf56aed18b733d8 to a6e053839fe27837626791d65efd87c9bcf55e14

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

a6e0538Ensure stderr is captured for repeat uninstalls

comment:13 follow-up: Changed 4 years ago by jdemeyer

#!/bin/bash should be #!/usr/bin/env bash

comment:14 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by jdemeyer

Replying to embray:

I don't know what you mean here. If --ignore-installed is dropped then it's a moot point. pip will uninstall the previous version of the package without that flag.

The whole point of adding --ignored-installed in #20218 was to fix cases of existing but broken installations. Those would now be uninstalled anyway, so you no longer need the --ignore-installed flag. And once you do that, you can drop --no-deps to enable dependency checking.

comment:15 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by jdemeyer

Replying to embray:

In the process I found out that Cython's setup.py is badly behaved--./setup.py --name should really only print the package's name

Is Cython the only misbehaving package? If so, I would rather add https://github.com/cython/cython/pull/1455 in order to simplify sage-pip-install.

comment:16 in reply to: ↑ 13 Changed 4 years ago by embray

Replying to jdemeyer:

#!/bin/bash should be #!/usr/bin/env bash

Sure, will do.

comment:17 in reply to: ↑ 14 Changed 4 years ago by embray

Replying to jdemeyer:

Replying to embray:

I don't know what you mean here. If --ignore-installed is dropped then it's a moot point. pip will uninstall the previous version of the package without that flag.

The whole point of adding --ignored-installed in #20218 was to fix cases of existing but broken installations. Those would now be uninstalled anyway, so you no longer need the --ignore-installed flag. And once you do that, you can drop --no-deps to enable dependency checking.

That makes sense. I think for the purposes of this ticket we shouldn't mess with it though--it's already working as is (minus this issue). I'd rather get the uninstallation fixed, and then worry about further tweaking the install options.

I'm also going to write up something about this experience for the pip mailing list. As pip is now the de-facto installer for Python packages it ought to be easier for system integrators to control it a bit (though I still maintain that as system integrators dependency management is our job, not pip's).

comment:18 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by embray

Replying to jdemeyer:

Replying to embray:

In the process I found out that Cython's setup.py is badly behaved--./setup.py --name should really only print the package's name

Is Cython the only misbehaving package? If so, I would rather add https://github.com/cython/cython/pull/1455 in order to simplify sage-pip-install.

There are a few others I've found:

  • pari_jupyter
  • widgetsnbextension
  • giacpy
  • matplotlib

I've already opened an issue for matplotlib, though I don't have a patch. I think the check is good to leave in even if these are fixed because you never know when another package is going to be added, or updated in such a way that it breaks this.

comment:19 Changed 4 years ago by jdemeyer

I see. For pari_jupyter, I need Cython 0.25 to fix that: https://github.com/jdemeyer/pari_jupyter/pull/1

comment:20 Changed 4 years ago by jdemeyer

Erik, I agree with everything you said. Remember to fix 13 and set this to needs review when you feel like it is ready.

comment:21 follow-up: Changed 4 years ago by jdemeyer

Important: we should also bump the version number of all involved packages to force reinstalling them. Alternatively, we could also just bump the Python version. That will also force a re-installation of all Python packages in Sage.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:22 in reply to: ↑ 21 ; follow-up: Changed 4 years ago by embray

Replying to jdemeyer:

Important: we should also bump the version number of all involved packages to force reinstalling them. Alternatively, we could also just bump the Python version. That will also force a re-installation of all Python packages in Sage.

Doesn't that mean basically any package that uses pip? I avoided doing that in #20218 since there was no real change in the packages themselves. How they are installed is changed, but it shouldn't have otherwise impacted their functionality.

I'm not opposed to doing it anyways, but I'm not sure it's really needed either.

comment:23 Changed 4 years ago by git

  • Commit changed from a6e053839fe27837626791d65efd87c9bcf55e14 to 2ee51db6b2f2e841b052cb6216b711deba51101b

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

2ee51dbChange the shebang line to user /usr/bin/env

comment:24 in reply to: ↑ 22 Changed 4 years ago by jdemeyer

Replying to embray:

Doesn't that mean basically any package that uses pip?

Yes indeed.

I avoided doing that in #20218 since there was no real change in the packages themselves.

The intent was to have no real change. But this whole ticket is about an accidental change.

How they are installed is changed, but it shouldn't have otherwise impacted their functionality.

Given that the packages were installed badly, the change does impact the functionality.

Another example of breakage is the patchbot report of #20767: it reports a failure because the Cython update #21126 didn't work. If you don't force a rebuild, it will keep using the wrong version.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:25 follow-up: Changed 4 years ago by embray

But that was only an issue specifically because Cython was updated. The version number of Cython was bumped, at it was reinstalled. It just didn't uninstall the old version. Neither the old version nor the new version were "installed badly" on their own--only insofar as two copies were installed.

If a package wasn't otherwise updated there's no reason to reinstall it.

comment:26 in reply to: ↑ 25 Changed 4 years ago by jdemeyer

Replying to embray:

But that was only an issue specifically because Cython was updated.

...in the latest beta. If you assume that people upgrade at every beta release, then you might be right. But people might have skipped some beta releases (upgrading from an older version to the current version) and then more packages might be affected. I suggest we don't guess and just upgrade all pip-installed packages.

Neither the old version nor the new version were "installed badly" on their own--only insofar as two copies were installed.

Right, it wasn't "installed badly" in the strict sense. But still, the packages are installed badly in the sense that if you try to import them, you get the wrong version.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:27 Changed 4 years ago by jdemeyer

Anyway, this is not something I want to fight about. My main concern is that this bug gets fixed :-)

comment:28 Changed 4 years ago by jdemeyer

I think the pushd/popd logic is not needed since this script will be run in a separate process. A simple cd should work.

comment:29 Changed 4 years ago by jdemeyer

Erik, do you plan to work on this or should I take over?

comment:30 in reply to: ↑ 18 Changed 4 years ago by jdemeyer

Replying to embray:

There are a few others I've found:

  • widgetsnbextension

For widgetsnbextension, see https://github.com/ipython/ipywidgets/pull/781

comment:31 Changed 4 years ago by jdemeyer

  • Authors changed from Erik Bray to Erik Bray, Jeroen Demeyer

comment:32 Changed 4 years ago by jdemeyer

  • Branch changed from u/embray/sage-pip-install to u/jdemeyer/sage-pip-install

comment:33 Changed 4 years ago by jdemeyer

  • Commit changed from 2ee51db6b2f2e841b052cb6216b711deba51101b to 92ae7d7e63ca5ad97603d208de351bef7a2085e3
  • Status changed from new to needs_review

New commits:

ccd9442Add a sage-pip-install wrapper for pip which handles uninstallation better.
3b61291Directly call sage-pip-install instead of using $PIP_INSTALL
f4ca23aRe-install all pip-installed packages
92ae7d7Minor fixes to sage-pip-install

comment:34 Changed 4 years ago by git

  • Commit changed from 92ae7d7e63ca5ad97603d208de351bef7a2085e3 to 8afdf212e25c9fd815d306ad89357f86309933aa

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

8afdf21Fix various dependencies

comment:35 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:36 Changed 4 years ago by embray

I think the mass replacement of PIP_INSTALL with sage-pip-install was excessive. I deliberately did not do that in the first place, because

a) I wanted to avoid another mass update of spkg-installs when that was already done to put PIP_INSTALL everywhere, and

b) Leaving PIP_INSTALL as a variable means such mass updates can be avoided in the future. For example, future improvements in pip itself might render the sage-pip-install script superfluous, at which point it might be nice to remove it.

I also have ideas for more work to generalize common installation steps so that maybe many of these spkg-installs will just be pointers to a common install script for Python packages, so that will reduce duplication as well, at which point PIP_INSTALL might not be as useful. But for now I think it's easier to leave it so that this can be changed in one place.

comment:37 Changed 4 years ago by embray

(Also, sorry, I was on vacation last week. No disagreement with anything else here.)

comment:38 Changed 4 years ago by jdemeyer

To be honest, I never liked the PIP_INSTALL the environment variable in the first place. And if that variable would just be the name of a script, why bother with the environment variable? And mass replacements are really not a big deal, that's trivially done with some sed script or other tool.

However, for the sake of peace, I don't mind to change this back if that's the only issue you have.

comment:39 Changed 4 years ago by jdemeyer

@embray: please clearly state what you require to set this to positive_review.

comment:40 follow-up: Changed 4 years ago by embray

Just removing the change to PIP_INSTALL.

I don't like adding yet another "sage-" script and would prefer to remove it sooner rather than later. But for now it's the easiest solution.

comment:41 Changed 4 years ago by git

  • Commit changed from 8afdf212e25c9fd815d306ad89357f86309933aa to e351c98d94505343082bc16d9397a87f7058eef5

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

4d4d72cVarious fixes and simplifications to sage-pip-install
2b28dd9Re-install all pip-installed packages
e351c98Fix various dependencies

comment:42 in reply to: ↑ 40 Changed 4 years ago by jdemeyer

Replying to embray:

I don't like adding yet another "sage-" script and would prefer to remove it sooner rather than later. But for now it's the easiest solution.

I would argue that adding a new environment variable in sage-env is uglier than a clean self-contained script. But here is what you asked for...

comment:43 follow-up: Changed 4 years ago by embray

I wouldn't have removed the . either. sage-pip-install takes that as an argument. The idea was to keep the interface otherwise the same, and minimize the changes needed to work around this issue, which I think represents deficiencies in pip more than anything else.

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

comment:44 in reply to: ↑ 43 Changed 4 years ago by jdemeyer

Replying to embray:

I wouldn't have removed the . either.

I know. However, Sage only uses . as directory, so I didn't see the point of supporting a directory argument (it's extra complexity without a use case). We could also keep the . and just ignore it in sage-pip-install, but that looked like a worse solution.

comment:45 Changed 4 years ago by embray

(I do agree having it as an environment variable sucks--in the future we'll want to redo how spkg-install works so that it instead sources what it needs instead of relying as much on environment variables).

comment:46 follow-up: Changed 4 years ago by embray

  • Status changed from needs_review to needs_work

See above.

comment:47 in reply to: ↑ 46 Changed 4 years ago by jdemeyer

Replying to embray:

See above.

Can you give a little more detail?

comment:48 Changed 4 years ago by jdemeyer

In other words: what would you like me to do concretely?

comment:49 Changed 4 years ago by embray

Remove any changes to spkg-installs.

comment:50 Changed 4 years ago by git

  • Commit changed from e351c98d94505343082bc16d9397a87f7058eef5 to 8fd8273a000d17fba9fde19311aaf7adf6052fcb

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

29957faVarious fixes and simplifications to sage-pip-install
783cd7dRe-install all pip-installed packages
8fd8273Fix various dependencies

comment:51 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Not fully tested yet, but this reverts all changes to spkg-install, except for the pip package.

comment:52 Changed 4 years ago by embray

  • Status changed from needs_review to positive_review

Looks good to me.

comment:53 Changed 4 years ago by vbraun

Reviewer name...

comment:54 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:55 follow-up: Changed 4 years ago by mkoeppe

The patch bumps the version numbers of dozens of packages without making changes to them. This seems unnecessary.

comment:56 Changed 4 years ago by mkoeppe

  • Reviewers set to Jeroen Demeyer, Erik Bray, Matthias Koeppe

comment:57 in reply to: ↑ 55 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to positive_review

Replying to mkoeppe:

The patch bumps the version numbers of dozens of packages without making changes to them.

This patch does make implicit changes to all those spkg-install scripts because it changes $PIP_INSTALL. See 21 and following.

In any case, if this is merged with #21552, then all Python packages need to be rebuilt anyway so it wouldn't matter.

comment:58 Changed 4 years ago by embray

I don't personally think it's terribly important to bump all these package versions since it doesn't actually change all of them--for example packages that did not use setuptools (and thus are not installed as .egg directories) really aren't changed at all. For those that are changed, this changes some small details about how they are installed, but nothing about how they run.

That said, strictly speaking, Jeroen is right that this does change runtime behavior since it will change what sys.path looks like. Also nice to bump the version numbers to force all the .egg installs to be removed.

comment:59 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/sage-pip-install to 8fd8273a000d17fba9fde19311aaf7adf6052fcb
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:60 Changed 4 years ago by mkoeppe

  • Commit 8fd8273a000d17fba9fde19311aaf7adf6052fcb deleted

OK, thanks for the explanation.

Note: See TracTickets for help on using tickets.