Conversation
a60329b to
1b901d3
Compare
c72a87d to
81e1b1f
Compare
|
Thanks for putting this together, @kipraveen. SummaryThis adds a document-translation column generator with MQM evaluation, optional rewrites and language pivots, windowing, Bengali-to-Meetei script remapping, and native-numeral prompts. The implementation matches the feature set described in the PR, but two output-preservation paths can change document content even when the model returned it correctly. FindingsCritical — Let's fix these before merge
What Looks Good
Residual RiskThe tests use fake model responses, so translation quality with the suggested live models remains unverified by this review. VerdictNeeds changes — preserve protected spans during script remapping and retain whitespace when merging window segments before this ships. This review was generated by an AI assistant. |
81e1b1f to
08e8737
Compare
Adds data-designer-nemotron-bharat-translation, a document-translation column generator that translates a text column into a fixed target language, scores it with an MQM-based evaluator, and optionally self-corrects via a rewrite loop. Supports an optional intermediate- language pivot, windowed translation for long documents, deterministic script remapping (Bengali -> Meetei Mayek, for Manipuri), and per-row native-numeral prose generation. Registers the plugin's catalog/CODEOWNERS/site-nav entries and its generated docs page alongside the package. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
08e8737 to
bfe8aa9
Compare
|
Thanks @nabinchha for the quick review
Preserving protected spans turned on by default. Added an option for the user to turn it off if they really want to map everything.
Removing only the window separator from the start and end in a while loop. Please check if this looks good. |
nabinchha
left a comment
There was a problem hiding this comment.
Thanks for the follow-up, @kipraveen. I rechecked commit bfe8aa9 and am comfortable merging it.
Summary
The new default preserves inline code, triple-backtick fences, and URLs during script remapping, and the window merge now retains indentation. Those changes address the concrete examples in my earlier review. The notes below are optional edge cases for you to weigh against the plugin's intended document formats.
Findings
Suggestions — Take it or leave it
plugins/data-designer-nemotron-bharat-translation/src/data_designer_nemotron_bharat_translation/script_remap.py:36 — Consider other protected-span forms
- What: The preservation pattern covers backtick code and HTTP(S) URLs, but not indented or tilde-fenced code or formulas. For example, Bengali text inside an indented code literal or
$...$is still remapped. - Why: Corpora using those forms could have protected text changed after translation. The newly added tests cover the supported forms well.
- Suggestion: If these forms matter for the intended inputs, extend the span handling and add representative tests. Otherwise, the current documented scope can stand.
plugins/data-designer-nemotron-bharat-translation/src/data_designer_nemotron_bharat_translation/translator.py:528-531 — Consider intentional blank lines at window boundaries
- What: The new helper removes every leading and trailing
window_separatorfrom an extracted segment. With the default newline separator, a returned segment ending in blank lines loses them. - Why: This normalizes model padding, but can also normalize blank lines that were part of the document.
- Suggestion: If exact boundary whitespace matters to downstream users, preserve it using the source segment as a guide. If boundary normalization is intended, the current behavior is reasonable to keep.
What Looks Good
- Both reported examples are addressed: inline-code literals survive remapping, and merged segments retain indentation.
- The new option is passed through sync and async paths, with focused tests for protected spans and merge behavior.
- I verified all 134 plugin tests pass, along with
make lint,make validate,make check, andmake docsonbfe8aa9.
Verdict
Ship it (with nits) — this supersedes my earlier “Needs changes” comment. The two suggestions above are optional and do not block merging.
This review was generated by an AI assistant.
Adds data-designer-nemotron-bharat-translation, a document-translation column generator that translates a text column into a fixed target language, scores it with an MQM-based evaluator, and optionally self-corrects via a rewrite loop. Supports an optional intermediate- language pivot, windowed translation for long documents, deterministic script remapping (Bengali -> Meetei Mayek, for Manipuri), and per-row native-numeral prose generation.
Checklist
Fixes #NNNorCloses #NNN), or I am a repository collaborator performing routine maintenance or already-planned workconfig.py,impl.py,plugin.py) if adding a pluginassert_valid_plugin(plugin)passesmake test-plugin PLUGIN=<name>)uv pip install -e plugins/<plugin-dir>)make plugin-docs)make docs)catalog/plugins.jsonupdated only if preparing a first release (make catalog PLUGIN=<name>).github/CODEOWNERSregenerated if ownership changed (make codeowners)CODEOWNERSfile included (auto-created byddp new)make check-license-headers)