Add ability to compose multi-value Machinery translations#4236
Add ability to compose multi-value Machinery translations#4236mathjazz wants to merge 24 commits into
Conversation
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
eemeli
left a comment
There was a problem hiding this comment.
Looked at the Python parts only thus far.
|
Thanks for the comments. Please note that I've intentionally not flagged anyone for code review yet, because I'd like to first get feedback on the functionality. The code is still deployed to https://pontoon.allizom.org/. |
For Fluent and MF2-handled formats (Android, Gettext, WebExt, Xcode, Xliff), the Machinery panel only matched on the first input field (via `getPlainMessage()`), so attributes and selector variants were never surfaced. This mirrors Pretranslation's complex string composition in Machinery, adding directly-pasteable composed suggestions alongside the existing results. More details: 1. Parameterize Pretranslation with mt_provider/mt_service_name/ mt_supported, and move entity-walking into `Pretranslation.walk_entity()` so Machinery can reuse the composition pipeline. 2. Add `/machinery-composed/` endpoint that walks the entity, looks up each value in TM, and falls back to the requested MT service for any remaining value. Returns the composed string + the actual mix of services used (TM badge + MT badge for hybrid results). 3. Frontend fires composed requests in parallel with the existing fetches when the entity format can have multiple values. Composed results dedupe through the existing `addResults()` merge.
on the entity having more than one translatable input, reusing the editor's field-counting logic. 2. Surface a quality badge. When every value is a 100% TM match, the composed string is a perfect TM match, so return quality 100 and pass it through to the panel. 3. Render composed (multi-value) suggestions as labeled fields, the same representation as the original string panel.
across all input fields, reusing the field-building logic that is already used by the History panel. The plain message is recorded as `machinery.translation` so that source attribution still matches the saved translation on submit.
string isn't suggested back to itself. The composed path didn't, so a composed TM result could be reconstructed from the entity's own translation after it was translated. Add an opt-in `exclude_entity` flag to Pretranslation that excludes the entity's own TM entries from per-value lookups, and enable it from the Machinery composed view. A value that can only be served by the entity's own translation then has no TM match, so a TM-only composition relying on it is no longer produced. Pretranslation behavior is unchanged.
Re-applying a composed Machinery suggestion (or restoring history) after editing a field took two clicks: the first did nothing. Typing updates only CodeMirror's internal doc and EditorResult, not EditorData.state.fields, so TranslationForm doesn't re-render and each EditField keeps a stale `defaultValue`. EditField re-syncs its document only in `useEffect(() => setValue(defaultValue), [defaultValue])`. distributeEntrySource builds new fields with placeholder handles while the on-screen editors stay bound, via their React key, to the previous fields' live handles. The re-applied value for the edited field equals its stale `defaultValue`, so the effect doesn't fire and the field isn't updated. A later re-render refreshes `defaultValue`, which is why the second click works. Push the distributed values straight into the live handles, matched by field id, the same way clearEditor does, so one click suffices.
The "Refine using AI" dropdown doesn't work on composed (multi-field/ plural) suggestions: the loader never shows, the refined result never updates the UI, and copying dumps the raw Fluent source into the first field. The backend /gpt-transform/ endpoint also refines a single string, so it can't preserve the entry structure (e.g. returns 2 plural forms instead of 4). Hide the dropdown for composed suggestions so they behave like a plain Google Translate source. Proper composed support is left as a follow-up.
When a suggestion combines multiple sources (e.g. GOOGLE TRANSLATE and TRANSLATION MEMORY), the source titles ran together with no separator.
…ntity.value and .properties represent a single-pattern or multi-pattern message.
e11f61f to
960112e
Compare
|
Rebased and addressed comments. |
|
Could you rebase this? Also, did we remove this code from the |
| # Only multi-pattern messages — those with multiple properties and/or | ||
| # selector variants — have something to compose. A single-pattern message | ||
| # composes to the same string the per-leaf machinery already returns, so | ||
| # there is nothing extra to show. |
There was a problem hiding this comment.
This is not always true; if the source message contains a plural selector, it may have only one pattern, but require multiple patterns in the target locale.
There was a problem hiding this comment.
I think the code works as you are describing, but the comment is confusing (also looking at the comment above), because it uses the term "pattern" differently than moz-l10n.
What do you think about replacing the term "pattern" here and in other places with "leaf"? A leaf essentially means the number of input fields shown in the editor.
There was a problem hiding this comment.
What I mean is that if the entity.value is representing a Fluent message like
popup-warning-exceeded-message =
{ $popupCount ->
*[other] { -brand-short-name } prevented this site from opening more than { $popupCount } pop-up windows.
}then languages like Slovenian will want to represent it with multiple patterns:
popup-warning-exceeded-message =
{ $popupCount ->
[one] { -brand-short-name } je strani preprečil, da bi odprla več kot { $popupCount } pojavno okno.
[two] { -brand-short-name } je strani preprečil, da bi odprla več kot { $popupCount } pojavni okni.
[few] { -brand-short-name } je strani preprečil, da bi odprla več kot { $popupCount } pojavna okna.
*[other] { -brand-short-name } je strani preprečil, da bi odprla več kot { $popupCount } pojavnih oken.
}even if the unused one variant is left out.
There was a problem hiding this comment.
Good point. Let me address this.
| response = { | ||
| "original": entity.string, | ||
| "translation": translation, | ||
| "sources": sources_used, | ||
| } |
There was a problem hiding this comment.
Why is this API serialising the value+properties, and not returning them as data model values?
There was a problem hiding this comment.
For consistency with the rest of the machinery API. Every machinery source returns { original, translation } as plain strings, and the Machinery panel sorts, dedups, renders, and copies them uniformly.
And, the frontend already parses full entry sources. On copy, distributeEntrySource calls `parseEntry(format, str)à to spread the leaves across the editor fields.
There was a problem hiding this comment.
We should work towards (later, separately) transitioning the other machinery API endpoints to also using a data model representation for messages, as we need to get that in any case to validate that what we're getting e.g. from machine translation is valid.
There was a problem hiding this comment.
That could be done as part of KR2: Refactor the TM entry representation around the message data model.
There was a problem hiding this comment.
Eh, if you insist. But I do think the PR's current API is trying to fit a square peg in a round hole just because the round hole already exists.
| // Set for `/machinery-composed/` results, whose `original` and `translation` | ||
| // are full entry sources (Fluent attributes, MF2 variants) rather than plain | ||
| // strings — the Machinery panel renders these in a rich, multi-field view. | ||
| composed?: boolean; |
There was a problem hiding this comment.
Wouldn't it be simpler to define a new ComposedMachineryTranslation type, as it needs to carry multiple patterns instead of the single pattern of MachineryTranslations?
That would also make it easier to pass the value+properties in the API as JSON data model values, rather than serialising them on the server and re-parsing on the client.
value_from_string in pontoon.sync duplicated parse_db_string_to_json. Consolidate into a single parse_source_string_to_json in pontoon.translations (beside its inverse, serialize_for_db); parse_db_string_to_json now delegates to it, and the key-aware path picks up the MF2 catchall normalization the sync copy was missing.
…cialFormats, which is byte-for-byte identical
…ntity.properties with no parse. Also, document the source-side heuristic.
The composed-machinery gate counted source patterns, so an en-US message with only `*[other]` was treated as single- pattern and skipped — even for locales like Slovenian that need one/two/few/other. Count target patterns instead: the backend reuses the plural expansion walk_entity() already performs, and the frontend's hasMultipleFields expands plural selectors to the locale's CLDR categories. A single-variant source now composes to a full multi-pattern suggestion for the target locale.
There was a problem hiding this comment.
At least a couple of the front-end comments from the previous review still need to be addressed too.
Edit: Actually, just #4236 (comment)
| translation = body["translation"] | ||
| assert "[one]" in translation | ||
| assert "[two]" in translation | ||
| assert "[few]" in translation | ||
| assert "*[other]" in translation | ||
| assert translation.count("TM_popups") == 4 | ||
| assert body["sources"] == ["translation-memory"] | ||
| assert body["quality"] == 100 |
There was a problem hiding this comment.
This would be clearer as a single assert body == ... test.
| def parse_db_string_to_json( | ||
| res_format: str, | ||
| source: str, | ||
| ) -> tuple[JsonMessage, dict[str, JsonMessage] | None]: | ||
| _, value, properties = parse_source_string_to_json(res_format, source) | ||
| return value, properties |
There was a problem hiding this comment.
This function should not exist, and we should just refactor the places where it's called to call parse_source_string_to_json instead.
| key, value, properties = parse_source_string_to_json(fmt, text) | ||
| entity = SimpleNamespace( | ||
| resource=resource, | ||
| string=text, | ||
| key=key, | ||
| value=value, | ||
| properties=properties, | ||
| ) |
There was a problem hiding this comment.
This is the only non-test code that makes any use of the key value returned by parse_source_string_to_json, and the use it's put to is to set the corresponding value in the fake Entity that's constructed here, which is only used to set the id of the temporary Entry that's passed to set_accesskeys, which ignores it.
So we don't actually need to return it in the first case.
| value=value, | ||
| properties=properties, | ||
| ) | ||
| pretranslation = get_pretranslation(entity=entity, locale=locale) |
There was a problem hiding this comment.
This needs another TODO/FIXME comment about refactoring so that we don't need to construct the fake Project, Resource, and Entity values to call it.
eemeli
left a comment
There was a problem hiding this comment.
Ah, sorry, looks like I missed some of the translate/ changes.
| let count = 1; | ||
| for (let i = 0; i < msg.sel.length; ++i) { | ||
| if (plurals.has(i)) { | ||
| count *= Math.max(1, pluralCategories); | ||
| } else { | ||
| // Non-plural selector (e.g. a gender): keep its distinct source keys. | ||
| const keys = new Set( | ||
| msg.alt.map((v) => { | ||
| const key = v.keys[i]; | ||
| return typeof key === 'string' ? key : '*'; | ||
| }), | ||
| ); | ||
| count *= keys.size || 1; | ||
| } | ||
| } | ||
| return count; |
There was a problem hiding this comment.
This seems unnecessarily accurate. We don't actually care about the pattern count, we only care if it's more than one.
| specialFormats.has(format) && | ||
| hasMultipleFields( | ||
| entity.value, | ||
| entity.properties, | ||
| locale.cldrPlurals.length, | ||
| ); |
There was a problem hiding this comment.
Given the early exits in patternCount(), there's no reason to gate this on the format as well.
| specialFormats.has(format) && | |
| hasMultipleFields( | |
| entity.value, | |
| entity.properties, | |
| locale.cldrPlurals.length, | |
| ); | |
| hasMultipleFields( | |
| entity.value, | |
| entity.properties, | |
| locale.cldrPlurals.length, | |
| ); |

Fix #2886.