Make vips the default variant processor for new apps

[brenogazzola]

Summary

Changes the default variant processor from :mini_magick to :vips.

Other Information

For the full, paragraphs long, explanation about my experiences with both and why I believe vips is the better choice, check my Rails Discussion post.

The abridged version:
I've been running active storage in production since version 5.2 for image galleries. I've noticed that the combination of Active Storage creating variants through an HTTP request in the web server, ImageMagick high resource needs, and servers with low puma concurrency and memory (eg: Heroku), cause increased response times for the entire application.

It took me a long time to understand all this and that using Vips can mitigate the problem. As a form of "conceptual compression", I propose that Rails 7 make vips the default variant processor.

For reference, I've prepared a gist with all the steps I had to go through to migrate from the original mini_magick macros and ImageMagick to the image_processing macros and libvips.

EDIT: Another user in the forum has raised another point: Security. The list of known CVEs for vips is considerably smaller than ImageMagick.

[ghiculescu]

you could roll your upgrade guide PR into this one?

[ghiculescu]

Since upgrading could be a reasonably large job, I think it's worth calling out that it's also fine to keep the current processor and that it won't be deprecated.

[brenogazzola]

In the end I did not open a PR for the migration guide. It evolved into this one. After our conversation I decided that if I felt so strongly about :vips I should go all the way and suggest changing the default and explain why. Otherwise it would not be obvious to others, as it was to me, that this is an actual upgrade.

I've simplified the text here, and added an entry in the "Upgrading Ruby on Rails" guide. However, I'm still unsure where I should put the upgrade guide. Any suggestions?

[ghiculescu]

Could it fit in the upgrading guide, or would it be way too long?

[brenogazzola]

I've added it to the upgrading guide. Not sure how to judge if it's too long or not though. Is there a way to view how this will look like when it's published?

[ghiculescu]

Suggested change

	The `:mini_magick` options is not being deprecated, so it is fine to keep using it.
	The `:mini_magick` option is not being deprecated, so it is fine to keep using it.

[brenogazzola]

@ghiculescu going back to our conversation in #42744 about the "tip" in the overview guide, I'm wondering whether to remove that part about vips being faster. It feels odd to mention that users can get better performance in jpeg transformations by using jpeg-turbo but not mention that one of the applications is dramatically faster than the other.

We could, of course, remove the entire "tip", so I'm wondering how this is usually handled. Should the guides only talk about Rails or are they allowed to make recommendations and give advice on things like this?

[ghiculescu]

clear your fragment caches

how?

[ghiculescu]

(link to relevant docs would be ideal)

[brenogazzola]

I think the easiest would be to advise them to Rails.cache.clear. I've checked the caching guide, but didn't find any reference on how to clear the caches. Did you have any other specific doc in mind?

[ghiculescu]

oh, that's what you meant by fragment caches. I don't think Rails.cache.clear is good upgrade advice.

I believe the cache keys are usually generated based on a hash of the view file's contents. Would that fix things here?

[brenogazzola]

Unfortunately not. The call to .variant might be inside a helper or a model, so the contents of the partial will not change. I agree that Rails.cache.clear is overkill but it's pretty much the only reliable way I've found to ensure all fragments are removed. We might be able to improve this by iterating on all cache entries and only removing those whose keys indicate they are HTML fragments, but I'm unsure of whats the best way to do this.

[brenogazzola]

Actually, "clear your fragment caches" is wrong. An app could have a piece of code like this:

Rails.cache.fetch("#{id}/photos") do
  photos.map{ |photo| photo.file.variant([resize_to_limit: 100]) }.processed.url
end

Either devs do a full cache clear or they hunt each and every instance of variant in their app and clear those specific caches. In my app I had those spread through dozens of templates/fragments so the time it would take was just not worth it.

[ghiculescu]

Right, but we can't really tell people to clear their entire Rails cache. It could be enormous and could hold critical data.

I think the better thing to do here is explain a bit more what's going on and suggest improvements. For example, the cache view helper method takes a list of args; you could pass a version number cache("v2"). That's not a very good solution, I'm open to better ones. Or alternatively we just state the problem and users work it out based on their codebase.

[brenogazzola]

Ok, I hadn’t considered that apps might use caches to hold critical data because to me cache = discardable.

Frankly, I don’t use much caching so I’m out of depth here (as you noticed from my “solution” of doing a full cache clear).

At this point best I can do is the second choice: state the problem, probably in a little more detail, but let users decide how to fix.

[ghiculescu]

did you mean something like this:

- variant(resize: "100x")
+ variant(resize_to_limit: [100, nil])

[brenogazzola]

Ah, much better. I'll fix it.

[ghiculescu]

https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#action-cable-javascript-api-changes

Not sure how to preview the changes, but this looks correct:

[ghiculescu]

Suggested change

	# operations. The `:mini_magick` option is not being deprecated,
	# operations. See the upgrading guide for detail on the changes required. The `:mini_magick` option is not deprecated,

[ghiculescu]

Should the guides only talk about Rails or are they allowed to make recommendations and give advice on things like this?

I'm not sure, I'm going to ask around and hopefully someone more experienced on the Rails team can give some advice here.

[brenogazzola]

A user in the Rails Discussion forums has raised another valid point: libvips has considerably less known CVE's than ImageMagick.

[brenogazzola]

Almost sure the broken test is unrelated. 🤔

[ghiculescu]

Yep, it failed to run. Do you want to rebase + squash? That'll also re run the tests.

[brenogazzola]

Done, but it broke somewhere else this time... 😕

.FileNotFoundError: [Errno 2] No such file or directory: './.buildkite/docker-compose.yml'

[ghiculescu]

🤷 those flakey tests are a bit annoying, but your changes look fine.