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

Always run pip installer to get implicit deps #186

Merged
merged 4 commits into from
Apr 25, 2017

Conversation

mal
Copy link
Contributor

@mal mal commented Mar 31, 2017

Unconditionally executing the contents of this if block results in the installation of wheel (consistent with <= 3.3 images) without over-zealous overhead:

[...] # end of make install output
Successfully installed pip-9.0.1 setuptools-28.8.0
+ wget -O /tmp/get-pip.py https://bootstrap.pypa.io/get-pip.py
Connecting to bootstrap.pypa.io (151.101.0.175:443)
get-pip.py           100% |*******************************|  1558k  0:00:00 ETA
+ python3 /tmp/get-pip.py pip==9.0.1
Requirement already up-to-date: pip==9.0.1 in /usr/local/lib/python3.6/site-packages
Collecting wheel
  Downloading wheel-0.29.0-py2.py3-none-any.whl (66kB)
Installing collected packages: wheel
Successfully installed wheel-0.29.0
+ rm /tmp/get-pip.py
[...]

Fixes #141, #170

Copy link
Member

@yosifkit yosifkit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems sane to me

@yosifkit yosifkit requested a review from tianon March 31, 2017 20:47
@tianon
Copy link
Member

tianon commented Apr 3, 2017

I'm a little bit confused about what this does that just pip3 install --no-cache-dir --upgrade --force-reinstall pip==9.0.1 is not doing (which is invoked later).

@dstufft would you be willing to give us some advice on whether it's sane / smart to always invoke get-pip.py, regardless of whether our Python install already includes a pip3 which we can use for bootstrapping via --force-reinstall? Any (other) suggestions for improvement? 🙏 😇 ❤️ ❤️

@mal
Copy link
Contributor Author

mal commented Apr 3, 2017

It installs wheel which is not present in any of the existing > 3.3 images. If there's a better way of achieving the same, all the better 😄

@dstufft
Copy link

dstufft commented Apr 3, 2017

I'm on my phone but a quick glance suggests the impetus for this change is that get-pip.py installs pip/setuptools/wheel but ensurepip only installs pip/setuptools. Expanding your later command to also include setuptools and wheel should solve this also I think.

I will take a closer look when I get home.

@tianon
Copy link
Member

tianon commented Apr 3, 2017

Oh, we also have #170, where folks are suggesting that we need newer setuptools, so perhaps simply keeping setuptools and wheel up-to-date in the same way we handle pip would make sense? 😄

(Definitely appreciate the input ❤️ ❤️ ❤️)

@mal
Copy link
Contributor Author

mal commented Apr 4, 2017

Just did a quick PoC of the suggested approach, following along with how pip version management is done.

There's a version difference between the wheel installed by pip's get-pip.py and the wheel installed as a result of version grabbing from pypi: wheel-0.29.0 vs wheel-0.30.0a0. I'm unsure if that's an issue but worth noting.

Happy to update this PR when we know which direction we'd like to take this 😄

@dstufft
Copy link

dstufft commented Apr 4, 2017

That'll be because pip excludes pre-releases by default.

@tianon
Copy link
Member

tianon commented Apr 4, 2017

Ah, get-pip.py includes the following:

    # Add any implicit installations to the end of our args
    if implicit_pip:
        args += ["pip"]
    if implicit_setuptools:
        args += ["setuptools"]
    if implicit_wheel:
        args += ["wheel"]

(which just installs the latest release of each)

Seems roughly sane (and I think the way we fetch the latest pip version in update.sh accounts for that already, and if not, should be fixed and then copied for doing setuptools and wheel also).

@mal
Copy link
Contributor Author

mal commented Apr 5, 2017

In that case I think the current way of getting the package versions might need a tweak, because as I pointed out above, right now it happily grabs pre-release versions.

# curl -fsSL 'https://pypi.python.org/pypi/pip/json' | awk -F '"' '$2 == "version" { print $4 }'
9.0.1
# curl -fsSL 'https://pypi.python.org/pypi/setuptools/json' | awk -F '"' '$2 == "version" { print $4 }'
34.3.3
# curl -fsSL 'https://pypi.python.org/pypi/wheel/json' | awk -F '"' '$2 == "version" { print $4 }'
0.30.0a0

A look at the docs claims that it will fetch the stable version by default but that does not appear to be happening:

This retrieves information about the latest stable release

