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

Do not install unsupported Pillow for Python 3.8+ #9118

Closed
hugovk opened this issue Apr 16, 2022 · 5 comments · Fixed by #9473
Closed

Do not install unsupported Pillow for Python 3.8+ #9118

hugovk opened this issue Apr 16, 2022 · 5 comments · Fixed by #9473
Assignees
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required

Comments

@hugovk
Copy link
Contributor

hugovk commented Apr 16, 2022

Details

Expected Result

Unnecessary dependencies are not installed (related #8208), and do not slow down the build.

Actual Result

https://docs.readthedocs.io/en/stable/build-default-versions.html#external-dependencies says that Pillow 5.4.1 is an external dependency and that it "could be removed in the future".

We require Python 3.9 to build the PEPs docs and do not need Pillow.

Pillow 5.4.1 (released Jan 2019) supported Python 2.7 and 3.4-3.7, which means binary wheels are only available for those versions. Pillow 8.0 is the first version support to Python 3.9 with binary wheels:

This means Python 3.9 downloads the big sdist and builds from source, which is much slower than installing from wheel:

Collecting pillow==5.4.1
  Downloading Pillow-5.4.1.tar.gz (16.0 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 16.0/16.0 MB 136.4 MB/s eta 0:00:00
...
Building wheels for collected packages: mock, pillow, future
  Building wheel for mock (setup.py): started
  Building wheel for mock (setup.py): finished with status 'done'
  Created wheel for mock: filename=mock-1.0.1-py3-none-any.whl size=23772 sha256=3178833b900aad79f904be4ec39fd9218034a3c8eb93ff168e7655db36f4a7ef
  Stored in directory: /tmp/pip-ephem-wheel-cache-bnbn_mt2/wheels/44/dc/c7/e89296d3612588db0aa6545ee3305c23b7a48b3a9922916e1f
  Building wheel for pillow (setup.py): started
  Building wheel for pillow (setup.py): finished with status 'done'
  Created wheel for pillow: filename=Pillow-5.4.1-cp39-cp39-linux_x86_64.whl size=1361211 sha256=065d9d668909332f20be3ebc31f77207067f846cdf142b3395dceac8aa236828
  Stored in directory: /tmp/pip-ephem-wheel-cache-bnbn_mt2/wheels/d3/de/21/f0ad1b0453288616d3f4d2ce28431aa7a2adde8f27c9b7a0af
  Building wheel for future (setup.py): started
  Building wheel for future (setup.py): finished with status 'done'
  Created wheel for future: filename=future-0.18.2-py3-none-any.whl size=491070 sha256=a59cfed6bc119fcf842c411312471446a70d5791d55d8bf4f27a58d52cf62dbf
  Stored in directory: /tmp/pip-ephem-wheel-cache-bnbn_mt2/wheels/2f/a0/d3/4030d9f80e6b3be787f19fc911b8e7aa462986a40ab1e4bb94
Successfully built mock pillow future
...
Command time: 39s Return: 0

Some suggestions, in my order of preference:

  • Do not install Pillow (as suggested by the docs, "could be removed in the future")
  • Install a newer version of Pillow for Python 3.8+, that has binary wheels
  • Cache the built wheels so they do not need to be rebuilt. But it's quite possible that Pillow 5.4.1 won't actually work on Python 3.8+, and at some point may even break the build.

I'm focusing on Pillow here as the main bottleneck, but we probably don't need most of the other external dependencies, and would prefer to only install things we've explicitly specified.

But if we can begin by addressing Pillow, that would be a good first step. Thank you!

@humitos
Copy link
Member

humitos commented Apr 19, 2022

Thanks for reporting this. We are working on the "Future builders" (build.jobs and build.commands config keys) and moving in a direction where all these things can be configured by the user. There are other issues where this conversation is taking place and we will consider this use case as well. However, I don't have a specific and good answer for a workaround currently.

@humitos
Copy link
Member

humitos commented Jul 4, 2022

I did a quick query to our database and I found 1.5k projects in this situation (Pillow==5.4.1 and Python 3.8) in the last month and a half.

In [3]: BuildData.objects.filter(
   ...:     data__packages__pip__all__contains=[{"name": "Pillow", "version": "5.4.1"}],
   ...:     data__python__startswith="3.8",
   ...: ).values_list("data__project__slug", flat=True).distinct().count()
Out[3]: 1564

It may be worth prioritizing a potential solution here to avoid re-compile Pillow over and over for these projects.

@ericholscher
Copy link
Member

This should now be deployed, if you could confirm it's working 👍

@hugovk
Copy link
Contributor Author

hugovk commented Aug 10, 2022

Yes, this is working insofar as it's installing a Pillow wheel suitable for the Python version, and not slowly building an old version from source. Thank you!

Before deploy:

After deploy:

I would prefer not having to install unused dependencies: they may install quickly, but we're still unnecessarily downloading from PyPI, multiplied by every build, which has a huge (although 100% discounted) $1.8m/month bill.

We touched on it here python/devguide#933 (comment) and python/devguide#933 (comment).


Quick investigation:

Here's the same docs built using build.commands to override the extra dependencies.

And adding in the extra dependencies:

So not too much slower: about 4s, but about 8.5 MB (+44 %) more download.

But my fork project is new; older projects also download/install sphinx<2 (3.1 MB)/sphinx-rtd-theme<0.5 (6.4 MB)/etc, before possibly re-downloading/installing modern versions/themes. This is an extra ~10 MB and +~100%.


Anyway, I'm happy this is a good improvement, that we can override the build process if needed, and you have a long term plan too. Thank you very much!

@humitos
Copy link
Member

humitos commented Aug 10, 2022

@hugovk thanks for opening this issue and for your quick investigation.

But my fork project is new; older projects also download/install sphinx<2 (3.1 MB)/sphinx-rtd-theme<0.5 (6.4 MB)/etc, before possibly re-downloading/installing modern versions/themes. This is an extra ~10 MB and +~100%.

Yeap. New projects do not pin sphinx and sphinx-rtd-theme dependencies. We did change old projects to the new unpinned command to avoid breaking those projects that were depending on them. I assume that's why devguide is installing sphinx<2 and hugovk-devguide is installing sphinx.

I could enable the feature flag USE_SPHINX_LATEST on devguide project if you think that will be better. I think it's just the same, unless, devguide is using the latest Sphinx version. In that case, it won't download it twice. Let me know.


I opened an internal discussion to find out a clean way to avoid installing Read the Docs' core dependencies under some circumstances without involving build.commands keeping backward compatibility. I'm not sure we will reach an immediate solution but I think it could be quicker than our long term plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants