DEV: Backfill per locale per model instead of just per model (#243)

nattsw · web-flow · commit c953291712a4 · 2025-03-06T22:55:16.000+08:00
Currently backfilling very inefficiently, as we were querying by a single Topic or Post as long as it does not have any of the target languages (based on an array rather than a single locale).

This commit splits the backfill job into per locale rather than grouping all the locales together and finding which topics have missing translations. e.g. from the log item below, you can sort of tell that 18 topics are "shared" between all the languages, even if the language already has the translation.
```
DiscourseTranslator: Translating 18 topics and 19 posts to en, zh_CN, es, fr, de, pt_BR, it, ar, he
```

It does potentially end up loading a topic multiple times, but the alternative is a much slower backfill if new languages get added to the target languages.
diff --git a/app/jobs/scheduled/automatic_translation_backfill.rb b/app/jobs/scheduled/automatic_translation_backfill.rb
@@ -18,35 +18,33 @@ def execute(args = nil)
       end
     end
 
-    def fetch_untranslated_model_ids(
-      model,
-      content_column,
-      limit,
-      target_locales = backfill_locales
-    )
+    def fetch_untranslated_model_ids(model, content_column, limit, target_locale)
       m = model.name.downcase
-      DB.query_single(<<~SQL, target_locales: target_locales, limit: limit)
+      DB.query_single(<<~SQL, target_locale: target_locale, limit: limit)
         SELECT m.id
-        FROM #{m}s m
-        LEFT JOIN discourse_translator_#{m}_locales dl ON dl.#{m}_id = m.id
-        LEFT JOIN LATERAL (
-          SELECT array_agg(DISTINCT locale)::text[] as locales
-          FROM discourse_translator_#{m}_translations dt
-          WHERE dt.#{m}_id = m.id
-        ) translations ON true
-        WHERE NOT (
-          ARRAY[:target_locales]::text[] <@
-            (COALESCE(
-              array_cat(
-                ARRAY[COALESCE(dl.detected_locale, '')]::text[],
-                COALESCE(translations.locales, ARRAY[]::text[])
-              ),
-              ARRAY[]::text[]
-            ))
-        )
-        AND m.deleted_at IS NULL
-        AND m.#{content_column} != ''
-        AND m.user_id > 0
+        FROM #{model.table_name} m
+        WHERE m.deleted_at IS NULL
+          AND m.#{content_column} != ''
+          AND m.user_id > 0
+          AND (
+            NOT EXISTS (
+              SELECT 1
+              FROM discourse_translator_#{m}_locales
+              WHERE #{m}_id = m.id
+            )
+            OR EXISTS (
+              SELECT 1
+              FROM discourse_translator_#{m}_locales
+              WHERE #{m}_id = m.id
+              AND detected_locale != :target_locale
+            )
+          )
+          AND NOT EXISTS (
+            SELECT 1
+            FROM discourse_translator_#{m}_translations
+            WHERE #{m}_id = m.id
+            AND locale = :target_locale
+          )
         ORDER BY m.updated_at DESC
         LIMIT :limit
       SQL
@@ -65,53 +63,60 @@ def secure_backfill_lock
     end
 
     def translations_per_run
-      [
-        (SiteSetting.automatic_translation_backfill_maximum_translations_per_hour / 12) /
-          backfill_locales.size,
-        1,
-      ].max
+      [(SiteSetting.automatic_translation_backfill_maximum_translations_per_hour / 12), 1].max
     end
 
     def backfill_locales
-      @backfill_locales ||= SiteSetting.automatic_translation_target_languages.split("|")
+      @backfill_locales ||=
+        SiteSetting.automatic_translation_target_languages.split("|").map { |l| l.gsub("_", "-") }
     end
 
     def translator
       @translator_klass ||= "DiscourseTranslator::#{SiteSetting.translator_provider}".constantize
     end
 
-    def translate_records(type, record_ids)
+    def translate_records(type, record_ids, target_locale)
       record_ids.each do |id|
         record = type.find(id)
-        backfill_locales.each do |target_locale|
-          begin
-            translator.translate(record, target_locale.to_sym)
-          rescue => e
-            # continue with other locales even if one fails
-            Rails.logger.warn(
-              "Failed to machine-translate #{type.name}##{id} to #{target_locale}: #{e.message}\n#{e.backtrace.join("\n")}",
-            )
-            next
-          end
+        begin
+          translator.translate(record, target_locale.to_sym)
+        rescue => e
+          # continue with other locales even if one fails
+          Rails.logger.warn(
+            "Failed to machine-translate #{type.name}##{id} to #{target_locale}: #{e.message}\n#{e.backtrace.join("\n")}",
+          )
+          next
         end
       end
     end
 
     def process_batch
       models_translated = [Post, Topic].size
-      translations_per_model = [translations_per_run / models_translated, 1].max
-      topic_ids = fetch_untranslated_model_ids(Topic, "title", translations_per_model)
-      translations_per_model = translations_per_run - topic_ids.size
-      post_ids = fetch_untranslated_model_ids(Post, "cooked", translations_per_model)
-
-      DiscourseTranslator::VerboseLogger.log(
-        "Translating #{topic_ids.size} topics and #{post_ids.size} posts to #{backfill_locales.join(", ")}",
-      )
+      avg_translations_per_model_per_language = [
+        translations_per_run / models_translated / backfill_locales.size,
+        1,
+      ].max
 
-      return if topic_ids.empty? && post_ids.empty?
+      records_to_translate = avg_translations_per_model_per_language
+      backfill_locales.each_with_index do |target_locale, i|
+        topic_ids =
+          fetch_untranslated_model_ids(Topic, "title", records_to_translate, target_locale)
+        post_ids = fetch_untranslated_model_ids(Post, "cooked", records_to_translate, target_locale)
+
+        # if we end up translating fewer records than records_to_translate,
+        # add to the value so that the next locales can have more quota
+        records_to_translate =
+          avg_translations_per_model_per_language +
+            ((records_to_translate - topic_ids.size - post_ids.size) / backfill_locales.size - i)
+        next if topic_ids.empty? && post_ids.empty?
+
+        DiscourseTranslator::VerboseLogger.log(
+          "Translating #{topic_ids.size} topics and #{post_ids.size} posts to #{backfill_locales.join(", ")}",
+        )
 
-      translate_records(Topic, topic_ids)
-      translate_records(Post, post_ids)
+        translate_records(Topic, topic_ids, target_locale)
+        translate_records(Post, post_ids, target_locale)
+      end
     end
   end
 end
diff --git a/spec/jobs/automatic_translation_backfill_spec.rb b/spec/jobs/automatic_translation_backfill_spec.rb
@@ -96,8 +96,8 @@ def expect_google_translate(text)
         topic.set_detected_locale("de")
         post.set_detected_locale("es")
 
-        expect_google_translate("hallo")
         expect_google_translate("hola")
+        expect_google_translate("hallo")
 
         described_class.new.execute
 
@@ -108,7 +108,7 @@ def expect_google_translate(text)
 
     describe "with just one locale ['de']" do
       before do
-        SiteSetting.automatic_translation_backfill_maximum_translations_per_hour = 5 * 12
+        SiteSetting.automatic_translation_backfill_maximum_translations_per_hour = 100
         SiteSetting.automatic_translation_target_languages = "de"
         expect_google_check_language
       end
@@ -149,64 +149,54 @@ def expect_google_translate(text)
 This is the scenario we are testing for:
     | Post ID | detected_locale | translations | selected? | Why? |
     |---------|-----------------|--------------|-----------|------|
-    |   1     | en              | none         | YES       | source not de/es, needs both translations
-    |   2     | es              | none         | YES       | source is es, but missing de translation
-    |   3     | null            | es           | YES       | missing de translation
-    |   4     | null            | de, es       | NO        | has both de and es translations
-    |   5     | de              | es           | NO        | source is de and has es translation
-    |   6     | de              | de           | YES       | both source and translation is de, missing es translation
-    |   7     | de              | ja           | YES       | source is de, missing es translation
+    |   1     | en              | none         | YES       | source not de
+    |   2     | null            | es           | YES       | missing de translation
+    |   3     | null            | de           | NO        | has de translation
+    |   4     | de              | es           | NO        | source is de and has es translation
+    |   5     | de              | de           | NO        | both source and translation is de, missing es translation
+    |   6     | null            | none         | YES       | no detected locale nor translation
 =end
 
-      [posts_1, posts_2, posts_3].flatten.each do |post|
-        post.set_translation("es", "hola")
-        post.set_translation("de", "hallo")
-      end
+      [posts_1, posts_2, posts_3].flatten.each { |post| post.set_detected_locale("de") }
 
       post_1.set_detected_locale("en")
-      post_2.set_detected_locale("es")
+      post_4.set_detected_locale("de")
       post_5.set_detected_locale("de")
-      post_6.set_detected_locale("de")
-      post_7.set_detected_locale("de")
 
-      post_3.set_translation("es", "hola")
-      post_4.set_translation("de", "hallo")
+      post_2.set_translation("es", "hola")
+      post_3.set_translation("de", "hallo")
       post_4.set_translation("es", "hola")
-      post_5.set_translation("es", "hola")
-      post_6.set_translation("de", "hallo")
-      post_7.set_translation("ja", "こんにちは")
+      post_5.set_translation("de", "hallo")
     end
 
     it "returns correct post ids needing translation in descending updated_at" do
-      # based on the table above, we will return post_7, post_6, post_3, post_2, post_1
+      # based on the table above, we will return post_6, post_2, post_1
       # but we will jumble its updated_at to test if it is sorted correctly
       post_6.update!(updated_at: 1.day.ago)
-      post_3.update!(updated_at: 2.days.ago)
+      post_1.update!(updated_at: 2.days.ago)
       post_2.update!(updated_at: 3.days.ago)
-      post_1.update!(updated_at: 4.days.ago)
-      post_7.update!(updated_at: 5.days.ago)
 
-      result = described_class.new.fetch_untranslated_model_ids(Post, "cooked", 50, %w[de es])
-      expect(result).to include(post_6.id, post_3.id, post_2.id, post_1.id, post_7.id)
+      result = described_class.new.fetch_untranslated_model_ids(Post, "cooked", 50, "de")
+      expect(result).to include(post_6.id, post_1.id, post_2.id)
     end
 
     it "does not return posts that are deleted" do
       post_1.trash!
-      result = described_class.new.fetch_untranslated_model_ids(Post, "cooked", 50, %w[de es])
+      result = described_class.new.fetch_untranslated_model_ids(Post, "cooked", 50, "de")
       expect(result).not_to include(post_1.id)
     end
 
     it "does not return posts that are empty" do
       post_1.cooked = ""
       post_1.save!(validate: false)
-      result = described_class.new.fetch_untranslated_model_ids(Post, "cooked", 50, %w[de es])
+      result = described_class.new.fetch_untranslated_model_ids(Post, "cooked", 50, "de")
       expect(result).not_to include(post_1.id)
     end
 
     it "does not return posts by bots" do
       post_1.update(user: Discourse.system_user)
 
-      result = described_class.new.fetch_untranslated_model_ids(Post, "cooked", 50, %w[de es])
+      result = described_class.new.fetch_untranslated_model_ids(Post, "cooked", 50, "de")
 
       expect(result).not_to include(post_1.id)
     end