In the meantime, I'll update this PR with the version that grabs pre-releases and we can go forward from there.

@mal
Copy link
Contributor Author

mal commented Apr 5, 2017

I'll squash this down once we're happy with it 😉

@mal
Copy link
Contributor Author

mal commented Apr 7, 2017

Is it permissable to use tools such as jq when processing the https://pypi.python.org/pypi/wheel/json style URLs, or are we limited to using common system tools?

@tianon
Copy link
Member

tianon commented Apr 7, 2017

Yeah, jq is one that's permissible (we use it in other places, so it's safe to assume it being installed). I was thinking of replacing the awk with jq since that'd be more reliable anyhow. 😄

I think the main issue I see is that the alpha is listed as the actual "latest release" for wheel by PyPI, so I'm not sure how we're expected to know that 0.29.0 is what we ought to be consuming unless we implement the PEP for Python version number sorting ourselves. 😞

@dstufft
Copy link

dstufft commented Apr 7, 2017

Try https://pypi.org/pypi/wheel/json.

@mal
Copy link
Contributor Author

mal commented Apr 7, 2017

Aha! Perfect! I'll get a commit going, thanks 😁

@tianon
Copy link
Member

tianon commented Apr 7, 2017

You're a hero, @dstufft ❤️ (thanks for all you do! 👍)

@dstufft
Copy link

dstufft commented Apr 7, 2017

I feel obligated to mention that URL is technically not "production" which mostly just means there's no alerting if it goes down, but that should be fairly rare to get downtime of that URL that actually makes it unavailable.

@tianon
Copy link
Member

tianon commented Apr 7, 2017

Yeah, I figured as much when I checked out https://pypi.org (and saw the warning), but it's only going to be used from our update.sh script that's used for updating the version numbers, not during builds, so I'm +1 on using it since it solves our issues. 😄 ❤️ (especially since now we can claim to be guinea pigs helping test pypi.org! 😇)

@mal
Copy link
Contributor Author

mal commented Apr 10, 2017

Anything more to do on this one @tianon?

tianon added a commit to docker-library/oi-janky-groovy that referenced this pull request Apr 25, 2017
@tianon
Copy link
Member

tianon commented Apr 25, 2017

I took the liberty of rebasing this on #187 for you, and added a commit (60d03bd) which both fixes our pip update code on Windows, and applies this same change there. 👍 🤘

@yosifkit yosifkit merged commit 8c6dcf6 into docker-library:master Apr 25, 2017
tianon added a commit to infosiftr/stackbrew that referenced this pull request Apr 26, 2017
- `docker`: experimental multiarch (docker-library/docker#52)
- `golang`: experimental multiarch (docker-library/golang#158)
- `nextcloud`: 10.0.5, 11.0.3, 9.0.58, update via container updates (nextcloud/docker#65)
- `postgres`: ensure `postgres` user's HOME has decent permissions (docker-library/postgres#277)
- `python`: fix `pip` install to also install `setuptools` and `wheel` (docker-library/python#186, docker-library/python#187)
@JayH5
Copy link
Contributor

JayH5 commented Apr 28, 2017

I'm a bit concerned about tracking the setuptools version as it moves very fast which would lead to these images being rebuilt very often. For example, there's already version 35.0.2.

https://github.com/pypa/setuptools/blame/master/CHANGES.rst

@tianon
Copy link
Member

tianon commented Apr 28, 2017

Hmm, good point @JayH5. @yosifkit and I took a look at Python's ./configure options, and we're considering using --without-ensurepip to have Python no longer install the embedded pip at all, and just use get-pip.py unilaterally, which means we can even move the get-pip.py into a separate layer and get even better cache (especially for minor pip bumps). 😅

(Then, since we'll be using get-pip.py unconditionally, we'll always get setuptools and wheel, and they'll be the "latest release at the time of our installation of pip", which also solves the problem of "when do we bump them?" -- they'll be naturally bumped any time Python, pip, or our base image are. 👍)

@tianon
Copy link
Member

tianon commented Apr 28, 2017

(Which ironically enough is basically exactly what @mal suggested originally -- sorry @mal 🙈 ❤️ ❤️)

tao12345666333 pushed a commit to tao12345666333/python that referenced this pull request Jun 28, 2018
Always run pip installer to get implicit deps
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

Successfully merging this pull request may close these issues.

5 participants