Skip to content
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

pip should be upgraded when the Python shims are added #338

Open
moooeeeep opened this issue Oct 25, 2021 · 13 comments
Open

pip should be upgraded when the Python shims are added #338

moooeeeep opened this issue Oct 25, 2021 · 13 comments

Comments

@moooeeeep
Copy link
Contributor

moooeeeep commented Oct 25, 2021

On Ubuntu 18.04 the python-pip and python3-pip packages provide pip version 9.0.1, which doesn't provide features many pip packages require for installation these days, e.g., opencv afaik.

Even when users had their pip upgraded and working for their user, when they set up the workspace they will default to the old version again, after the PYTHONUSERBASE is adjusted. Thus the installation of pip osdeps or pip packages can fail with obscure error messages, typically pointing towards missing dependencies.

This should be avoidable by running {python_executable} -m pip install --upgrade pip (or similar) after setting up the python workspace.

Not sure if users are affected that did run the pip upgrade via sudo for the global install.

Otherwise I'd suggest this should be done for everyone, as the old version really isn't supposed to be run by anyone anymore.


Note sure if this is the right place to report this, or if this would be the better fit: https://github.com/rock-core/package_set/issues

@g-arjones
Copy link
Contributor

As per discussed in the other issues, autoproj should not install anything during bootstrap (other than importer's dependencies) because it can't know what should be installed.

@moooeeeep
Copy link
Contributor Author

moooeeeep commented Nov 22, 2021

The problem is, we need this upgrade for installing some pip osdeps. We need to do this after the shims were set up, but before the pip osdeps are installed.

Maybe we can get away with hooking into some package set's init.rb in this case. But I'd prefer if this would be done right at the pip_manager for example.

@moooeeeep
Copy link
Contributor Author

Not sure if users are affected that did run the pip upgrade via sudo for the global install.

Users that upgrade their pip system-wide (sudo -H pip3 install --upgrade pip) are on the safe side in this regard, it seems.

@moooeeeep
Copy link
Contributor Author

moooeeeep commented Feb 3, 2022

FWIW: A more intricate version of this problem occurs on Ubuntu 20.04, with pip version 20.0.2. There it occurs, when you install some Python package dependency via apt-get, e.g., python3-networkx and then other Python packages via pip, e.g., pgmpy and bnlearn that require different version ranges of that dependency networkx, one of which is satisfied by the then existing installation.

pip will check if the dependency is already satisfied and come to the wrong conclusion if both packages are installed with the same pip install invocation. It will write a log entry for the other package about the incompatible version of the dep and potentially cause obscure errors during installation or at runtime, depending on when and how the dependency is actually used.

It works with the latest version of pip. It also works if the packages are installed with separate calls of pip install. Then it will also install the newer version of networkx that is only required for one of the osdeps.

@doudou
Copy link
Member

doudou commented Feb 9, 2022

I guess this is something that could be done by the PipManager, no ? Maybe to avoid spending a lot of time there, check the current version of pip and act only if we're below a certain version ?

@moooeeeep
Copy link
Contributor Author

moooeeeep commented Feb 10, 2022

I think I would expect to find the upgrade code somewhere around here: https://github.com/rock-core/autoproj/blob/master/lib/autoproj/python.rb

We have in one of our recent buildconf's init.rb a hack that checks the version and does the upgrade. Similar to this:

# check if pip needs an upgrade
begin
    body = Net::HTTP.get(URI('https://pypi.org/pypi/pip/json'))
    latest_version = JSON[body]['info']['version']
    Kernel::system("if python3 -c \"import pip; exit(pip.__version__ == #{latest_version})\"; then sudo -H pip3 install --upgrade pip --no-input; fi")
rescue => error
    Autoproj.error "error checking/upgrading pip (#{error.message})"
end

We do the global upgrade there, because at the time this code is run we can't be sure the Python environment is already set up. I'd prefer to do install the upgrade to the local environment though.

@doudou
Copy link
Member

doudou commented Feb 10, 2022

Actually, even if this file creates the pip shims, it does not use pip. But then, where the update method is defined is one thing (whether in Autoproj::Python or Autoproj::PackageManagers::PipManager is not really important). What's bothering me is where it will be called.

My understanding is that we can do it in #install (with a flag telling us we've already done it to reduce the cost), if we make sure that Autobuild.tool('pip') points to our shim (which we should since that's the only thing that guarantees the python version).

@moooeeeep
Copy link
Contributor Author

With no particular reason I would have it happen somewhere below activate_python, but install should work too I guess.

@doudou
Copy link
Member

doudou commented Feb 10, 2022

In principle, one can use activate_python without ever using pip. Since I see no particular reason why it is better to do it in activate_python, I'd rather do it when we know we have to use pip.

@moooeeeep
Copy link
Contributor Author

I'd prefer to have a properly set up Python environment from the start, even if it's not immediately used by aup.

Potential other reasons: If one day autobuild turns towards installing packages using pip too, that version check might need to be duplicated there.

@g-arjones
Copy link
Contributor

g-arjones commented Feb 10, 2022

I'd prefer to have a properly set up Python environment from the start

Please don't do this in places that will be called in commands that have nothing to do with osdeps. activate_python gets called pretty much everywhere (autoproj which, autoproj show, amake, autoproj exec, autoproj envsh, etc).

@g-arjones
Copy link
Contributor

We have to be mindful of --checkout-only and --only-local as well

@moooeeeep
Copy link
Contributor Author

Please don't do this in places that will be called in commands that have nothing to do with osdeps. activate_python gets called pretty much everywhere (autoproj which, autoproj show, amake, autoproj exec, autoproj envsh, etc).

Sounds reasonable too. I hope you can find a place where this can be hookep up sensibly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants