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

move JPEG reference test into separate CI job #5184

Closed
wants to merge 27 commits into from

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Jan 10, 2022

Addresses #5162 (comment). This is the newly added CI job.

cc @seemethere

@facebook-github-bot
Copy link

facebook-github-bot commented Jan 10, 2022

💊 CI failures summary and remediations

As of commit c070c6b (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@pmeier pmeier marked this pull request as ready for review January 10, 2022 16:10
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR @pmeier , I took a quick look, LMK what you think

test/test_image.py Outdated Show resolved Hide resolved
test/test_image.py Outdated Show resolved Hide resolved
@@ -359,6 +359,33 @@ jobs:
- run_tests_selective:
file_or_dir: test/test_prototype_*.py

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: ?

test/test_image.py Outdated Show resolved Hide resolved
test/test_image.py Outdated Show resolved Hide resolved
.circleci/config.yml.in Show resolved Hide resolved
test/common_utils.py Show resolved Hide resolved
test/common_utils.py Show resolved Hide resolved
Comment on lines +364 to +365
# against libjpeg. By installing Pillow from conda-forge, it uses whatever jpeg library is already present instead of
# packaging one.
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" ?

@@ -359,6 +359,33 @@ jobs:
- run_tests_selective:
file_or_dir: test/test_prototype_*.py

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.

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

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

Successfully merging this pull request may close these issues.

None yet

3 participants