Warn when webp is asked to decode into grayscale #9101
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/9101
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit bcf6488 with merge base 904dad4 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
| mode == IMAGE_READ_MODE_GRAY || | ||
| mode == IMAGE_READ_MODE_GRAY_ALPHA, |
There was a problem hiding this comment.
I think we TORCH_WARN_ONCE works differently than TORCH_CHECK, and doesn't accept a bool as the first parameter (well it does, but it'll just print it). So right now we always warn unconditionally, I think we need to add this condition check in an if block
There was a problem hiding this comment.
That explains the output I was seeing! Got it, will fix.
test/test_image.py
Outdated
| # Note that we warn at the C++ layer because dispatching for decode_image | ||
| # doesn't happen until we hit C++. The C++ layer does not propagate | ||
| # warnings up to Python, so we can't test for them. | ||
| img = decode_fun(encoded_bytes, mode=ImageReadMode.GRAY) |
There was a problem hiding this comment.
It's a shame we don't get a nice python warning, but I guess that'll do for now. We can still catch it by adding the capfdfixture , and then something like:
assert "Webp does not support grayscale conversions." in capfd.readouterr().errNow the first test parametrization with decode_webp will pass, but the one with decode_image will fail as expected, because we only warn once. Maybe we should only test decode_image then, since it's the highest-level function.
There was a problem hiding this comment.
Actually, if we're capturing the output, then both functions should pass the test, as decode_webp() calls the C++ function of the same name - and that's where the check is. I'll see if I can improve it.
There was a problem hiding this comment.
Also: I misread your comment, as you were talking about the warn-once aspect. I addressed that with the API that lets us temporarily turn on warnings all the time. Without it, both tests failed, which means that some test somewhere was already triggering the warning.
| torch._C._set_warnAlways(False) | ||
|
|
||
| with set_always_warn(): | ||
| img = decode_fun(encoded_bytes, mode=ImageReadMode.GRAY) |
There was a problem hiding this comment.
No need to address but as a side note, I think we only need this one line above to be within the context manager.
|
Hey @scotts! You merged this PR, but no labels were added. |
Reviewed By: AntoineSimoulin Differential Revision: D79175058 fbshipit-source-id: 0d5b6256a5d00d992d8190a7c25296f7ce72bcf8
Addresses #8753 by adding a warning when users request to decode a webp image into a grayscale format.
Note that we only warn at the C++ layer, and not the Python layer. My reasoning is that for
decode_image(), we only can warn at the C++ layer because we don't know the image format until we get to the C++ layer. We could warn at the Python level in the Python functiondecode_webp(), but then users would get a double warning: one at the Python level and one at the C++ level. I figure it's cleaner to only do the warning in one place.