fix(model): add truncation to tokenizer for long category names#897
fix(model): add truncation to tokenizer for long category names#897rajeshgangireddy wants to merge 2 commits into
Conversation
-e Signed-off-by: rajeshgangireddy <rajesh.gangireddy@intel.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the SAM3 model’s text tokenization to safely handle long category names (e.g., UUID-like labels) by enabling tokenizer truncation, avoiding failures when inputs exceed the model’s 32-token text context.
Changes:
- Enable
truncation=Truewhen tokenizing prompts for cached exemplar text features. - Enable
truncation=Truewhen tokenizing prompts in the classic prediction path. - Add a unit test asserting truncation is enabled for long category names in
fit()(visual exemplar mode).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
library/src/instantlearn/models/sam3/sam3.py |
Adds tokenizer truncation in both cached-text and classic prediction tokenization calls. |
library/tests/unit/models/test_sam3.py |
Adds a unit test to verify tokenizer truncation is used for long category names during fit(). |
| text_inputs = self.tokenizer( | ||
| [prompt], | ||
| return_tensors="pt", | ||
| padding="max_length", | ||
| truncation=True, | ||
| max_length=32, | ||
| ) |
There was a problem hiding this comment.
The PR metadata mentions fixing an error when bounding boxes are passed to SAM3, but the code changes here are about truncating long text prompts for the tokenizer. Please align the PR description (or title) with the actual change so reviewers understand the root cause and affected paths.
| """Test fit() truncates long category names (e.g. UUIDs) to CLIP max_length. | ||
|
|
||
| The backend passes label UUIDs as category names, which tokenize to ~35 | ||
| subword tokens — exceeding CLIP's 32-token max_position_embeddings. | ||
| The tokenizer must truncate to avoid ValueError. |
There was a problem hiding this comment.
The docstring says this is "CLIP's" 32-token max_position_embeddings, but CLIP models commonly use a larger context length. If the 32-token limit is specific to SAM3's text encoder config, update the wording to avoid confusion for future maintainers.
| """Test fit() truncates long category names (e.g. UUIDs) to CLIP max_length. | |
| The backend passes label UUIDs as category names, which tokenize to ~35 | |
| subword tokens — exceeding CLIP's 32-token max_position_embeddings. | |
| The tokenizer must truncate to avoid ValueError. | |
| """Test fit() truncates long category names (e.g. UUIDs) to the configured max length. | |
| The backend passes label UUIDs as category names, which tokenize to ~35 | |
| subword tokens — exceeding the 32-token limit used by SAM3's text | |
| encoder configuration in this test setup. The tokenizer must truncate | |
| to avoid ValueError. |
| padding="max_length", | ||
| truncation=True, | ||
| max_length=32, | ||
| ) |
There was a problem hiding this comment.
This adds truncation for the CLASSIC prediction path as well, but the new unit test only asserts truncation during fit() in VISUAL_EXEMPLAR mode. Please add a test that exercises _predict_classic with a long category name (e.g., via fitted category_mapping or per-sample categories) and asserts the tokenizer is called with truncation enabled, to prevent regressions in the path this change also affects.
# Conflicts: # library/src/instantlearn/models/sam3/sam3.py
Fixes the error when bounding boxes are passed to SAM3
Pull Request
Description
Type of Change
feat- New featurefix- Bug fixdocs- Documentationrefactor- Code refactoringtest- Testschore- MaintenanceRelated Issues
Breaking Changes
Examples
Screenshots