Skip to content

Add Nemotron Bharat document translation plugin - #94

Open
kipraveen wants to merge 1 commit into
mainfrom
kipraveen/nemotron-bharat
Open

kipraveen wants to merge 1 commit into
mainfrom
kipraveen/nemotron-bharat

Conversation

@kipraveen

@kipraveen kipraveen commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

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

  • Linked to the open, triaged issue this PR addresses (Fixes #NNN or Closes #NNN), or I am a repository collaborator performing routine maintenance or already-planned work
  • Follows the template structure (config.py, impl.py, plugin.py) if adding a plugin
  • assert_valid_plugin(plugin) passes
  • Unit tests included and passing (make test-plugin PLUGIN=<name>)
  • Plugin installs standalone (uv pip install -e plugins/<plugin-dir>)
  • Plugin docs regenerated if plugin docs, list, or metadata changed (make plugin-docs)
  • Documentation builds if docs changed (make docs)
  • catalog/plugins.json updated only if preparing a first release (make catalog PLUGIN=<name>)
  • .github/CODEOWNERS regenerated if ownership changed (make codeowners)
  • Per-plugin CODEOWNERS file included (auto-created by ddp new)
  • NVIDIA SPDX headers on all files (make check-license-headers)

@kipraveen kipraveen self-assigned this Sep 23, 2026
@kipraveen
kipraveen requested a review from a team as a code owner September 23, 2026 15:15
@kipraveen
kipraveen force-pushed the kipraveen/nemotron-bharat branch 2 times, most recently from a60329b to 1b901d3 Compare September 23, 2026 15:26
@kipraveen
kipraveen force-pushed the kipraveen/nemotron-bharat branch 3 times, most recently from c72a87d to 81e1b1f Compare September 23, 2026 15:48
@nabinchha

Copy link
Copy Markdown
Contributor

Thanks for putting this together, @kipraveen.

Summary

This 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.

Findings

Critical — Let's fix these before merge

plugins/data-designer-nemotron-bharat-translation/src/data_designer_nemotron_bharat_translation/document_translation.py:214 — Script remapping changes protected content

  • What: remap_script() runs over the entire final translation. Bengali characters inside code literals are converted too: `label = "বাংলা"` becomes `label = "ꯕꯥꯪꯂꯥ"`.
  • Why: The translation prompt promises to preserve code and other protected spans byte-for-byte. In the Manipuri remap path, valid code or quoted data can therefore be silently corrupted after translation.
  • Suggestion: Keep protected spans out of the remapper, then add a test with Bengali prose alongside an inline or fenced code literal to verify that only the prose changes script.

plugins/data-designer-nemotron-bharat-translation/src/data_designer_nemotron_bharat_translation/translator.py:510 — Window merging discards document whitespace

  • What: _require() calls strip() on every extracted segment before the windowed document is assembled. A correctly returned segment such as " indented line\n" becomes "indented line".
  • Why: This changes indentation and trailing newlines in whitespace-sensitive content, including code blocks, even when the merge model preserved them. It also undermines the prompt's format-preservation rule.
  • Suggestion: Use text.strip() only to reject blank output, then return the original text. Add a windowed-merge test with indentation and a trailing newline.

What Looks Good

  • The plugin is self-contained and its entry point passes DataDesigner's plugin validation.
  • The config checks supported script-remap pairs and native-numeral coverage before generation.
  • The package includes substantial focused tests and generated documentation. I verified all 126 new-plugin tests pass, along with the full repository test suite, lint, validation, metadata checks, and documentation build.

Residual Risk

The tests use fake model responses, so translation quality with the suggested live models remains unverified by this review.

Verdict

Needs 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.

@kipraveen
kipraveen force-pushed the kipraveen/nemotron-bharat branch from 81e1b1f to 08e8737 Compare September 24, 2026 13:17
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>
@kipraveen
kipraveen force-pushed the kipraveen/nemotron-bharat branch from 08e8737 to bfe8aa9 Compare September 24, 2026 13:32
@kipraveen

Copy link
Copy Markdown
Collaborator Author

Thanks @nabinchha for the quick review

Script remapping changes protected content

Preserving protected spans turned on by default. Added an option for the user to turn it off if they really want to map everything.

Window merging discards document whitespace

Removing only the window separator from the start and end in a while loop.

Please check if this looks good.

@nabinchha nabinchha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_separator from 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, and make docs on bfe8aa9.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants