Refactor sync #3312

eemeli · 2024-09-02T14:38:20Z

Closes #2023 -- entity- and translation-level changes are logged in ActionLog
Fixes #2057
Fixes #2068
Fixes #2078
Closes #2083 -- source changes are now sync'd to targets eagerly
Fixes #2087 -- git file moves are caught, but not copies
Closes #2110 -- the remaining sync tests are largely replaced here
Closes #2129 -- refactoring the sync changes the performance characteristics completely
Fixes #2169
Fixes #2175
Fixes #2189
Fixes #2211
Fixes #2242
Closes #2285 -- not relevant after the refactor
Fixes #2641
Fixes #3302
Fixes #3449

This is effectively a rewrite of the sync_project() function that's currently here, and which ends up calling most of the code under pontoon/sync/.

The end results of the code here should be the same as currently, but the implementation is completely new, and does things in a different order.

Per-locale repositories are dropped here, as per #3303.

Explicitly left out of this PR but liable to change later:

What sync logging information is persisted in the database.
How files are read and written.
How localizable messages are represented in the database.
How aggregated stats are gathered.

mathjazz

Very high-level look, but this already looks very promising!

Currently, Pretranslation is tightly integrated with sync in order to minimize time between exposing new strings for localization and pretranslating them. What's your plan with this?

pontoon/base/models/changed_entity_locale.py

pontoon/base/models/project.py

mathjazz · 2024-09-04T12:48:19Z

pontoon/sync/sync_project.py

+        paths = get_paths(project, checkouts)
+    except Exception as e:
+        log.error(f"{log_prefix} {e}")
+        project_sync_log.skip()


Nit: when we'll be refactoring the sync log, we should make a distinction between skip() (no changes needed during sync) and fail() (sync failed due to an error).

pontoon/sync/sync_project.py

mathjazz · 2024-09-04T15:14:56Z

pontoon/sync/checkouts.py

+        if db_repo.last_synced_revisions is None:
+            self.prev_commit = None
+        else:
+            pc = db_repo.last_synced_revisions.get("single_locale", None)


We should drop the single_locale but if we're resolving #3303.

Agreed. But let's do it as a follow-up; I would much prefer being able to do this refactor without any db changes, so that it's easier to roll back if necessary.

pontoon/sync/sync_entities_from_repo.py

mathjazz · 2024-09-04T15:52:15Z

pontoon/sync/sync_translations_to_repo.py

