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

Fix test_draw_boxes #3631

Merged
merged 2 commits into from Apr 6, 2021
Merged

Fix test_draw_boxes #3631

merged 2 commits into from Apr 6, 2021

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Apr 4, 2021

Looks like something changed (don't know what TBH) and this test is failing on new PRs like #3628 and #3624

This PR updates the reference image (EDIT: we also avoid the check if PIL_version < 8.2). The new image seems to be more correct as in the current one, the top pixel of the "b" letter seems to be separate from the rest https://github.com/pytorch/vision/blob/master/test/assets/fakedata/draw_boxes_util.png

EDIT: Closes #3637

@NicolasHug NicolasHug changed the title new image Fix test_draw_boxes Apr 4, 2021
@oke-aditya
Copy link
Contributor

oke-aditya commented Apr 5, 2021

One possible reason can be that a new version of PIL was released 3 days ago. Tests seem to start failing then.
Possible PR that might have affected.

This might bring to one question.
Dependencies on any specific PIL version for plotting utils functionalities.

For draw.Rectangle()

As we can see there is extra width param in stable Pillow

While 4.1.1 does not have. Have a look here

I see that in the setup.py file we have mentioned the minimum version here.
Maybe we can update that version or mention a note about plotting utils that.

It depends on the PIL version installed on your system. Results may vary depending on it.

I verified once but I don't think so we have any other plotting dependency.

@NicolasHug
Copy link
Member Author

NicolasHug commented Apr 5, 2021

One possible reason can be that a new version of PIL was released 3 days ago

You're right, thanks. The failure is caused by the release of PIL 8.2 (tests still pass on master with 8.1).

Regarding the PIL dependency: it might indeed be time to bump the minimal version. But more importantly: it seems that we're currently only testing against the latest PIL version, which is a bit risky. Only testing the latest version means that if we're using a PIL API that was only introduced recently, our PIL dependency pillow_ver = ' >= 4.1.1' would be effectively broken but the CI wouldn't catch that. It'd be preferable to have at least 2 CI instances: one with the latest PIL and one with the minimal supported version.

@fmassa WDYT?

@@ -120,8 +123,11 @@ def test_draw_boxes(self):
res = Image.fromarray(result.permute(1, 2, 0).contiguous().numpy())
res.save(path)

expected = torch.as_tensor(np.array(Image.open(path))).permute(2, 0, 1)
self.assertTrue(torch.equal(result, expected))
if PILLOW_VERSION >= (8, 2):
Copy link
Member Author

Choose a reason for hiding this comment

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

Another solution is to set the condition to if PILLOW_VERSION < (8, 2) and not update the image.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I'm ok adding one extra CI for an older version of PIL as well in a follow-up PR

@fmassa fmassa merged commit 9b19f0f into pytorch:master Apr 6, 2021
facebook-github-bot pushed a commit that referenced this pull request Apr 13, 2021
Summary:
* new image

* avoid check if pil version is < 8.2 as the reference image would be different

Reviewed By: NicolasHug

Differential Revision: D27706947

fbshipit-source-id: 4c43289e4ebf742b643112e24a8119fc1f70ac55
@jayfurmanek jayfurmanek mentioned this pull request May 10, 2021
3 tasks
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.

Unit test Tester.test_draw_boxes failing on Windows/Linux/MacOS test builds
4 participants