Skip to content

move JPEG reference test into separate CI job #5184

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

Closed
wants to merge 27 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions .circleci/config.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 34 additions & 0 deletions .circleci/config.yml.in
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,39 @@ jobs:
- run_tests_selective:
file_or_dir: test/test_prototype_*.py

# The JPEG reference tests will only pass if torchvision is built against the same JPEG library as Pillow. Starting
# from Pillow==9, the Pillow wheels are built against libjpeg-turbo on all platforms whereas torchvision is built
# against libjpeg. By installing Pillow from conda-forge, it uses whatever jpeg library is already present instead of
# packaging one.
Comment on lines +364 to +365
Copy link
Member

Choose a reason for hiding this comment

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

, it uses whatever jpeg library is already present instead of
packaging one.

I'm not sure I understand what this means. Should we just say "By installing Pillow from conda-forge, we make sure that pillow is built against libjpeg" ?

unittest_jpeg_ref:
docker:
- image: condaforge/mambaforge
Copy link
Member

Choose a reason for hiding this comment

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

Any chance we could stick to conda instead of mamba, so as to stick to the current tools that we use in the rest of the job?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We probably can (there is most likely a docker image that gives us conda), but I'm not sure we should. mamba is a re-implementation of the protocol, but faster. So we effectively waste CI resources by using conda.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we rely on the same image as the other unittest jobs here, to avoid any risk of deviating from our "baseline" testing infra? I believe they all rely on conda as well so there must be a way to install conda, even if we don't choose condaforge/mambaforge as the image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Besides our CPU workflows on Linux, none of the other workflows even use docker:

vision/.circleci/config.yml

Lines 719 to 722 in d675c0c

unittest_linux_cpu:
<<: *binary_common
docker:
- image: "pytorch/manylinux-cuda102"

vision/.circleci/config.yml

Lines 758 to 761 in d675c0c

unittest_linux_gpu:
<<: *binary_common
machine:
image: ubuntu-1604-cuda-10.2:202012-01

vision/.circleci/config.yml

Lines 808 to 811 in d675c0c

unittest_windows_cpu:
<<: *binary_common
executor:
name: windows-cpu

vision/.circleci/config.yml

Lines 846 to 849 in d675c0c

unittest_windows_gpu:
<<: *binary_common
executor:
name: windows-gpu

vision/.circleci/config.yml

Lines 893 to 896 in d675c0c

unittest_macos_cpu:
<<: *binary_common
macos:
xcode: "12.0"

Apart from that, they all download the miniconda installer:

wget -O miniconda.sh "http://repo.continuum.io/miniconda/Miniconda3-latest-${os}-x86_64.sh"

curl --output miniconda.exe https://repo.anaconda.com/miniconda/Miniconda3-latest-Windows-x86_64.exe -O

There is a container for that, but again, I see no point. It makes no difference if you install the environment with conda or mamba other than the latter is significantly faster.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to switch all of our jobs to mamba in another PR, but could we please stick to the same workflow that the other jobs are using? There is value in avoiding discrepancies with the rest of our CI jobs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What other workflow? Manually installing conda and setting things up or can I at least use a docker image with conda pre-installed?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm a bit confused: what prevents us from staying as close as possible to unittest_linux_cpu: ?

resource_class: xlarge
steps:
- run:
name: Prepare mamba
command: |
mv ~/.bashrc ~/._bashrc
conda init bash
cat ~/.bashrc >> $BASH_ENV
mv ~/._bashrc ~/.bashrc
- checkout
- run:
name: Create environment
command: |
mamba env create -n jpeg-ref -f .circleci/unittest/jpeg-ref-env.yml
echo 'conda activate jpeg-ref' >> $BASH_ENV
- run:
name: Install torchvision
command: pip install -v --no-build-isolation --editable .
- run:
name: Enable JPEG ref tests
command: echo 'export PYTORCH_TEST_JPEG_REF=1' >> $BASH_ENV
- run_tests_selective:
file_or_dir: >
test/test_image.py::test_encode_jpeg
test/test_image.py::test_write_jpeg

binary_linux_wheel:
<<: *binary_common
docker:
Expand Down Expand Up @@ -1093,6 +1126,7 @@ workflows:
- unittest_torchhub
- unittest_onnx
- unittest_prototype
- unittest_jpeg_ref
{{ unittest_workflows() }}

cmake:
Expand Down
19 changes: 19 additions & 0 deletions .circleci/unittest/jpeg-ref-env.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
channels:
- pytorch-nightly
- conda-forge

dependencies:
- python == 3.7.*
- setuptools
- compilers
- ninja
- cmake

- cpuonly
- pytorch

- numpy
- requests
- libpng
- jpeg
- pillow >=5.3.0, !=8.3.*
23 changes: 23 additions & 0 deletions test/common_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@


def get_bool_env_var(name, *, exist_ok=False, default=False):
"""Gets the value of a boolean environment variable, by mapping common bool-ish
values to a :class:`bool`.

Args:
name: Name of the environment variable.
exist_ok: Returns ``True`` if the environment variable exists regardless of the
value it holds. This is useful for checking if an environment variable
exists even if it holds no value.
default: Value to return if the environment variable doesn't exist. Defaults to
``False``.
"""
value = os.getenv(name)
if value is None:
return default
Expand Down Expand Up @@ -216,4 +227,16 @@ def _test_fn_on_batch(batch_tensors, fn, scripted_fn_atol=1e-8, **fn_kwargs):


def run_on_env_var(name, *, skip_reason=None, exist_ok=False, default=False):
"""Decorator for tests to only run them if an environment variable is ``True``-ish.

Args:
name: Name of the environment variable.
skip_reason: Reason to skip the test if environment variable is not ``True``-ish.
exist_ok: Returns ``True`` if the environment variable exists regardless of the
value it holds. This is useful for checking if an environment variable
exists even if it holds no value.
default: Value to return if the environment variable doesn't exist. Defaults to
``False``.

"""
return pytest.mark.skipif(not get_bool_env_var(name, exist_ok=exist_ok, default=default), reason=skip_reason)
19 changes: 15 additions & 4 deletions test/test_image.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import torch
import torchvision.transforms.functional as F
from common_utils import needs_cuda, assert_equal
from common_utils import run_on_env_var
from PIL import Image, __version__ as PILLOW_VERSION
from torchvision.io.image import (
decode_png,
Expand Down Expand Up @@ -478,8 +479,19 @@ def test_write_jpeg_reference(img_path, tmpdir):
assert_equal(torch_bytes, pil_bytes)


# TODO: Remove the skip. See https://github.com/pytorch/vision/issues/5162.
@pytest.mark.skip("this test fails because PIL uses libjpeg-turbo")
run_if_test_jpeg_ref = run_on_env_var(
"PYTORCH_TEST_JPEG_REF",
skip_reason=(
"JPEG reference tests compare `torchvision` JPEG encoding against `Pillow`'s. "
"By default `torchvision` is build against `libjpeg` while `Pillow` on wheels are built against "
"`libjpeg-turbo` on all platform starting from `Pillow >= 9`. For `Pillow < 9`, only the windows wheel was "
"built against `libjpeg-turbo`. Make sure to use the same underlying library (by running `Pillow < 9` on Linux "
"or macOS or by installing `Pillow` from conda-forge) and set PYTORCH_TEST_JPEG_REF=1 to run the tests."
),
)


@run_if_test_jpeg_ref
@pytest.mark.parametrize(
"img_path",
[pytest.param(jpeg_path, id=_get_safe_image_name(jpeg_path)) for jpeg_path in get_images(ENCODE_JPEG, ".jpg")],
Expand All @@ -498,8 +510,7 @@ def test_encode_jpeg(img_path):
assert_equal(encoded_jpeg_torch, encoded_jpeg_pil)


# TODO: Remove the skip. See https://github.com/pytorch/vision/issues/5162.
@pytest.mark.skip("this test fails because PIL uses libjpeg-turbo")
@run_if_test_jpeg_ref
@pytest.mark.parametrize(
"img_path",
[pytest.param(jpeg_path, id=_get_safe_image_name(jpeg_path)) for jpeg_path in get_images(ENCODE_JPEG, ".jpg")],
Expand Down