Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to transform AS variants in the background #47473

Merged
merged 1 commit into from Jul 3, 2023

Conversation

shouichi
Copy link
Contributor

@shouichi shouichi commented Feb 23, 2023

Motivation / Background

ActiveStorage variants are transformed on the fly when they are needed
but sometimes we're sure that they are accessed and want to transform
them upfront.

Detail

preprocessed option is added when declaring variants.

class User < ApplicationRecord
  has_one_attached :avatar do |attachable|
    attachable.variant :thumb, resize_to_limit: [100, 100], preprocessed: true
  end
end

Additional information

See also #47387.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@hahmed
Copy link
Contributor

hahmed commented Feb 27, 2023

eager_transform: true tbh i'm not really too sure about the naming here.

The variants are being loaded on demand, however they will be pushed to the job queue and processed in due course. In active record terms, when the records are eager loaded they are batched together with the original record(s). This naming is a bit too close to AR.

@shouichi
Copy link
Contributor Author

How about transform_later or process_later?

@shouichi
Copy link
Contributor Author

shouichi commented Apr 5, 2023

@hahmed is there anything I need to do to get this merged? Is renaming eager_transform to transform_later or process_later sufficient?

@hahmed
Copy link
Contributor

hahmed commented Apr 14, 2023

@shouichi transform_later sounds good but maybe even transform: true would work? This is on my list to review, but won't be able to get to it for a couple of weeks unfortunately.

Would need someone from core/committer to review + merge, maybe try discord?

@shouichi
Copy link
Contributor Author

Thank you! I'll share this PR in discord.

@casperisfine
Copy link
Contributor

The feature makes sense to me, but I agree that the naming is weird. Maybe transform_immediately: true?

That said, CI is failing for legitimate reasons.

@shouichi
Copy link
Contributor Author

  • Options hash for variant method are passed to the variant processor. So I want to avoid a collision.
  • I want to make it clear that the processing happens in the background.

So personally I prefer transform_later or process_later. It won't collide with the options for the variant processor (I believe). It's clear that the processing happens in the background because of the naming similarity with ActiveJob's perform_later.

Side note: ActiveStorage document uses the term "transform" and "process". I'm not sure which term to stick with.

@shouichi shouichi force-pushed the background-variants-transformation branch 2 times, most recently from 4505d6e to 8611315 Compare June 30, 2023 08:18
@shouichi shouichi changed the title Add eager transformation of AS variants Add an option to transform AS variants in background Jun 30, 2023
@shouichi shouichi changed the title Add an option to transform AS variants in background Add an option to transform AS variants in the background Jun 30, 2023
@shouichi shouichi force-pushed the background-variants-transformation branch from 8611315 to 9efdbd5 Compare June 30, 2023 08:20
@shouichi
Copy link
Contributor Author

@hahmed @casperisfine I changed the option name to transform_later. Could you take a look?

@casperisfine
Copy link
Contributor

I still think the naming is a bit weird. I do understand the reasoning of using _later to map with Active Job, but what is weird is that the default behavior is already "delayed" as it will be transformed on first access.

Hence why I think something in the lines of schedule_transform: true would be better.

@shouichi
Copy link
Contributor Author

The term schedule gives me the impression that I can schedule the transformation at a specific time.

How about preprocessed: true?

@casperisfine
Copy link
Contributor

How about preprocessed: true?

Not bad. Perhaps pretransform: true ? But may seem a bit weird. I'd be ok with either of these.

@casperisfine
Copy link
Contributor

cc @byroot

@shouichi shouichi force-pushed the background-variants-transformation branch from 9efdbd5 to 4b81552 Compare July 3, 2023 10:35
ActiveStorage variants are processed on the fly when they are needed but
sometimes we're sure that they are accessed and want to processed them
upfront.

`preprocessed` option is added when declaring variants.

```
class User < ApplicationRecord
  has_one_attached :avatar do |attachable|
    attachable.variant :thumb, resize_to_limit: [100, 100], preprocessed: true
  end
end
```
@shouichi shouichi force-pushed the background-variants-transformation branch from 4b81552 to b1c544b Compare July 3, 2023 10:36
@shouichi
Copy link
Contributor Author

shouichi commented Jul 3, 2023

I decided to use preprocessed because there are far more search results than pretransform.

@byroot byroot merged commit c388a4d into rails:main Jul 3, 2023
9 checks passed
@shouichi shouichi deleted the background-variants-transformation branch August 18, 2023 07:32
@tosbourn
Copy link

tosbourn commented Sep 11, 2023

I'm really excited to use this feature; Any idea when it will get merged into stable? I had hoped to see it in 7.0.8

@skipkayhil
Copy link
Member

this feature

New features get released with the next major version, so it will be available in 7.1 (7.0 is bugfix only)

@tomrossi7
Copy link
Contributor

tomrossi7 commented May 23, 2024

@shouichi I am looking to extend this with an option to transform immediately, create_on_attach. Is that something you would use in your ecosystem as well?

@shouichi
Copy link
Contributor Author

Though the apps I manage don't need the feature, I can imagine some apps need one. For example, in a chat-like app, when someone posts an image, many users access the image thumbnail simultaneously. In that case, it makes sense to create a thumbnail before broadcasting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants