Skip to content

fix(bart): honor added-token lstrip - #1383

Open
WenJing95 wants to merge 1 commit into
NVIDIA:mainfrom
WenJing95:fix/bart-mask-whitespace
Open

WenJing95 wants to merge 1 commit into
NVIDIA:mainfrom
WenJing95:fix/bart-mask-whitespace

Conversation

@WenJing95

Copy link
Copy Markdown
Contributor

Background

BART tokenizer ignores lstrip: true on <mask>, leaving an extra whitespace token.

Exit Criteria

Strip preceding whitespace for flagged added tokens without affecting normal tokens.

Implementation

Track flagged IDs, trim preceding text, and add a CPU regression test target. No API/ABI changes.

Change categories

  • Model or runtime behavior

Validation

Commands and Results

  • Added CPU regression harness passed (bart_added_token_whitespace).
  • python -m tools.model_ci validate and python tools/test_impact.py --validate passed.

Hardware, Environment, and Revisions

Windows x86_64 CPU (GCC 14.2.0 / Python 3.12.3).

Not Run / Remaining Gaps

Tested on CPU harness only; full GPU/TensorRT builds and inference left to CI.

Contributor Self-Review

  • I have completed a self-review of this change.

Notes For Future Readers

Rebuild the BART runtime.

Risk level

  • Low

Signed-off-by: wj <126436871+WenJing95@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 19, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: NVIDIA/TensorRT-Model-Connect/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1ac9407e-2053-43fd-ad4c-983619533ac0

📥 Commits

Reviewing files that changed from the base of the PR and between 393ab02 and d2e621f.

📒 Files selected for processing (3)
  • families/bart/runtime/CMakeLists.txt
  • families/bart/runtime/bpe_tokenizer.cpp
  • families/bart/tests/cpp/test_bart_added_token_whitespace.cpp

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Summary

Summary

Updates the BART tokenizer to honor AddedToken.lstrip: true.

  • Strips preceding Unicode whitespace before added tokens such as <mask>.
  • Leaves normal tokens and other special tokens unchanged.
  • Adds CPU regression coverage for ASCII and Unicode whitespace, adjacent masks, zero-width characters, and disabled post-processing.
  • Registers the bart_added_token_whitespace CTest target.

Architecture impact

  • Family-owned files: Changes remain within BART runtime and test files.
  • Shared surfaces: No public API or ABI changes are reported.
  • Dependency directions: The test depends on the BART tokenizer and nlohmann JSON. No new runtime dependency is added.
  • Affected consumers: BART tokenizer consumers that use added tokens with lstrip: true.
  • Unresolved blast-radius questions: Full GPU, TensorRT, and inference validation remains for CI. The supplied evidence does not establish impact on other tokenizer families.

Status

HUMAN REVIEW REQUIRED

The supplied context reports passing CPU and validation checks. No current review findings or severity counts were supplied. Full cross-backend validation remains incomplete.

Walkthrough

The BART tokenizer now honors AddedToken.lstrip for leading Unicode whitespace. A standalone regression test covers mask-token behavior across whitespace types, adjacent tokens, special tokens, and post-processing modes.

Changes

BART added-token whitespace handling

Layer / File(s) Summary
Implement added-token left-space stripping
families/bart/runtime/bpe_tokenizer.cpp
The tokenizer records added-token IDs with lstrip: true and removes leading Unicode White_Space characters from the preceding normal segment.
Add whitespace regression coverage
families/bart/tests/cpp/test_bart_added_token_whitespace.cpp, families/bart/runtime/CMakeLists.txt
The test covers ASCII and Unicode whitespace, adjacent tokens, special tokens, zero-width characters, and post-processing modes. CMake builds and registers the test when TRTMC_BUILD_TESTS is enabled.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix

🚥 Pre-merge checks | ✅ 8 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (8 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: honoring AddedToken.lstrip in the BART tokenizer.
Description check ✅ Passed The description covers the background, exit criteria, implementation, behavior category, validation, remaining gaps, self-review, and rebuild note. It is mostly complete, but it does not provide the e…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Family Ownership Boundary ✅ Passed The pull request changes only three files under families/bart. The tokenizer includes families/bart/runtime/tokenizer.h, and the new test includes the same BART header. The new CMake target compil…
Shared Semantic Neutrality ✅ Passed PASS. The pull request changes only families/bart/runtime and families/bart/tests/cpp. These are explicitly excluded from this check. The lstrip behavior is implemented in BART's family-owned `b…
Benchmark Validation Integrity ✅ Passed PASS. This pull request adds functional tokenizer validation, not benchmark or performance accounting. The new bart_added_token_whitespace test in families/bart/runtime/CMakeLists.txt compares `IT…
Shared Change Blast Radius ✅ Passed The pull request changes only BART-owned files: families/bart/runtime/bpe_tokenizer.cpp, its family CMake file, and a BART test. The tokenizer declaration and its consumers are also under `families/…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 unsupported.)


Comment @coderabbitai help to get the list of available commands.

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.

1 participant