Code

Opened 17 months ago

Closed 14 months ago

Last modified 4 months ago

#19934 closed Cleanup/optimization (fixed)

Switch to a Python 3-compatible imaging library

Reported by: aaugustin Owned by: daniellindsley
Component: Python 3 Version: master
Severity: Release blocker Keywords:
Cc: reinout@…, sj@…, mike+django-trac@…, daniellindsley, chrismedrela Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Originally reported by Marijonas Petrauskas on the django-developers mailing-list


Why isn't Pillow the recommended Django image library yet? PIL has
been unmaintained for almost 3 years and has a number of annoying bugs
(e.g. fails to open some valid JPEG files, maybe has some security
issues as well). Pillow, on the other hand, is a backwards-compatible
community-maintained fork, which has most of those issues fixed and
will even support Python 3 soon.

I think this change would involve (1) running the test suite with
Pillow, (2) updating the documentation and (3) updating the ImageField
warning shown when PIL is not installed.


Marking as a release blocker because of our goal to fully support Python 3 in 1.6.

I don't know if Pillow is the best choice, we should review the alternatives in all cases.

Attachments (0)

Change History (32)

comment:1 Changed 17 months ago by reinout

  • Cc reinout@… added

comment:2 Changed 17 months ago by claudep

I must say that I monitor Pillow for some time, waiting eagerly for its Python 3 support. At my current knowledge, it's the best target for a PIL replacement. Takes this as a +1 from me.

comment:3 Changed 17 months ago by static

I have had many problems using PIL and solved it by swiching to Pillow. Take this as a +1 too.

comment:4 Changed 17 months ago by apollo13

+1 for Pillow, even if it doesn't have Python 3 support (yet).

comment:5 Changed 17 months ago by sjaensch

  • Cc sj@… added

comment:6 Changed 17 months ago by aclark

Pillow's Python 3 support will land in version 2.0. At which point, anyone needing < Python 2.6 should use Pillow < 2.0. I'm not sure if old Python versions are a concern for Django, but I thought I'd mention it.

comment:7 Changed 17 months ago by carljm

Django 1.5 already has Python 2.6 as minimum version, so Python < 2.6 is not an issue.

comment:8 Changed 17 months ago by micsco

  • Cc mike+django-trac@… added

I'm happy to spend some time getting things up to speed here and submit a patch, however is it felt better to wait for 2.0? As that would be a better integration point? (Considering Docs etc)

Last edited 17 months ago by micsco (previous) (diff)

comment:9 Changed 16 months ago by claudep

Yes, I think we should wait for version 2 (Python 3 support) before actually recommending Pillow. But nothing prevents working on a patch now, if you don't mind maintaining the patch (i.e. periodic rebasing) until the time of merging.

comment:10 Changed 16 months ago by claudep

I have successfully ran PIL tests with Pillow 2.0.0 on Python 3.2, except for the _imaging import in from PIL import Image, _imaging.
_imaging cannot be imported from Pillow. This was introduced in [4016d5264a] (#7727) to workaround a PyPy issue. Is this PyPy issue still valid? If yes, is there an other test that we can make which would be both Pillow and PyPy compatible?

comment:11 Changed 16 months ago by russellm

@claudep IIRC, the logic here is that Image is importable under PyPy (because it's pure python), but _imaging isn't (because it's a C module). What is being done here is a way of identifying PyPy by functionality -- if you try to use ImageField under PyPy, an exception will be raised because the _imaging module doesn't exist..

I have no idea what the analog is under Pillow. If Pillow can be executed under PyPy, the issue doesn't exist at all (which gives us another reason to switch to Pillow). If Pillow doesn't work under PyPy, then we'll need a way to identify that at runtime.

comment:12 Changed 16 months ago by claudep

From https://bitbucket.org/pypy/compatibility/wiki/Home:
The Python Imaging Library adds image processing capabilities to your Python interpreter. Since it is not maintained, use the pillow fork instead. Pillow is pypy compatible.

See also https://github.com/python-imaging/Pillow/issues/67

comment:13 Changed 16 months ago by aclark

IIUC you should be able to revert 4016d5264ab049c33c845c5337ce2a3e50754268 since (AFAIK) as of Pillow 2.0.0 we fully support PyPy. If this is not the case, please add a comment to: https://github.com/django/django/commit/4016d5264ab049c33c845c5337ce2a3e50754268

comment:14 Changed 16 months ago by claudep

I don't think we will require Pillow >2.0.0, IMHO we should still support the legacy PIL for Django on Python 2.

comment:15 Changed 14 months ago by aaugustin

To sum up,

1) We must make the following decisions:

1.a) Will we deprecate support for PIL?

On one hand, this would make testing easier and simplify import code — unlike PIL, Pillow is importable from a predictable location.

On the other hand, there isn't much value in forcing this change upon everyone. Since Pillow and PIL can't coexist, it would be a problem for projects that depend on PIL internals (eg. import _imaging), and even more for projects that depend on libraries that depend on PIL internals.

Finally, I haven't found any way to determine if the installed library is PIL or Pillow. We'd need one to show deprecation warnings when PIL is installed.

1.b) Will we keep the error reporting shim introduced in #7727?

Even if we deprecate support for PIL, that'll take two releases, and we need a solution in the mean time to allow using support PIL (CPython only) and Pillow (CPython or PyPy).

Unless we find a way to tell PIL from Pillow, it seems better to remove this check and allow using Pillow on PyPy, even at the cost of less-than-helpful error messages when attempting to use PIL with PyPy. When you're using PyPy, you're supposed to know what you are doing...

(NB: PIL/Image.py appears to contain code to display a nice error when _imaging isn't available, both in PIL and in Pillow. Does #7727 predate this code?)

2) We must make the following changes:

2.a) Modify the documentation to point to Pillow instead of PIL.

2.b) Modify the ImportError messages.

2.c) If the answer to 1.b is "no", revert [4016d526].

2.d) Describe the changes in the release notes. Specifically, mention that it's safer to uninstall PIL before installing Pillow (recommended by Alex Clark on django-developers).


My vote is -0 for 1.a and -0 for 1.b. I'd like to hear more opinions before making a final decision.

Last edited 14 months ago by aaugustin (previous) (diff)

comment:16 Changed 14 months ago by daniellindsley

  • Owner changed from nobody to daniellindsley
  • Status changed from new to assigned

comment:17 Changed 14 months ago by daniellindsley

My vote is +0 for deprecating the PIL (in that Pillow becomes the "blessed" version), but I think detecting which is installed & using shims to gloss what little Django uses of either is a very doable task. By my count, the PIL/_imaging is only used in 4 files in Django (plus lots of test files) & none of the usages are particularly advanced.

I'm also +0 on the error reporting shim, in that I think we need to be able to inform the user that their install may be mis-configured. The existing comment (& the detection going on) will have to change. Again, I'm of the belief we can shim over that & have some ideas on detection.

The only thing I'd add to the plan you've laid out is """2.a.1) Create any necessary shims to aid Django in detecting which, if any, are installed & to aid in providing useful error messages about the installation."""

comment:18 Changed 14 months ago by aaugustin

That works for me, I'm happy to review a patch along these lines.

comment:19 Changed 14 months ago by daniellindsley

I think I have pretty much everything implemented, at least to start with. PR is at https://github.com/django/django/pull/1059. Feedback would be appreciated.

comment:20 Changed 14 months ago by daniellindsley

@aaugustin - One thing I missed was 2.d. I'm not actually sure where that documentation ought to go. Thoughts?

comment:21 Changed 14 months ago by daniellindsley

  • Cc daniellindsley added
  • Has patch set

comment:22 Changed 14 months ago by aaugustin

For 2.d, I was thinking of a paragraph in the release notes, probably in the backwards-incompatible changes section.

comment:23 Changed 14 months ago by Daniel Lindsley <daniel@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 33793f7c3edd8ff144ff2e9434367267c20af26a:

Fixed #19934 - Use of Pillow is now preferred over PIL.

This starts the deprecation period for PIL (support to end in 1.8).

comment:24 Changed 12 months ago by chrismedrela

  • Cc chrismedrela added

I'm working on my GSoC project and I need to be sure what kind of exceptions may be raised while importing django.utils.image. Is it possible to end with an ImportError? There is one import statement which is not wrapped in try-except clause https://github.com/django/django/commit/33793f7c3edd8ff144ff2e9434367267c20af26a#L3R133. If it may fail, we should wrap it in try-except blocks and reraise as an ImproprelyConfigured exception. If not, then adding a comment why it can't happen would clarify the situation.

For me it looks like it can't happen. If line 95 or 99 succeed then respectively line 131 or 133 must succeed too; otherwise an ImproperlyConfigured exception is being raised (line 102). Nonetheless, I'm not sure and I would like to hear your opinion.

comment:25 Changed 12 months ago by daniellindsley

@chrismedrela - I'm not aware of a way that import can fail without a completely broken Pillow/PIL install (missing Python modules, not just a missing C compiler). It was also directly moved from https://github.com/django/django/commit/33793f7c3edd8ff144ff2e9434367267c20af26a#L0L38, so it would've been failing that way before. AFAICT, that shouldn't ever fail, and if it does, we should be raising an ImportError so the user knows what's wrong.

comment:26 Changed 12 months ago by chrismedrela

@daniellindsley - Thank you for your answer. I created a pull request clarifying the situation https://github.com/django/django/pull/1349.

comment:27 Changed 12 months ago by Christopher Medrela <chris.medrela@…>

In b4c61c2665c0aa06dcdc4f823be03f23b9696408:

Added clarification comments to django.utils.image; refs #19934

comment:28 Changed 12 months ago by Tim Graham <timograham@…>

In b82a2c41387648f81387b1e03371331db3630269:

Merge pull request #1349 from chrismedrela/ticket19934-comment

Added clarification comments to django.utils.image; refs #19934

comment:29 Changed 11 months ago by Simon Charette <charette.s@…>

In b9590a69357917a3dbf2c7234774803bd43767ef:

Correctly format missing Pillow/PIL exceptions messages. refs #19934

comment:30 Changed 11 months ago by Simon Charette <charette.s@…>

In e7a6eaf5fee9c5854e0d2d86f21132ac4a1415f6:

[1.6.x] Correctly format missing Pillow/PIL exceptions messages. refs #19934

Backport of b9590a6935 from master.

comment:31 Changed 11 months ago by Andrew Godwin <andrew@…>

In e9b703f5a5e5de68a28abd2e6651e6100b0c6b49:

Correctly format missing Pillow/PIL exceptions messages. refs #19934

comment:32 Changed 4 months ago by Tim Graham <timograham@…>

In 4965a774074780f3e4858bcc975476f71edf2c2c:

Removed PIL compatability layer per deprecation timeline.

refs #19934.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.