[MRG+1] Do not degrade JPEG files. #3689

anubhavp28 · 2019-03-15T12:45:20Z

Fixes #3055 by not converting a JPEG file again.

anubhavp28 · 2019-03-15T18:34:52Z

Hey @Gallaecio, could you help me please? The tests are failing only for Python 3.7. I am not sure if it is related to my changes.

anubhavp28 · 2019-03-17T11:37:47Z

Hey @kmike, could you help me with this please? The tests are failing only for Python 3.7. I am not sure if it is related to my changes.

codecov · 2019-03-22T11:50:40Z

Codecov Report

Merging #3689 into master will increase coverage by 0.9%.
The diff coverage is 57.14%.

@@            Coverage Diff            @@
##           master    #3689     +/-   ##
=========================================
+ Coverage   84.54%   85.44%   +0.9%     
=========================================
  Files         167      169      +2     
  Lines        9420     9989    +569     
  Branches     1402     1586    +184     
=========================================
+ Hits         7964     8535    +571     
  Misses       1199     1199             
+ Partials      257      255      -2

Impacted Files	Coverage Δ
scrapy/pipelines/images.py	`86.62% <57.14%> (-4.1%)`	⬇️
scrapy/commands/check.py	`24% <0%> (-1.72%)`	⬇️
scrapy/http/__init__.py	`100% <0%> (ø)`	⬆️
scrapy/http/request/__init__.py	`100% <0%> (ø)`	⬆️
scrapy/core/downloader/middleware.py	`100% <0%> (ø)`	⬆️
scrapy/squeues.py	`100% <0%> (ø)`	⬆️
scrapy/utils/gz.py	`100% <0%> (ø)`	⬆️
scrapy/http/request/json_request.py	`93.75% <0%> (ø)`
scrapy/pqueues.py	`98.97% <0%> (ø)`
scrapy/item.py	`98.48% <0%> (+0.07%)`	⬆️
... and 23 more

Gallaecio · 2019-03-22T11:52:11Z

The error is not related to your pull request. That test failure occurs randomly, we’ll have to look into it eventually. In the meantime, I’ve closed and reopened your pull request, which triggers the test runs again. Hopefully there will be no false positives this time.

Gallaecio

Please, add a test to cover this change, one that verified that JPEG/RGB images are not modified at a binary level.

anubhavp28 · 2019-03-27T14:12:51Z

@Gallaecio Can you review this again? I have made the changes suggested by you.

tests/test_pipeline_images.py

anubhavp28 · 2019-04-08T07:45:43Z

@Gallaecio Can you review this again please? If I am not wrong, my changes have decreased the code coverage, hence codecov is complaining. How should I increase it?

scrapy/pipelines/images.py

tests/test_pipeline_images.py

Gallaecio · 2019-04-11T15:48:58Z

I’ve left a few minor comments. But do not worry about coverage, its integration with GitHub feels a bit clunky at times, if you look at the actual details I think you are increasing coverage.

Gallaecio · 2019-04-17T05:41:42Z

I think this is ready. Thank you!

kmike · 2019-04-17T08:30:53Z

scrapy/pipelines/images.py

            yield thumb_path, thumb_image, thumb_buf

-    def convert_image(self, image, size=None):
+    def convert_image(self, image, response_body, size=None):


Unfortunately this is backwards incompatible; there are image pipeline subclasses in a wild which override this method - they'll stop working because of signature change. I've checked some codebases, and found examples of this. Pipeline may also override a method which calls this method. Could you please make it backwards compatible? It seems something like this is needed:

make response_body optional (None by default), make it a last argument

return buf if response_body is not passed (i.e. disable the feature in this PR if a middleware uses deprecated signature).

because user may override convert_image method, pipeline shouldn't always pass response_body - it should inspect convert_image method signature, and pass response_body only if this argument is present.

issue a warning when response_body is not passed (ScrapyDeprecationWarning) - it seems two warnings are needed, one when convert_image is overridden in incompatible way, and another one is when convert_image is called from an overridden method in incompatible way.

@kmike I have made changes, could you review this again?

@anubhavp28 One thing you could do, since the goal is to support the previous API, is to, instead of modifying the tests to use the new API, keep the old tests and add new tests that test the new API.

