Old installed version of Cython is used

[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.

2. ./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.

[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.

[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.

[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.

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

[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".

[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.

[embray]

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:

​015197a

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

[git]

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

​ea52ac9

Make sure to consume additional flags passed to the PIP_INSTALL command

[git]

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

​a6e0538

Ensure stderr is captured for repeat uninstalls

[jdemeyer]

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

[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.

[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.

[embray]

Replying to jdemeyer:

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

Sure, will do.

[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).

[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.

[jdemeyer]

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

[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.

[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.

[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.

[git]

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

​2ee51db

Change the shebang line to user /usr/bin/env

[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.

[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.

[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.

[jdemeyer]

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

[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.

[jdemeyer]

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

[jdemeyer]

Replying to embray:

There are a few others I've found:

• widgetsnbextension

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

[jdemeyer]

New commits:

​ccd9442	Add a sage-pip-install wrapper for pip which handles uninstallation better.
​3b61291	Directly call sage-pip-install instead of using $PIP_INSTALL
​f4ca23a	Re-install all pip-installed packages
​92ae7d7	Minor fixes to sage-pip-install

[git]

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

​8afdf21

Fix various dependencies

[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.

[embray]

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

[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.

[jdemeyer]

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

[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.

[git]

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

​4d4d72c	Various fixes and simplifications to sage-pip-install
​2b28dd9	Re-install all pip-installed packages
​e351c98	Fix various dependencies

[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...

[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.

[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.

[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).

[embray]

See above.

[jdemeyer]

Replying to embray:

See above.

Can you give a little more detail?

[jdemeyer]

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

[embray]

Remove any changes to spkg-installs.

[git]

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

​29957fa	Various fixes and simplifications to sage-pip-install
​783cd7d	Re-install all pip-installed packages
​8fd8273	Fix various dependencies

[jdemeyer]

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

[embray]

Looks good to me.

[vbraun]

Reviewer name...

[mkoeppe]

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

[jdemeyer]

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.

[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.

[mkoeppe]

OK, thanks for the explanation.