Fix sentencepiece >= 0.2.2 version compatibility #2847
Fix sentencepiece >= 0.2.2 version compatibility #2847Nikhilyammani007 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the version constraint on sentencepiece and updates SentencePieceTokenizer to support both the older SWIG wrapper and the newer PyBind11 wrapper. However, the proposed implementation for the PyBind11 wrapper passes unsupported arguments (out_type, add_bos, add_eos) to the SentencePieceProcessor constructor, which will raise a TypeError. Additionally, without setting out_type, the processor defaults to integer IDs during encoding. A robust delegation wrapper is suggested to handle these compatibility issues correctly.
| else: | ||
| # Newer PyBind11 wrapper (>=0.2.2) | ||
| self._sentence_piece_spm = spm.SentencePieceProcessor( | ||
| model_proto=proto, | ||
| out_type=out_type, | ||
| add_bos=self.add_bos, | ||
| add_eos=self.add_eos, | ||
| ) |
There was a problem hiding this comment.
In sentencepiece >= 0.2.2 (which uses the PyBind11 wrapper), the SentencePieceProcessor constructor does not accept out_type, add_bos, or add_eos as arguments. Passing these arguments will raise a TypeError (e.g., TypeError: __init__() got an unexpected keyword argument 'out_type').
Additionally, since the Init method is removed in the PyBind11 wrapper, we cannot set a default out_type on the processor instance. If we do not pass out_type to Encode, it will default to integer IDs even when self.compute_dtype is a string type, leading to incorrect tokenization behavior.
To fix this robustly without modifying code outside the diff hunks, we can use a lightweight delegation wrapper for the PyBind11 processor that intercepts calls to Encode and injects the correct out_type.
| else: | |
| # Newer PyBind11 wrapper (>=0.2.2) | |
| self._sentence_piece_spm = spm.SentencePieceProcessor( | |
| model_proto=proto, | |
| out_type=out_type, | |
| add_bos=self.add_bos, | |
| add_eos=self.add_eos, | |
| ) | |
| else: | |
| # Newer PyBind11 wrapper (>=0.2.2) | |
| class PyBind11Processor: | |
| def __init__(self, processor, out_type): | |
| self._processor = processor | |
| self._out_type = out_type | |
| def Encode(self, *args, **kwargs): | |
| if "out_type" not in kwargs: | |
| kwargs["out_type"] = self._out_type | |
| return self._processor.Encode(*args, **kwargs) | |
| def __getattr__(self, name): | |
| return getattr(self._processor, name) | |
| processor = spm.SentencePieceProcessor() | |
| processor.LoadFromSerializedProto(proto) | |
| self._sentence_piece_spm = PyBind11Processor(processor, out_type) |
References
- Poke Holes in the Implementation & Demand Robustness: Actively search for and point out failing edge cases or unhandled exceptions in the implementation, and do not accept fragile code. (link)
e5ef4c5 to
b287798
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request unpins the sentencepiece dependency constraint (previously <0.2.2) and updates SentencePieceTokenizer to initialize spm.SentencePieceProcessor directly via its constructor instead of calling .Init(). The reviewer notes that passing initialization arguments directly to the constructor is only supported in sentencepiece >= 0.2.2 and will cause a TypeError on older versions. To maintain backward compatibility since the dependency is unpinned rather than strictly upgraded, the reviewer suggests implementing a try-except block to fall back to .Init() if a TypeError occurs.
| out_type = str if is_string_dtype(self.compute_dtype) else int | ||
| self._sentence_piece_spm = spm.SentencePieceProcessor( | ||
| model_proto=proto, | ||
| out_type=str if is_string_dtype(self.compute_dtype) else int, | ||
| out_type=out_type, | ||
| add_bos=self.add_bos, | ||
| add_eos=self.add_eos, | ||
| alpha=1.0, | ||
| ) |
There was a problem hiding this comment.
The proposed change removes the .Init() call and passes the initialization arguments directly to the spm.SentencePieceProcessor constructor. While this works for sentencepiece >= 0.2.2, it will raise a TypeError on older versions of sentencepiece (e.g., < 0.2.2) because their constructor does not accept these keyword arguments. Since the version constraint has been completely unpinned (rather than being set to >=0.2.2), we must maintain backward compatibility for users with older versions of sentencepiece installed.
We can achieve this robustly by using a try-except block to attempt the new constructor-based initialization first, and fall back to the .Init() method if a TypeError is raised.
| out_type = str if is_string_dtype(self.compute_dtype) else int | |
| self._sentence_piece_spm = spm.SentencePieceProcessor( | |
| model_proto=proto, | |
| out_type=str if is_string_dtype(self.compute_dtype) else int, | |
| out_type=out_type, | |
| add_bos=self.add_bos, | |
| add_eos=self.add_eos, | |
| alpha=1.0, | |
| ) | |
| out_type = str if is_string_dtype(self.compute_dtype) else int | |
| try: | |
| self._sentence_piece_spm = spm.SentencePieceProcessor( | |
| model_proto=proto, | |
| out_type=out_type, | |
| add_bos=self.add_bos, | |
| add_eos=self.add_eos, | |
| ) | |
| except TypeError: | |
| self._sentence_piece_spm = spm.SentencePieceProcessor() | |
| self._sentence_piece_spm.Init( | |
| model_proto=proto, | |
| out_type=out_type, | |
| add_bos=self.add_bos, | |
| add_eos=self.add_eos, | |
| alpha=1.0, | |
| ) |
References
- Demand Robustness: Do not accept fragile code. If the proposed code is not robust enough or lacks proper error handling, explicitly tell the author why the current approach is brittle and what must be done to reinforce it.
laxmareddyp
left a comment
There was a problem hiding this comment.
Don't use try/except TypeError for version branching
Using try/except to detect API versions is an not good coding practice, it hides real bugs and it makes the code harder to reason about. Need code changes here. Understand the requirement with new versioning and alpha=1.0 is missing from both branches
The original code explicitly set alpha=1.0 and Your code dropped it from both branches.
Description of the change
Context: In PR #2800, we temporarily forced KerasHub to use older versions of sentencepiece (below 0.2.2) to stop the automated tests from crashing. Now that this PR have fixed the underlying code , It removes that temporary restriction so we can safely support the newest versions of sentencepiece moving forward.
Reference:
Resolves issues caused by the
0.2.2update. Replaces the temporary hotfix from PR #2800 .Changes in this PR:
Removes the version lock: unpinned sentencepiece in both requirements-common.txt and pyproject.toml so we can use the latest releases.
Updates the code: sentencepiece >= 0.2.2 removed the .Init() method. This PR removes the deprecated .Init() branching from SentencePieceTokenizer
Keeps compatibility: The tokenizer now passes model_proto and other arguments directly to the SentencePieceProcessor constructor. It initializes on both older versions (< 0.2.2) and newer versions (>= 0.2.2).
Colab Notebook:
Gist
Checklist