You could also extend the old tests to ensure that a deprecation warning is used.

@Gallaecio I am not sure how to write tests to ensure that deprecation warnings are received. Currently, I am just counting the number of warnings. Would that be enough?

scrapy/tests/test_pipeline_images.py

Line 111 in 881bade

self.assertTrue(len(w) >= 4)

@anubhavp28 Please, check other usages of catch_warnings in the tests and you’ll see how to check the warning message as well. You could ensure that the warnings contain certain text to ensure that they are the expected warnings, and not some other warnings.

Gallaecio · 2019-05-08T08:28:10Z

scrapy/pipelines/images.py

        yield path, image, buf

        for thumb_id, size in six.iteritems(self.thumbs):
            thumb_path = self.thumb_path(request, thumb_id, response=response, info=info)
-            thumb_image, thumb_buf = self.convert_image(image, size)
+            if convert_image_overriden:
+                _warn()


I don’t think you need to log a warning each time you check the value of convert_image_overriden, logging the warning once should be enough.

anubhavp28 · 2019-05-16T07:19:35Z

@Gallaecio I have made few changes. Though the AppVeyor build is failing, it doesn't seem to be related to my changes? Could you review this again?

Gallaecio

I’ve left a few minor comments, but it looks good to me.

scrapy/pipelines/images.py

anubhavp28 · 2019-05-18T10:23:12Z

@Gallaecio I have made the changes suggested by you. Could you review this again?

scrapy/pipelines/images.py

kmike · 2019-09-26T18:43:56Z

scrapy/pipelines/images.py

+            self._deprecated_convert_image = 'response_body' not in get_func_args(self.convert_image)
+            if self._deprecated_convert_image:
+                from scrapy.exceptions import ScrapyDeprecationWarning
+                import warnings


Could you please move these import to the top level, as well as imports in convert_image?

kmike · 2019-09-26T18:45:06Z

scrapy/pipelines/images.py

            yield thumb_path, thumb_image, thumb_buf

-    def convert_image(self, image, size=None):
+    def convert_image(self, image, size=None, response_body=None):
+        if not response_body:


Suggested change

if not response_body:

if response_body is None:

I think response_body can be really empty in some cases.

kmike · 2019-09-26T18:45:24Z

scrapy/pipelines/images.py

@@ -151,7 +175,9 @@ def convert_image(self, image, size=None):
        if size:
            image = image.copy()
            image.thumbnail(size, Image.ANTIALIAS)
-
+        elif response_body and image.format == 'JPEG':


Suggested change

elif response_body and image.format == 'JPEG':

elif response_body is not None and image.format == 'JPEG':

kmike · 2019-09-26T18:46:09Z

tests/test_pipeline_images.py

@@ -76,35 +76,71 @@ def test_thumbnail_name(self):
                         'thumbs/50/850233df65a5b83361798f532f1fc549cd13cbe9.jpg')

    def test_convert_image(self):
+        # tests for old API
+        with warnings.catch_warnings(record=True) as w:


could you please move "tests for old API" to a separate test method (or multiple test methods)?

anubhavp28 added 2 commits March 15, 2019 18:09

do not degrade JPEG files.

25e616f

make tests work with new convert_image

07487dd

Gallaecio closed this Mar 22, 2019

Gallaecio reopened this Mar 22, 2019

Gallaecio requested changes Mar 22, 2019

View reviewed changes

include test

ca882d8

Gallaecio requested changes Mar 28, 2019

View reviewed changes

tests/test_pipeline_images.py Outdated Show resolved Hide resolved

fix test

398639a

Gallaecio reviewed Apr 11, 2019

View reviewed changes

scrapy/pipelines/images.py Outdated Show resolved Hide resolved

tests/test_pipeline_images.py Outdated Show resolved Hide resolved

tests/test_pipeline_images.py Outdated Show resolved Hide resolved

tests/test_pipeline_images.py Outdated Show resolved Hide resolved

anubhavp28 added 2 commits April 13, 2019 10:10

make suggested changes

c6769d6

aesthetic changes

6039b66

Gallaecio approved these changes Apr 17, 2019

View reviewed changes

Gallaecio changed the title ~~Do not degrade JPEG files.~~ [MRG+1] Do not degrade JPEG files. Apr 17, 2019

kmike reviewed Apr 17, 2019

View reviewed changes

anubhavp28 added 3 commits April 29, 2019 21:21

makes fix backward compatible

2a6bcdb

test for deprecation warning

33925a7

tests for new API

881bade

Gallaecio reviewed May 8, 2019

View reviewed changes

makes suggested changes

653ac3e

Gallaecio changed the title ~~[MRG+1] Do not degrade JPEG files.~~ Do not degrade JPEG files. May 16, 2019

anubhavp28 closed this May 16, 2019

anubhavp28 reopened this May 16, 2019

Gallaecio reviewed May 16, 2019

View reviewed changes

scrapy/pipelines/images.py Outdated Show resolved Hide resolved

scrapy/pipelines/images.py Outdated Show resolved Hide resolved

makes suggested changes

2994b62

Gallaecio reviewed May 22, 2019

View reviewed changes

scrapy/pipelines/images.py Outdated Show resolved Hide resolved

makes suggested changes

c8e28ec

Gallaecio reviewed May 23, 2019

View reviewed changes

scrapy/pipelines/images.py Outdated Show resolved Hide resolved

cache if convert_image has deprecated signature

90fdefc

Gallaecio changed the title ~~Do not degrade JPEG files.~~ [MRG+1] Do not degrade JPEG files. May 27, 2019

anubhavp28 closed this Jul 4, 2019

anubhavp28 reopened this Jul 4, 2019

kmike added this to the v1.8 milestone Aug 8, 2019

kmike requested a review from wRAR September 26, 2019 12:45

wRAR approved these changes Sep 26, 2019

View reviewed changes

kmike reviewed Sep 26, 2019

View reviewed changes

Gallaecio modified the milestones: v1.8, v2.0 Oct 29, 2019

drs-11 added a commit to drs-11/scrapy that referenced this pull request Aug 25, 2020

cleaned up code relating to issue scrapy#3689

8b84a65

drs-11 mentioned this pull request Aug 25, 2020

Do not recode JPEG files in imagepipelines #4753

Merged

wRAR closed this in #4753 Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] Do not degrade JPEG files. #3689

[MRG+1] Do not degrade JPEG files. #3689

anubhavp28 commented Mar 15, 2019

anubhavp28 commented Mar 15, 2019

anubhavp28 commented Mar 17, 2019

codecov bot commented Mar 22, 2019 •

edited

Loading

Gallaecio commented Mar 22, 2019

Gallaecio left a comment

anubhavp28 commented Mar 27, 2019

anubhavp28 commented Apr 8, 2019

Gallaecio commented Apr 11, 2019

Gallaecio commented Apr 17, 2019

kmike Apr 17, 2019 •

edited

Loading

anubhavp28 Apr 29, 2019

Gallaecio May 3, 2019

anubhavp28 May 7, 2019 •

edited

Loading

Gallaecio May 8, 2019

Gallaecio May 8, 2019

anubhavp28 commented May 16, 2019

Gallaecio left a comment

anubhavp28 commented May 18, 2019

kmike Sep 26, 2019

kmike Sep 26, 2019

kmike Sep 26, 2019

kmike Sep 26, 2019

	elif response_body and image.format == 'JPEG':
	elif response_body is not None and image.format == 'JPEG':

[MRG+1] Do not degrade JPEG files. #3689

[MRG+1] Do not degrade JPEG files. #3689

Conversation

anubhavp28 commented Mar 15, 2019

anubhavp28 commented Mar 15, 2019

anubhavp28 commented Mar 17, 2019

codecov bot commented Mar 22, 2019 • edited Loading

Codecov Report

Gallaecio commented Mar 22, 2019

Gallaecio left a comment

Choose a reason for hiding this comment

anubhavp28 commented Mar 27, 2019

anubhavp28 commented Apr 8, 2019

Gallaecio commented Apr 11, 2019

Gallaecio commented Apr 17, 2019

kmike Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anubhavp28 May 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anubhavp28 commented May 16, 2019

Gallaecio left a comment

Choose a reason for hiding this comment

anubhavp28 commented May 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2019 •

edited

Loading

kmike Apr 17, 2019 •

edited

Loading

anubhavp28 May 7, 2019 •

edited

Loading