#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, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
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:
./sage --sh -c 'easy_install cython==0.23.3'
# Install an older version of Cython.
./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 6 years ago by
Description: | modified (diff) |
---|---|
Summary: | Old version of Cython might be used → Old installed version of Cython is used |
comment:2 Changed 6 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 6 years ago by
Description: | modified (diff) |
---|
comment:4 Changed 6 years ago by
Description: | modified (diff) |
---|
comment:5 Changed 6 years ago by
comment:6 follow-up: 7 Changed 6 years ago by
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 follow-up: 8 Changed 6 years ago by
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 callspip 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 follow-up: 14 Changed 6 years ago by
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 callspip uninstall
.That is not entirely trivial because you need to figure out which package to uninstall. You cannot do
pip uninstall .
, you need something likepip uninstall $PKGNAME
.
Fair point. A broader idea I had is that as many of the spkg-install
s (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 6 years ago by
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: 15 Changed 6 years ago by
Authors: | → Erik Bray |
---|---|
Branch: | → u/embray/sage-pip-install |
Commit: | → 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:
015197a | Add a sage-pip-install wrapper for pip which handles uninstallation better.
|
comment:11 Changed 6 years ago by
Commit: | 015197a071c29cdd158581f230d11668f63a6e00 → ea52ac9527aebf6ac967b6aa6cf56aed18b733d8 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
ea52ac9 | Make sure to consume additional flags passed to the PIP_INSTALL command
|
comment:12 Changed 6 years ago by
Commit: | ea52ac9527aebf6ac967b6aa6cf56aed18b733d8 → a6e053839fe27837626791d65efd87c9bcf55e14 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
a6e0538 | Ensure stderr is captured for repeat uninstalls
|
comment:14 follow-up: 17 Changed 6 years ago by
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 follow-up: 18 Changed 6 years ago by
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 Changed 6 years ago by
comment:17 Changed 6 years ago by
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 follow-up: 30 Changed 6 years ago by
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 nameIs 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 6 years ago by
I see. For pari_jupyter
, I need Cython 0.25 to fix that: https://github.com/jdemeyer/pari_jupyter/pull/1
comment:20 Changed 6 years ago by
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: 22 Changed 6 years ago by
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.
comment:22 follow-up: 24 Changed 6 years ago by
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 6 years ago by
Commit: | a6e053839fe27837626791d65efd87c9bcf55e14 → 2ee51db6b2f2e841b052cb6216b711deba51101b |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
2ee51db | Change the shebang line to user /usr/bin/env
|
comment:24 Changed 6 years ago by
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.
comment:25 follow-up: 26 Changed 6 years ago by
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 Changed 6 years ago by
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.
comment:27 Changed 6 years ago by
Anyway, this is not something I want to fight about. My main concern is that this bug gets fixed :-)
comment:28 Changed 6 years ago by
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:30 Changed 6 years ago by
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 6 years ago by
Authors: | Erik Bray → Erik Bray, Jeroen Demeyer |
---|
comment:32 Changed 6 years ago by
Branch: | u/embray/sage-pip-install → u/jdemeyer/sage-pip-install |
---|
comment:33 Changed 6 years ago by
Commit: | 2ee51db6b2f2e841b052cb6216b711deba51101b → 92ae7d7e63ca5ad97603d208de351bef7a2085e3 |
---|---|
Status: | new → needs_review |
comment:34 Changed 6 years ago by
Commit: | 92ae7d7e63ca5ad97603d208de351bef7a2085e3 → 8afdf212e25c9fd815d306ad89357f86309933aa |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
8afdf21 | Fix various dependencies
|
comment:35 Changed 6 years ago by
Description: | modified (diff) |
---|
comment:36 Changed 6 years ago by
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 6 years ago by
(Also, sorry, I was on vacation last week. No disagreement with anything else here.)
comment:38 Changed 6 years ago by
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 6 years ago by
@embray: please clearly state what you require to set this to positive_review.
comment:40 follow-up: 42 Changed 6 years ago by
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 6 years ago by
Commit: | 8afdf212e25c9fd815d306ad89357f86309933aa → e351c98d94505343082bc16d9397a87f7058eef5 |
---|
comment:42 Changed 6 years ago by
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: 44 Changed 6 years ago by
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.
comment:44 Changed 6 years ago by
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 6 years ago by
(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:50 Changed 6 years ago by
Commit: | e351c98d94505343082bc16d9397a87f7058eef5 → 8fd8273a000d17fba9fde19311aaf7adf6052fcb |
---|
comment:51 Changed 6 years ago by
Status: | needs_work → needs_review |
---|
Not fully tested yet, but this reverts all changes to spkg-install
, except for the pip
package.
comment:54 Changed 6 years ago by
Status: | positive_review → needs_work |
---|
comment:55 follow-up: 57 Changed 6 years ago by
The patch bumps the version numbers of dozens of packages without making changes to them. This seems unnecessary.
comment:56 Changed 6 years ago by
Reviewers: | → Jeroen Demeyer, Erik Bray, Matthias Koeppe |
---|
comment:57 Changed 6 years ago by
Status: | needs_work → 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 6 years ago by
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 6 years ago by
Branch: | u/jdemeyer/sage-pip-install → 8fd8273a000d17fba9fde19311aaf7adf6052fcb |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:60 Changed 6 years ago by
Commit: | 8fd8273a000d17fba9fde19311aaf7adf6052fcb |
---|
OK, thanks for the explanation.
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.