Move machine translation processing to Celery (Backend) - #4504
Move machine translation processing to Celery (Backend)#4504hannaseithe wants to merge 14 commits into
Conversation
There was a problem hiding this comment.
This is a preliminary review, I have not done any extensive testing yet.
The general implementation seems really good, I've added a few note and lots of questions here and there.
I also have a general question:
We both set the currently_in_machine_translation on the translation row and acquire a Redis lock on the (content_type, object_id, language_slug) tuple. Wouldn't it suffice to just apply the former? If it is for the case of a "not yet existing" translation, could we solve this by creating a (locked) translation row, with an empty content? It would be cleaner to use a nullable column, but that would be a larger change.
If possible, I would like to avoid a situation, where the cache and the DB disagree, specifically since the Redis persistence is not 100% reliable. I like to treat Redis as a pure cache that can go away any time.
| if unix_socket := os.environ.get("INTEGREAT_CMS_REDIS_UNIX_SOCKET"): | ||
| default_celery_redis_url = f"redis+socket://{unix_socket}" | ||
| else: | ||
| default_celery_redis_url = "redis://127.0.0.1:6379/0" | ||
| CELERY_BROKER_URL = os.environ.get("CELERY_REDIS_URL", default_celery_redis_url) | ||
| CELERY_RESULT_BACKEND = CELERY_BROKER_URL |
There was a problem hiding this comment.
❓ It is a bit hard to follow, how the environment variables interact. There is REDIS_CACHE, INTEGREAT_CMS_REDIS_UNIX_SOCKET, CELERY_BROKER_URL and CELERY_RESULT_BACKEND.
How about a single CELERY_BROKER_URL and maybe an additional CELERY_RESULT_BACKEND if this might ever be different from CELERY_BROKER_URL
There was a problem hiding this comment.
Salt only sets INTEGREAT_CMS_REDIS_UNIX_SOCKET once, and it configures both the Django cache and Celery from that single value. Using CELERY_BROKER_URL / CELERY_RESULT_BACKEND directly would require repeating the socket path in two separate salt env vars, which is more likely to get out of sync. The CELERY_REDIS_URL override exists as an escape hatch if the two ever need to diverge, but in practice it's never set. So I deleted that.
There was a problem hiding this comment.
Just a remark: the previous settings architecture with CELERY_REDIS_URL allowed to point to a remote Redis. We now hardcode to localhost. That is fine for me, we don't need to over-engineer this. But maybe we should be explicit that at this point, integreat-cms basically needs a local redis up and running to function properly.
We can have a discussion if we want to make redis a hard requirement. But we are currently introducing more and more features that fail silently if redis is not running, including this one: With no redis, the async machine translation happily writes into the local memory cache, and nothing ever consumes it. I'd prefer things to break loudly, either on startup or when the jobs are queued and redis is not up.
There was a problem hiding this comment.
I would propose to enable a system where we can point to a remote Redis instance. The reason is that I want to run Redis in a docker-compose setup and for this I need to specify a host name. And yes, I would absolutely make it a requirement. We should always strive to be as close to the production environment as possible, and it is very easy to do with Redis.
There was a problem hiding this comment.
I made redis a hard requirement now (when REDIS_CACHE is enabled) and it also let's you set INTEGREAT_CMS_REDIS_HOST and INTEGREAT_CMS_REDIS_PORT
Thank you very much for bringing this up, I had considered these questions before, but I have gone even more in depth this time: My conclusion so far is that we actually do need the two. There is a follow-up task (#4512) to clean up any stale states regularly, which is intentionally scoped separately to keep this PR focused. Let me try to present my thoughts on the different scenarios: Dropping the redis lock and keeping (some form of) the DB flag:Overall I would say that your concern about redis reliability is valid (I adress it more further below), but on the other hand using redis for distributed locks is a well-established pattern. Even Celery's own cookbook recommends using cache.add() for task locking with an atomic cache backend — which is exactly what we do, using Redis as that backend. Furhtermore the redis lock does two things the DB can't:
On dropping the DB flag in favour of the Redis lock onlyYour concern about Redis reliability is valid, but the different failure scenarios play out differently: Redis crash (no auto-restart configured): The Celery task will fail almost immediately — update_state() writes task progress to Redis (which is also the result backend), so the task crashes at the next progress update. By the time Redis is manually restarted, the task is already dead. A user retrying is the correct behaviour — and importantly, a stuck DB flag in this scenario would actually block the legitimate retry, since the flag-clearing code at the end of the task never ran. So the DB flag makes this scenario worse, not better. Cache wipe during deployment: This is the real scenario where the DB flag earns its place. cache_signals.py calls cache.clear() on every post_migrate signal. Looking at the Salt deployment config, the Celery worker restart and the migration step are not strictly ordered — the worker can still be running with in-flight tasks when migrate fires and wipes the cache. Redis stays up, the task keeps running, but the lock is gone. A user seeing the spinner, getting impatient, and clicking translate again would successfully queue a duplicate task. The DB flag catches exactly this. Conclusion:The two mechanisms solve genuinely different problems — the Redis lock for atomic mutual exclusion and task ID tracking, the DB flag specifically as a safety net against cache wipes during deployment (a concrete, non-hypothetical scenario given the existing cache.clear() on post-migrate). Moving away from the Redis lock would require either accepting the loss of language granularity, polluting the translation model with placeholder rows, or building a Postgres-based equivalent of what Redis already does natively. I'd suggest filing the DB-only design as a follow-up issue rather than blocking this PR on it — it deserves proper scoping rather than being squeezed into an already substantial PR. |
3956586 to
49017b6
Compare
andrew8er
left a comment
There was a problem hiding this comment.
Thanks for the reply. I mainly wrote this question with my perspective of using the least amount of external services in critical paths. Also, in my experience, when the posibility of data drift exists, it will eventually happen, and usually in a very non-obvious way. I also have little trust in Redis (the company) and Celery as a task queue solution tbh.
- Preventing triggering the sam translation task twice under race conditions.
cache.add()prevents duplicate translations in a single atomic operation. A DB-based mutex requires SELECT FOR UPDATE, which only works if the translation row already exists (as you mentioned) — but for a first-time translation there's no row yet. Creating a placeholder row to lock on introduces its own race (two concurrent requests can both find "no row exists" and both try to insert) and pollutes the translation model with rows that aren't real translations — every place that queries translations would need to be aware of and filter out these placeholders, which is a significant blast radius.
That is my real concern: a lot of places would need adjustment, but since we use an ORM, the places where we have these queries should be easily identifiable.
The premise of this objection is not true, however: Postgres can easily handle conflicting inserts with INSERT … ON CONFLICT DO <action> (docs).
Task ID storage, scoped by language. The lock stores the Celery task ID as its value, keyed by (content_type, object_id, language_slug). This is what enables progress polling — get_mt_task_ids() batch-fetches task IDs for all objects in a list view in one Redis round trip. The language scoping is essential: page 28 can be translated into English and Arabic simultaneously as independent tasks.
Moving the task ID to the DB requires preserving this granularity:
- A nullable field on the content model loses the language dimension entirely — one field can only track one in-progress language per object.
- A nullable field on the translation model hits the "no row yet" problem for first-time translations.
- A separate (content_type, object_id, language_slug) → task_id table would essentially reimplement the Redis lock in Postgres — same semantics, more latency, more moving parts.
A nullable field on the translation model has the "adjust all queries" problem, but adequately replicates the functionality for which we now use Redis locks. Postgres can of course fully support atomic reads and writes with conflict management (it is just not that good for high volume inserts/updates, but at our scale, this does not constitute a relevant factor IMHO). What we have now is basically the third option ("A separate (content_type, object_id, language_slug) → task_id"), but in another DB.
We also have the problem that translation tasks' reports are now queued into Redis (with a TTL), but they might get evicted before the user had a chance to see them. A pre-existing row for every translation task could solve this as well.
I'm not completely against the current solution, but I would like to present the alternatives and seriously consider their trade-offs.
| return any(object_report.get("failed", {}).values()) | ||
|
|
||
|
|
||
| def _get_report_outcome(results: dict[str, dict[str, dict[str, Any]]]) -> str: |
There was a problem hiding this comment.
How about just returning a bool then?
|
|
||
|
|
||
| @shared_task(bind=True) | ||
| def start_async_translation( |
There was a problem hiding this comment.
💭 Overall, this function is too long and does too much imperative data manipulation (i.e. for loops), where a list/dict comprehension would be more apt.
There was a problem hiding this comment.
I think I was pretty aware of it while writing it and then I chose to ignore. I will fix it :) But comprehensions dont really work in most places (but one) I think, because of side effects, I will just do decomposition.
jonbulz
left a comment
There was a problem hiding this comment.
As there are unresolved conversations and an ongoing discussion that could potentially result in substantial changes (keep redis lock vs implement a postgres solution), I don't want to be too detailed with my review yet. However, I want to bring up a few things that I did not see mentioned yet:
- release lock on aborted translation tasks: to my understanding, nothing currently removes the lock if the translation task throws. maybe wrap the task body in
try/finallyand/or give the lock a finite TTL? the TTL part obviously only applies if we stick to Redis, but we need a cleanup either way - the single object edits (
PageFormView.post,EventFormView.post,POIFormView.post) are wrapped intransation.atomicblocks. the task is dispatched before the transaction commits, so a celery worker could read a stale source. also, if the transation is rolled back, the task still executes. better to go throughtransation.on_commit(...)? - the feature silently fails without Redis, see my other comment
My 2 cents on the Redis vs Postgres discussion: I think either solution works. What's important to me is that stuff breaks loudly, and that we don't end up with semi-permanent transient states that require manual fixing. There are also steps we could take to make Redis more durable, if that is a concern.
| if unix_socket := os.environ.get("INTEGREAT_CMS_REDIS_UNIX_SOCKET"): | ||
| default_celery_redis_url = f"redis+socket://{unix_socket}" | ||
| else: | ||
| default_celery_redis_url = "redis://127.0.0.1:6379/0" | ||
| CELERY_BROKER_URL = os.environ.get("CELERY_REDIS_URL", default_celery_redis_url) | ||
| CELERY_RESULT_BACKEND = CELERY_BROKER_URL |
There was a problem hiding this comment.
Just a remark: the previous settings architecture with CELERY_REDIS_URL allowed to point to a remote Redis. We now hardcode to localhost. That is fine for me, we don't need to over-engineer this. But maybe we should be explicit that at this point, integreat-cms basically needs a local redis up and running to function properly.
We can have a discussion if we want to make redis a hard requirement. But we are currently introducing more and more features that fail silently if redis is not running, including this one: With no redis, the async machine translation happily writes into the local memory cache, and nothing ever consumes it. I'd prefer things to break loudly, either on startup or when the jobs are queued and redis is not up.
|
The thing is I have not implemented a lock like this before, so I cannot rely on my own experience here. From what I read redis is a legitimate way to store semi-permanent state like locks, reports and progress state. In regards to the "permanancy" of state: With our setup a graceful restart of redis will persist state before restart, a hard crash would make us lose state from up to one hour before crash. We already use both redis and celery and I dont see how that will change. So the argument of a general distrust against those as external, additional software does not apply; that dependency already exists. There is an actual bug in the code base (authored by me), where on every post_migrate we wipe the whole redis cache clean. This needs to be fixed especially for the reports that are stored on redis (as they don't have a DB fallback and are expected to persist for longer than the task runs) On the DB flag vs. Redis lock specifically: I see real arguments on multiple sides and haven't fully settled on one. I will try to collect my thoughts here: I personally agree that having both a DB lock and a redis lock might hold the risk of state divergence. Even though I feel that the follow up task to regularly clean up possible stale flags + locks should mitigate that. I think it is legitimate to keep it if we actively work against the drift. The flag basically serves as a performance improvement where we check on already queried rows if the flag exists and only fall back to the redis lock when the translation does not exist yet (get_translation_state() in abstract_content_model.py). If we should get rid of one, I personally prefer though to drop the DB flag and keep the redis lock, as I understand the redis cache to be a legitimate storage place for this kind of semi-permanent state. If we go with the DB lock only route though, I would strongly argue against any placeholder row approach in the translation models. But then actually chose to mirror the redis_lock structure in a table (content_type + object_id + language_slug + task_id) and get rid of the flag on the translation model. My suggestion is to wait and hear what a second reviewer who knows the code base better than you and me, says I see three options:
|
I have spent a lot of time already trying to weigh the pro and cons of any of the three options. And from my perspective the question of redis vs postgres for this kind of semi-permanent states looks more like a question of preference than necessarily one being better than the other. Therefore I am making a decision now: I will keep things as they are with the redis-lock plus the flag. For me this means three things: A) Make sure that stale states on the locks do not happen I hope you are ok with this decision for now @andrew8er. I am not saying this is permanent. But I would suggest we go with this and see how it works out. I will also write an ADR that documents the pro and cons of this decision vs the other options, so that it will be easier in the future to chose a different option if necessary. |
No worries, I did not intent to challenge Redis or Celery. I'm fully aware, that this ship has long sailed. I just wanted to express that if Redis Inc pulls off another of their license shenanigans, we might need to switch to Valkey (should not be a big deal), and expect Celery workers to fail at some point, so the chance of a data drift is definitely there.
I'll have a look at the PR later.
To me it is a legitimate place, my concern is more with having data that describes the same fact be placed in two places.
I'm fine with that. But let's be real here, we will not change this setup anytime soon after it is implemented. It will mostly work and the problems that might arise might be a little bit annoying, but they will not justify a refactor. This will be permanent for the forseeable future. |
- Tighten mark_unsuccessful/mark_too_long type from Any to ErrorDict | Exception - Inline _format_message body, removing intermediate variable - Remove lazy parameter from all get_*_message methods — always evaluate eagerly since the synchronous alert path no longer exists - Remove unused CELERY_REDIS_URL env var override in settings - Improve comments on MT_SUPPORTED_LOCK_TYPES, ready() in DeepLApiClientConfig, and get_mt_task_ids() docstring
0352010 to
055e5f3
Compare
b1a4e8b to
0fb81ec
Compare
e74267f to
4f89b7f
Compare
|
I would kindly ask you to re-review. I have worked through all your comments and tried to implement everything as discussed.
|
I implemented all these three |
Short description
This PR provides the logic to run machine translations through Celery instead of synchronous execution. At the core is the Celery task
start_async_translation. The asynchronous execution has been wired up to all former synchronous call sites in the GUI. Furthermore, groundwork has been laid for reporting on progress and result in the frontend (see follow-up PR) by creating two polling endpoints.Proposed Changes
1. Locking and queueing (
core/utils/machine_translation_celery_task.py)queue_translations(request, user_id, region_id, content_type, object_ids, language_slugs)is the new single entry point that every call site now goes through instead of talking to a provider client directly:(content_type, object_id, language_slug)pair in the batch, viacache.add(key, task_id, timeout=None)(acquire_locks).cache.addonly succeeds if the key doesn't already exist, so this is an atomic "claim or fail" per key. If any key in the batch is already locked, all locks acquired so far are released and the whole request is rejected with an error message- no partial-batch locking, no partially-started translation.currently_in_machine_translation = Trueon the affected translation rows synchronously, before the task is even queued. This has to happen here rather than at the start of the task itself: the task only starts once a Celery worker actually picks it up, some indeterminate time later, which could easily race with the very next page render after the redirect.start_async_translationviaapply_async, passing the requesting user's id and their active language explicitly (get_language()) — the task has no request to read this from, and needs it to render error messages in the right language.2. The task itself (
start_async_translation)content_type(page/event/poi/pushnotification→ the corresponding*TranslationForm), imported lazily inside a function to avoid a circular import (these form modules importqueue_translationsfrom this same module).mock_request(a bareHttpRequestwith.user/.regionset) so the existing provider API clients — which expect a request — can be reused unmodified.language_slugs × content_objects. One API client instance is created per provider and reused across the whole batch (clients_by_provider), rather than once per object.self.update_state(state="IN_PROGRESS", meta={...progress...})after each object, which is what the new progress endpoint (see below) reads back.translate_queryset) are caught, recorded into atranslation_reportdict, and the loop continues — one bad object/language doesn't abort the rest of the batch.currently_in_machine_translationflags.get_translation_state()to check, so it would otherwise fall back to checking the still-present lock and wrongly report "in progress" forever.invalidate_cached_translations()on every object — thecontent_objectsqueryset instances are the same ones used throughout the loop, so their cached translation state would otherwise still reflect whatever was cached before the task ran.pages_datadict (translation state + title/slug/status/last_updated per object/language) and pushes a report viaqueue_mt_report.{"progress": 1.0, "pages": pages_data}dict rather than callingupdate_state(state="SUCCESS", ...)directly — returning normally is what Celery treats as the actualSUCCESSresult, overwriting anything set viaupdate_statebeforehand.3. Two new read endpoints, for two different questions
GET .../translation-task-progress/<task_id>/(machine_translation_progress.py) — "how far along is this specific Celery task?", read viaAsyncResult(task_id). Meant for callers that already know which task governs a whole batch of objects (e.g. a content list view resolves the task id once at render time for a group of rows, then polls that one task instead of checking each row).GET .../translation-report/(machine_translation_report.py) — "what finished for me, that I haven't seen yet?", read via a per-(user, region, content_type)cache key (get_mt_report_cache_key). This is a destructive read: fetching the reports also deletes them from the queue (7-day TTL as a safety net if nobody ever asks)._get_report_outcome/_get_report_messageturn the raw per-object report into a simpleFULL_SUCCESS/PARTIAL_SUCCESS+ translated banner text, so the frontend doesn't have to interpret the raw report shape at all.Both endpoints are permission-checked (
cms.view_{model_type}) and registered incms/urls/protected.py.4. Data layer: a live "in progress" state, not just a flag
currently_in_machine_translationBooleanFieldon all five translatable models (EventTranslation,ImprintPageTranslation,PageTranslation,POITranslation,PushNotificationTranslation) - migration0159.translation_status.MACHINE_TRANSLATION_IN_PROGRESSconstant.AbstractContentModel.get_translation_state()now also checksget_machine_translation_task_id()- i.e. it checks the lock directly, not the flag. This matters because a translation being created into a language for the first time has no row yet to hold the flag on at all, but the lock still exists and correctly reports "in progress" regardless.get_mt_task_ids(content_type, object_ids, language_slugs)batch-resolves the lock lookup for many objects at once in a singlecache.get_many()round trip.PageTreeView/partial_page_tree_view.pynowcompute this once per render and pass it down (mt_task_idsin the template context) instead of hitting Redis per row.get_translation_state,get_machine_translation_task_idincontent_filters.py) expose the same thing to templates, accepting the same optional precomputed lookup.content_edit_lock.pyexposes the equivalent status for the edit-mode heartbeat: whether the specific object/language currently open for editing is itself mid-translation, and separately, whether a translation into one of its child languages was triggered from the language currently being edited (_get_active_child_mt_task_id- deliberately does not filter by the child's currentmt_providerconfig, since "is something already running" shouldn't depend on whether that config would still permit starting a new one today). The frontend PR uses both signals to decide whether to lock the editor outright or just show a non-blocking banner.None of this data is rendered anywhere yet in this PR — it's the context/data plumbing the frontend PR's templates and JS consume.
5. Wiring at the four synchronous call sites
Every former direct call to a provider API client was replaced with a call to
queue_translations:MachineTranslationForm.save()(single-object, editor "auto-translate" checkboxes)PushNotificationTranslationForm.save()(same, for push notifications)BulkMachineTranslationView.post()(bulk action from a list view)I found no other remaining call sites — grepped the whole codebase for
.translate_queryset(/.translate_object(and found no callers left outside the API client and Celery task modules themselves.6.
machine_translation_api_client.py: decoupling "build the message" from "show the message"The old
alert_*methods (alert_successful_translations,alert_failed_translations, etc.) both computed a message and calledmessages.success/error(self.request, ...)in one step. The Celery task has no request to attach a Django message to, and needs a message it can store in the report dict right now, in the requesting user's language — not a lazy translation proxy resolved later at render time.Each
alert_*method is now split into aget_*_message(lazy: bool = True)that just builds and returns the string (lazy by default, for the old use case;lazy=Falseforces immediate translation, used by the Celery task'sget_language_report()), plus a thinalert_*wrapper that posts it as a Django message as before.Note for reviewers: I grepped for callers and found that
alert_messages()and all the individualalert_*wrapper methods are now unused anywhere in the codebase (including tests) — the only consumers left are the newget_*_message()methods via the Celery task. Worth deciding whether to remove them in this PR or leave them as public API for a future synchronous caller.7. Provider app config:
ready()doesn't mean "the process is up" under Celerydeepl_api/apps.pyandgoogle_translate_api/apps.pyeach check API availability (fetch supported languages, verify credentials/usage limits) once, inAppConfig.ready(). That previously assumedready()firing meant the server process was fully up — true forrunserver/Apache, but under Celery,ready()fires while the worker is still bootstrapping. The check is now deferred to Celery'sceleryd_after_setupsignal in that case, and still runs immediately as before otherwise.8. Incidental fix:
machine_translations.pybuild_json_for_machine_translation(word-count endpoint used to build the "auto-translate" checkboxes) unconditionally calledget_translatable_attributes/word_counteven when there's no source translation to translate from. Guarded that behindif source_translation:, since that object is routed into thenon_translatablebucket regardless — this isn't strictly a Celery change but was needed to exercise this endpoint under the new async paths without crashing.9. Bug found and fixed while rewriting
mt_apitestsRewrote
tests/mt_api/*to assert against the newmachine_translation_reportendpoint instead of Django messages/logs, since outcomes are no longer available synchronously. While making sure no previously-asserted error path got lost in that switch, found that_get_report_outcomeonly checked for a top-level"exception"key to detect a failure — butget_language_report()'s shape reports a failure via a populated"failed"dict instead, whenever the provider client catches its own error internally (e.g.mark_unsuccessful()/mark_too_long()) rather than raising. A batch that failed entirely this way (e.g. every object hit a DeepL rate limit) was silently reported asFULL_SUCCESS. Fixed via_object_report_has_failure, with a regression test (test_report_outcome_partial_success_for_failure_reported_via_failed_dict).Side effects
Faithfulness to issue description and design
This PR only implements the backend logic
How to test
Resolved issues
Fixes: #4393
Pull Request Review Guidelines