-
Notifications
You must be signed in to change notification settings - Fork 0
159 alt text sidekiq job #184
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
Conversation
Rather than serializing the entire, just passes the path to the Job. This is enough for the Job to send to the AltText client.
Upon refresh; live updates will be implemented in #161.
app/jobs/image_alt_text_job.rb
Outdated
| timer = 0 | ||
| until alt_text = client.process_image(tmp_path, prompt: File.read('prompt.txt'), | ||
| model_id: ENV.fetch('LLM_MODEL', nil)) | ||
| sleep OUTPUT_POLLING_INTERVAL | ||
| timer += OUTPUT_POLLING_INTERVAL | ||
| if timer > output_polling_timeout | ||
| update_with_failure(job, 'Timed out waiting for alt-text') | ||
| FileUtils.rm_f(tmp_path) | ||
| return true | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The polling process here can be removed. AltText::Client#process_image immediately returns the generated alt-text once done. It isn't checking for a file like the PDF remediation process.
app/jobs/image_alt_text_job.rb
Outdated
| # File.delete(tmp_path) if File.exist?(tmp_path) | ||
| include AppJobModule | ||
|
|
||
| def perform(job_uuid, tmp_path, output_polling_timeout: OUTPUT_POLLING_TIMEOUT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the output_polling_timeout argument can be removed.
app/jobs/image_alt_text_job.rb
Outdated
| finished_at: Time.zone.now, | ||
| alt_text: alt_text | ||
| ) | ||
| FileUtils.rm_f(tmp_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should put the file delete in an ensure block after the rescue so it always happens, even when there's an error.
app/jobs/image_alt_text_job.rb
Outdated
| job = Job.find_by!(uuid: job_uuid) | ||
| alt_text = client.process_image( | ||
| tmp_path, | ||
| prompt: File.read('prompt.txt'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but it might be safer to include the full path like this:
File.read(Rails.root.join("prompt.txt"))
So it knows where that file is regardless of where a rails process is ran.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
closes #159
This provides the logic for the ImageAltText Job, and uses our own AltText gem to create alt text for whatever we upload. Though the alt text is added to the ImageJob model, live updates are not implemented until ticket 161.