In this PR: Fix #8307 -- Saving a model with an ImageField with width_field or height_field no longer results in an extra read operation by john-parton · Pull Request #18070 · django/django · GitHub
Based off this ticket: #8307 (ImageFile use of width_field and height_field is slow with remote storage backends) – Django (djangoproject.com)
There is a behaviour change proposed without a deprecation but detailed in the release notes as:
django.db.models.ImageField no longer re-reads the file from the storage backend after width_field and height_field are set. If your storage backend resizes images, the actual image’s width and height will not match the auto-populated fields.
I know this is sometimes the right way to go, but it’s not clear to me when it is and isn’t and if we were to try to deprecate the previous behaviour in this case I am not sure how we would do it.
I would love to hear your opinions on this case, then if you want to share any examples for my understanding that would also be really interesting.
Hey Sarah 
So… to almost certainty, if we add a breaking change, that’s going to affect folks in production, and there will be reports about it after the release.
Does that mean we don’t do it? No. Look at every release where there’s a Miscellaneous breaking changes section in the notes. (Reviewing those for the last few versions would give you the best ideas of what counts I’d suggest.)
Is there a deprecation path available? If so we should take it.
Otherwise, is there a quick adjustment that resolves it? If so, a misc. breaking change may be OK.
Otherwise, it needs to be worth it to justify. Folks are very keen on Django not breaking their projects, and we want folks to update. (Not breaking stuff is almost a USP.)
If it’s a pain to work around a breaking change, it needs to be called out clearly, with an extended discussion, near the top of the release notes.
That’s roughly it, I think 
Pin that Carlton gold. 
How many storage backends resize images? Do we know of any?
IMO the storage layer is the wrong place to do resizing, or any other file processing like optimization. If you’re resizing an image, you probably also care about saving the original copy and one or more resized variants. In that case, you probably want the ability to store multiple ImageFields, maybe on the same model or several instances related by M2M.
1 Like
Thank you both 
In this case, in order to deprecate, I would consider introducing a setting flag (to turn off the changed behaviour) but I also know settings are considered quite controversial.
However, perhaps temporary settings to enable deprecation paths are not considered controversial?
cc @john-parton
I don’t know of any but that’s not saying much. John might have thoughts here.
I think @codingjoe maintains django-pictures and might be able to add to/generally be interested in this discussion. Don’t worry (and also sorry) if not 
Yes… so introducing a transitional setting, that is then immediately deprecated, is the way here. That’s fine.
The issue with settings is ending up with the nuclear reactor control panel…
… Just one more knob.
With a transitional setting, we’re not keeping it, and there’s nowhere it could be better encapsulated, so if necessary…
A couple of Examples:
1 Like
Alright, strap in, I guess?
First, off the bat, regarding deprecation: I can easily add a setting to opt into this behavior. It would be one setting and literally one block of guarded code. Not bad.
I think it’s important to keep in perspective which storage backends don’t resize images:
- None of the first party storage classes: File storage API | Django documentation | Django
- None of the storage classes defined by django-storages: django-storages — django-storages 1.14.2 documentation
So for any user of these storage classes, there will be no breakage.
I found the issue because I created a storage backend to directly upload images to Cloudflare Images. Here’s a repo that implements a similar storage backend to what I ended up implementing: GitHub - KalvadTech/django-cloudflare-images: Django library to add Cloudflare Images support for ImageField
This is maybe a somewhat philosophical or academic question, but what’s the difference between a storage backend that resized images immediately upon save, and a storage server that has a cron job to resize images periodically? With the way django works today, in the former case you get a double-read and the “correct” on disk size, but in the latter case you get a double-read with the “original” size, only for the binary file to be changed later, rendering the values in the database to be incorrect.
My general argument is that my fix actually puts the code in line with what users expect is happening already, and just isn’t. Secondly, the performance penalty is bad. For a remote storage backend that has slow reads, but fast writes (CloudFlare images), it means that saving is 2-3x slower than without the fix.
I’m happy to add the setting and a clearer deprecation path if that’s the consensus. Just my two cents.
One additional thing: If we add a setting, we should probably add it to the output of startproject with the new behavior avoiding the double-read.
If someone is using the width and height fields, they obviously care about performance. There’s literally no reason to use it other than performance. It’s a pure performance optimization. I think tanking performance when users opt into a performance feature is pretty obviously nonsense.
If we did have a setting, it should be opt-out (i.e. set it to keep the old behaviour.)
It may be that this can just go in Misc Breaking Changes if the consequences are small enough.
(I’m not deep enough in to really see that in this case.)
Sometimes the deprecation policy can seem like overkill, but it’s not a big ask often, and sticking to it has helped Django and Django’s users many times.
There’s an alternate suggestion versus adding an opt-out setting: We can just document what is required to restore the old behavior. An image subclass or a simple code snippet would likely work. Then users can have more granularity in when they want to apply the old behavior, perhaps only applying it to some fields and not others.
If it’s a simple thing, then maybe that’s the way. (I’d suggest discussing concrete proposals on the PR… — “how about this?” goes a long way. 
I was summoned here. Do you want me to summarize this discussion on the PR?
I don’t want anything in particular .
As I understand it, Sarah asked about when a breaking change is OK. Points were given in general. (I took Sarah’s CC of you to bring those to your attention.)
It sounds like in this case either a deprecation with a transitional setting, or a misc breaking change with docs are the reasonable options.
Of those two, I’d say the default is the deprecation with a transitional setting.
But you might discuss it with the Fellows (and others) on the PR to the extent that (maybe) the breaking change with docs is sufficient in this case. (Likely you’re quite right: I don’t have the bandwidth to get up to speed to say is all.)
My suggestion would be that only you and those closely involved in the PR can make the final call there, so that’s where I’d continue on the fine details.
Correct, I am not experienced in deprecating things and wanted general advice. I also wanted to apply that advice to this specific PR.
@carltongibson thank you, I appreciate the guidance
I did move this discussion from the PR to the forum, as I wasn’t sure about and wanted more eyes on the deprecation decision (I find that more people browse the forum than open PRs).
Yes I think let’s do this. If this is roughly as painless as them using a setting, then we should go for this.
I would say if anyone lands here, they’re welcome to challenge this approach.
1 Like
Cheerio ,
Thanks for pinging me about this. djanog-pictures discourages touching the original file. This goes for the deprecated django-stdimage as well. For django-s3file You never know what future variations of a file you might need. It’s good to make copies of the original source only. JPEG, WebP, AVIF things are always progressing.
I’d personally welcome it, it would improve performance for django-s3file. We did add some dark magic there, to avoid reading files from the storage all together. It’s all fun, until someone uploads a 10 GB animated AVIF and your app-server grinds to a halt.
TL;DR: go for it
Cheers,
Joe
4 Likes
