-
Notifications
You must be signed in to change notification settings - Fork 7.1k
remove explicit install of Pillow throughout CI #5950
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
Conversation
@@ -10,10 +10,9 @@ dependencies: | |||
- jpeg | |||
- ca-certificates | |||
- hdf5 | |||
- setuptools == 58.0.4 | |||
- setuptools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a refinement as proposed in #5868 (comment). See the next comment for details.
if [[ "${PYTHON_VERSION}" == '3.7' ]]; then | ||
pip install --upgrade setuptools==58.0.4 | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, setuptools
and similar packages used cp3_10
as the interpreter tag for Python 3.10. After PEP 641 was rejected, they changed to cp310
. This patch was included in setuptools==59.1.0
. With this tag set incorrectly, python setup.py develop
ignores wheels on Python 3.10 and tries to use the source version. That is fine for pure python libraries, but means that it will try to compile Pillow from source.
Unfortunately, the defaults
channel provides nothing between setuptools==58.0.4
and setuptools==61.2.0
. The former has the problems described above and the latter has problems on Python 3.7 (see #5868). Thus, I opted to use the latest release in general and special case Python 3.7 here. When we hard-depend on Python 3.8, we can simply remove this.
Failures are unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and the tests also pass (kicked them again to resolve the mypy problem).
@NicolasHug Any concerns? I think keeping the pil versions in 2 places is suboptimal but better than the previous approach, so I think it's worth merging this.
Hey @pmeier! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
Summary: * remove explicit install of Pillow throughout CI * explicitly install zlib in windows conda env * check pillow on Windows * move check into conda env * fix * more debug output * more debug * add debug also to linux * retry w/ and w/o packaging third party library * tag debug * more debug * pin setuptools < 61 * install setuptools from pip * cleanup * try remove one more Reviewed By: YosuaMichael Differential Revision: D36281604 fbshipit-source-id: 451322d4184abfb649b8ee70540c2dee6f008031 Co-authored-by: Vasilis Vryniotis <[email protected]>
While looking into unifying our dependencies I wondered why we are installing PIL explicitly in multiple places throughout the CI, although installing
torchvision
should pull it in automatically. We aren't doing that with the other hard dependencies such asnumpy
orrequests
.Turns out, we don't need to do that. After this PR, the only mentions of the
Pillow
version arevision/setup.py
Lines 67 to 70 in b0dbbd7
and
vision/packaging/torchvision/meta.yaml
Line 31 in b0dbbd7
Given that these are "package blueprints" there is not much room of improvement.
setup.py
is executed, so we could parse the version from the conda recipe. But that feels a little to much IMO.