+    removed_source_paths: set[str],
+    now: datetime,
+) -> None:
+    lc_readonly = set(


Nit: There are quite a few prefixes and acronyms used in this patch (lc_, mod_, tx etc.), that are not immediately understandable like some of the more established ones (pk, db, etc.).

Fair point. lc is short for "locale code", mod for "modified", and tx for "translation", but those are by no means obvious.

For local variables, I have a strong preference for keeping their names short, and relying on the reader to be able to figure out the meaning from where the values come from. Would documenting these somewhere be a sufficient improvement, or do you think that some or all of these are just too obscure? Or are there alternative shortenings that would be better?

For me even single-letter variables are acceptable in cases like this. I'd find l and teasier to understand than lc and tx.

I don't think we should document that.

eemeli · 2024-09-04T22:08:30Z

Currently, Pretranslation is tightly integrated with sync in order to minimize time between exposing new strings for localization and pretranslating them. What's your plan with this?

Ah, I'd missed that! Yeah, that needs to happen the same as before.

…object

requirements/default.txt

codecov-commenter · 2024-10-01T15:26:11Z

Codecov Report

Attention: Patch coverage is 90.24552% with 147 lines in your changes missing coverage. Please review.

Project coverage is 79.92%. Comparing base (24f37e5) to head (cebc5b2).
Report is 4 commits behind head on main.

Additional details and impacted files

eemeli · 2024-10-01T15:47:30Z

This is now at the dangerous stage of looking like it works. But some verification work still remains:

Add tests for the fixtures used by the old sync tests.
Check the sync results against the active pontoon.mozilla.org projects.

pontoon/base/__init__.py

eemeli · 2024-10-01T16:01:25Z

pontoon/base/views.py

+    # FIXME: zip downloads should only be for projects with 2..10 resources
+    from pontoon.sync.utils import download_translations_zip


This is a compromise reached after discussion with @mathjazz elsewhere. We'll be refactoring this code again as a part of the subsequent data model refactor, at which point we'll be able to introduce download support that won't require us to re-fetch any files.

eemeli · 2024-10-01T16:06:02Z

pontoon/sync/core/__init__.py

+    if project.pretranslation_enabled and changed_paths:
+        # Pretranslate changed and added resources for all locales
+        pretranslate(project, changed_paths)


The new sync is retaining slightly different information than before, so it makes more sense to trigger the pretranslation for all entities in changed resources. The pretranslation will then filter out the entities and locales for which pretranslation is not needed or wanted.

eemeli · 2024-10-15T16:23:03Z

I can now download files from AMO Linter, but downloading from Firefox still fails on stage (it works locally).

From my own experimentation, it looks like there's at least some timing issue due to the git clone that's now needed. Retrying got amo-frontend download to work for me.

Locally I couldn't make upload work, though. I downloaded the brand.ftl file, deleted a string in Pontoon, uploaded the file, and the deleted string didn't get imported.

Does this work even now? As in, does uploading a resource with a string removed remove it from Pontoon?

mathjazz · 2024-10-15T16:26:40Z

Does this work even now? As in, does uploading a resource with a string removed remove it from Pontoon?

I uploaded a resource which had a translation that didn't exist in Pontoon.

mathjazz · 2024-10-15T16:41:26Z

From my own experimentation, it looks like there's at least some timing issue due to the git clone that's now needed. Retrying got amo-frontend download to work for me.

Can we use git clone --depth 1 for downloading files?

mathjazz · 2024-10-15T20:24:29Z

We should update the README file.

eemeli · 2024-10-16T11:52:06Z

The stats update should now be much more robust, and a bit simpler -- albeit written in SQL, as I couldn't figure out the corresponding Django invocations. In particular the COUNT(*) FILTER (WHERE ...) values are probably not even possible.

This also guarantees that the work is entirely done within the database, where even for the most complex projects it should take at most a few seconds even though the update is applied to the entire current project.

A "messy local setup" as mentioned above should no longer be able to produce the error that @flodolo encountered.

As a minor regression, gettext plurals are now counted as separate entries. This means that when translating from English to a locale with more plural categories may result in an approved_strings count greater than total_strings. This will get fixed by my subsequent work, which will change gettext plurals are stored in the database.

flodolo · 2024-10-16T12:29:07Z

A "messy local setup" as mentioned above should no longer be able to produce the error that @flodolo encountered.

Confirming that this worked locally 👍🏼

flodolo · 2024-10-16T13:08:01Z

Confirming that this worked locally 👍🏼

Correction: the error now shows up when you update any string (e.g. reject one), e.g.

2024-10-16 15:07:05 server-1      | django.db.utils.IntegrityError: new row for relation "base_locale" violates check constraint "base_locale_approved_strings_check"
2024-10-16 15:07:05 server-1      | DETAIL:  Failing row contains (50, 0, -1, 0, 0, 0, 1, it, it, it, it-it, , Italian, (n != 1), 1,5, Latin, ltr, 69025000, , 25173, 100, 99, , , , t).

mathjazz · 2024-10-16T15:38:09Z

The stats update should now be much more robust, and a bit simpler -- albeit written in SQL, as I couldn't figure out the corresponding Django invocations. In particular the COUNT(*) FILTER (WHERE ...) values are probably not even possible.

As a minor regression, gettext plurals are now counted as separate entries. This means that when translating from English to a locale with more plural categories may result in an approved_strings count greater than total_strings. This will get fixed by my subsequent work, which will change gettext plurals are stored in the database.

Note that calculate_stats() is called from many places (including the translate view) and that approved_strings greater than total_strings might cause our charts (in dashboards as well as in translate view) to break (I didn't actually test). All that means we shouldn't just thoroughly test the sync module, but the entire app.

flodolo · 2024-10-30T06:57:45Z

Also for the same new file (not committed to the repo). Trying to download translations will crash Pontoon, because the file is not in the repo.

2024-10-30 07:56:43 [ERROR:django.request] 2024-10-30 06:56:43,698 Internal Server Error: /translations/
2024-10-30 07:56:43 Traceback (most recent call last):
2024-10-30 07:56:43   File "/usr/local/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
2024-10-30 07:56:43     response = get_response(request)
2024-10-30 07:56:43                ^^^^^^^^^^^^^^^^^^^^^
2024-10-30 07:56:43   File "/usr/local/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
2024-10-30 07:56:43     response = wrapped_callback(request, *callback_args, **callback_kwargs)
2024-10-30 07:56:43                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2024-10-30 07:56:43   File "/usr/local/lib/python3.11/contextlib.py", line 81, in inner
2024-10-30 07:56:43     return func(*args, **kwds)
2024-10-30 07:56:43            ^^^^^^^^^^^^^^^^^^^
2024-10-30 07:56:43   File "/app/pontoon/base/views.py", line 768, in download_translations
2024-10-30 07:56:43     content, filename = download_translations_zip(project, locale)
2024-10-30 07:56:43                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2024-10-30 07:56:43   File "/app/pontoon/sync/utils.py", line 31, in download_translations_zip
2024-10-30 07:56:43     zipfile.write(lc_path, basename(tgt_path))
2024-10-30 07:56:43   File "/usr/local/lib/python3.11/zipfile.py", line 1796, in write
2024-10-30 07:56:43     zinfo = ZipInfo.from_file(filename, arcname,
2024-10-30 07:56:43             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2024-10-30 07:56:43   File "/usr/local/lib/python3.11/zipfile.py", line 535, in from_file
2024-10-30 07:56:43     st = os.stat(filename)
2024-10-30 07:56:43          ^^^^^^^^^^^^^^^^^
2024-10-30 07:56:43 FileNotFoundError: [Errno 2] No such file or directory: '/app/media/projects/firefox/[email protected]:flodolo/firefox-l10n.git/it/mobile/android/mobile-l10n.js'

EDIT: it actually fails also after syncing to the repo, so the file is actually there.

flodolo · 2024-10-31T11:24:23Z

~~Looks like I'm once again unable to set up Firefox from scratch (sync doesn't pick up translations, even with --force).~~
~~The log is full of messages like about the TOML config~~

[DEBUG:pontoon.sync.core.translations_from_repo] 2024-10-31 11:20:03,375 [firefox:../../[email protected]:mozilla-l10n/firefox-l10n.git/it/devtools/client/toolbox.ftl] Not an L10n target path

Update: Not sure what happened, but I think my local DB is toasted (too many tests). Creating a new project with a different slug worked 🤷🏼

flodolo · 2024-11-13T14:09:25Z

pontoon/sync/core/translations_to_repo.py

+            lc_str = f"{len(updated_locales)} localizations"
+        else:
+            lc_str = ", ".join(f"{loc.name} ({loc.code})" for loc in updated_locales)
+    commit_msg = f"Pontoon/{project.name}: Update {lc_str}"


I'm not a fan of this commit message.

Pontoon/Firefox (local): Update Italian (it)

I find the existing one more readable:

Pontoon: Update Italian (it) localization of Firefox (local)

Have you considered what the message looks like if it contains changes for multiple locales? With the old format, it quickly gets much more variable and clunkier.

Pontoon only commits one locale at a time now. Is that changing with the new code?

I'm guessing yes, given lc_str = "1 locale" if lc_count == 1 else f"{lc_count} locales".

I feel like we have discussed this at some point, but it's been long enough that I don't know if that happened, or where 🤔

Add new sync implementation (WIP, untested)

3d4c761

mathjazz reviewed Sep 4, 2024

View reviewed changes

eemeli added 2 commits September 9, 2024 15:03

Address code review comments, start adding tests

9e65c9a

Merge branch 'main' into sync-refactor

3d6a1ec

eemeli mentioned this pull request Sep 16, 2024

Path improvements for use in Pontoon mozilla/moz-l10n#22

Merged

Add integration tests for new code; fix discovered issues

397e5f5

eemeli force-pushed the sync-refactor branch from 1f24718 to 397e5f5 Compare September 16, 2024 15:57

eemeli added 5 commits September 17, 2024 13:56

Update requirements

5778090

Satisfy lint

7f3bfcb

Call repo commit() with pre-formatted author string rather than User …

83b8fd7

…object

Add end-to-end test

3005870

Add task wrapper & force option

5b227e9

flodolo reviewed Sep 22, 2024

View reviewed changes

requirements/default.txt Outdated Show resolved Hide resolved

eemeli added 9 commits September 22, 2024 09:00

Refactor handle_upload_content() into sync_uploaded_file()

e657223

Replace get_download_content() with download_translations_zip()

5da483f

Remove old sync implementation

2f0ea10

Pretranslate added & changed resources, fix task invocations

bb97199

Move & rename pontoon.sync contents around, adding pontoon.sync.core

3da84c0

Combine upload & download functions into pontoon.sync.utils

92fb993

Merge branch 'main' into sync-refactor

410a278

Include --no-strip-extras in uv pip compile calls

83049d9

Satisfy ruff

f92e331

File format detector cleanup, drop unused template

fcfc5fa

eemeli commented Oct 1, 2024

View reviewed changes

pontoon/base/__init__.py Show resolved Hide resolved

eemeli commented Oct 1, 2024

View reviewed changes

Fix last_synced_revision data, drop remaining multi_locale references

15ef5b6

This comment was marked as resolved.

Sign in to view

eemeli added 2 commits October 16, 2024 10:45

Simplify aggregated stats for .po plurals, use SQL UPDATE queries

646ab4b

Reduce stats updates further, include total_strings calculation

b4854c4

Use simpler query for looking up entity identifiers

d322f3d

eemeli added 6 commits October 17, 2024 10:18

Use new update_stats() for manage.py calculate_stats command

68f94f7

Add & remove TranslatedResource objects when locales change

240cd73

Sum project total_strings from translated resources, not resources

b70451b

Update moz.l10n to 0.5.2, log changed resources

33c307d

Always sync all translated resources

263a2f1

Merge branch 'main' into sync-refactor

e6d0425

This comment was marked as resolved.

Sign in to view

flodolo reviewed Nov 13, 2024

View reviewed changes

flodolo mentioned this pull request Nov 19, 2024

Can't approve translations in a private project #3447

Closed

eemeli added 3 commits November 26, 2024 15:08

Merge branch 'main' into sync-refactor

5eb6a9f

Fix manual pretranslation task

f7171bf

Fix file upload, ensure that it reports at least some error on failure

cf4e4b8

This was referenced Nov 27, 2024

Set all translation authors and commit authors #2158

Closed

Changing repo URLs may break sync #2025

Open

eemeli added 2 commits November 27, 2024 16:21

Update to moz.l10n 0.5.5

bd649e7

Satisfy ruff

cebc5b2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor sync #3312

Refactor sync #3312

eemeli commented Sep 2, 2024 •

edited

Loading

mathjazz left a comment

mathjazz Sep 4, 2024

mathjazz Sep 4, 2024

eemeli Sep 4, 2024

mathjazz Sep 4, 2024

eemeli Sep 4, 2024

mathjazz Sep 5, 2024

eemeli commented Sep 4, 2024

codecov-commenter commented Oct 1, 2024 •

edited

Loading

eemeli commented Oct 1, 2024 •

edited

Loading

eemeli Oct 1, 2024

eemeli Oct 1, 2024

eemeli commented Oct 15, 2024

mathjazz commented Oct 15, 2024

mathjazz commented Oct 15, 2024

mathjazz commented Oct 15, 2024

This comment was marked as resolved.

eemeli commented Oct 16, 2024 •

edited

Loading

flodolo commented Oct 16, 2024

flodolo commented Oct 16, 2024

mathjazz commented Oct 16, 2024 •

edited

Loading

This comment was marked as resolved.

flodolo commented Oct 30, 2024 •

edited

Loading

flodolo commented Oct 31, 2024 •

edited

Loading

flodolo Nov 13, 2024

eemeli Nov 27, 2024

flodolo Nov 27, 2024

flodolo Nov 27, 2024

		# FIXME: zip downloads should only be for projects with 2..10 resources
		from pontoon.sync.utils import download_translations_zip

Refactor sync #3312

Are you sure you want to change the base?

Refactor sync #3312

Conversation

eemeli commented Sep 2, 2024 • edited Loading

mathjazz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eemeli commented Sep 4, 2024

codecov-commenter commented Oct 1, 2024 • edited Loading

Codecov Report

eemeli commented Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eemeli commented Oct 15, 2024

mathjazz commented Oct 15, 2024

mathjazz commented Oct 15, 2024

mathjazz commented Oct 15, 2024

This comment was marked as resolved.

eemeli commented Oct 16, 2024 • edited Loading

flodolo commented Oct 16, 2024

flodolo commented Oct 16, 2024

mathjazz commented Oct 16, 2024 • edited Loading

This comment was marked as resolved.

flodolo commented Oct 30, 2024 • edited Loading

flodolo commented Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eemeli commented Sep 2, 2024 •

edited

Loading

codecov-commenter commented Oct 1, 2024 •

edited

Loading

eemeli commented Oct 1, 2024 •

edited

Loading

eemeli commented Oct 16, 2024 •

edited

Loading

mathjazz commented Oct 16, 2024 •

edited

Loading

flodolo commented Oct 30, 2024 •

edited

Loading

flodolo commented Oct 31, 2024 •

edited

Loading