Fix Python workflow for WordPieceTokenizer - #2963
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a pure Python tokenization and detokenization workflow to WordPieceTokenizer so it can run without tensorflow_text installed, and includes matching unit tests. The review feedback points out two robustness issues in the input canonicalization logic: strict type checks in _canonicalize_python_detokenize_inputs can fail with NumPy scalar types (like np.int32), and 0D NumPy arrays can bypass tensor checks in _canonicalize_python_inputs and cause iteration errors. Suggestions were provided to make these checks more robust.
| if isinstance(inputs, (int, str, bytes)): | ||
| return [[inputs]], True | ||
| if isinstance(inputs, (list, tuple)): | ||
| if len(inputs) == 0 or isinstance(inputs[0], (int, str, bytes)): | ||
| return [list(inputs)], True | ||
| return [list(x) for x in inputs], False |
There was a problem hiding this comment.
Using strict type checks like isinstance(inputs, (int, str, bytes)) and isinstance(inputs[0], (int, str, bytes)) is fragile because it fails for non-standard scalar types such as NumPy integer/string scalars (e.g., np.int32, np.str_).
Specifically, if inputs is a 1D list of NumPy integers (e.g., [np.int32(1), np.int32(2)]), isinstance(inputs[0], (int, str, bytes)) evaluates to False. This causes the function to incorrectly treat the input as a 2D batch and execute [list(x) for x in inputs], which tries to call list(np.int32(1)) and raises TypeError: 'numpy.int32' object is not iterable.
Using negative checks (i.e., checking if the input or its elements are NOT containers like list, tuple, or np.ndarray) provides a robust and future-proof way to identify scalar values and 1D sequences of scalars.
| if isinstance(inputs, (int, str, bytes)): | |
| return [[inputs]], True | |
| if isinstance(inputs, (list, tuple)): | |
| if len(inputs) == 0 or isinstance(inputs[0], (int, str, bytes)): | |
| return [list(inputs)], True | |
| return [list(x) for x in inputs], False | |
| if not isinstance(inputs, (list, tuple, np.ndarray)): | |
| return [[inputs]], True | |
| if isinstance(inputs, (list, tuple)): | |
| if len(inputs) == 0 or not isinstance(inputs[0], (list, tuple, np.ndarray)): | |
| return [list(inputs)], True | |
| return [list(x) for x in inputs], False |
References
- Poke Holes in the Implementation: Your goal is to critically test the logic. Actively search for and point out failing edge cases, race conditions, or unhandled exceptions in the implementation. (link)
| else: | ||
| inputs = inputs.tolist() | ||
|
|
||
| if isinstance(inputs, bytes): |
There was a problem hiding this comment.
If inputs is a raw np.ndarray (e.g., when passed directly or from non-TF backends), a 0D array (like np.array("hello")) will bypass the tf.Tensor check and cause a TypeError: iteration over a 0-d array when iterating in the self.split block.
Converting any np.ndarray to a Python scalar or list at the entry point of the canonicalization path resolves this and ensures consistent behavior across all backends.
| if isinstance(inputs, bytes): | |
| if isinstance(inputs, np.ndarray): | |
| inputs = inputs.item() if inputs.ndim == 0 else inputs.tolist() | |
| if isinstance(inputs, bytes): |
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. (link)
- When handling inputs that may be passed as tensors in a Python-only execution path, perform defensive type conversion (e.g., converting to int) at the entry point of the path rather than deep inside helper functions, especially if multiple downstream helper functions perform arithmetic on the value.
| split_regex = re.compile( | ||
| f"({keep_split_pattern})|(?:{WHITESPACE_REGEX})" | ||
| ) | ||
| words = [piece for piece in split_regex.split(text) if piece] |
There was a problem hiding this comment.
Unicode whitespace — Python's \s is Unicode-wide; RE2's is ASCII-only. "the\u00a0fox" → Python [the, fox], TF [UNK] (same for U+2009, U+3000). Fix: a Python-side whitespace pattern using the regex module's scoped ASCII flag, (?a:\s)|\p{Cc}|\p{Cf}.
There was a problem hiding this comment.
Hi @divyashreepathihalli , Thank you for your Review!
Separated whitespace patterns into WHITESPACE_REGEX_TF(using standard ASCII characters for RE2/TensorFlow Text compatibility) and
WHITESPACE_REGEX_PYTHON(using the regex module with the scoped ASCII flag (?a:\s)|\p{Cc}|\p{Cf}) to ensure exact parity across both workflows without breaking RE2 compilation.
| for word in words | ||
| ] | ||
| else: | ||
| words = [word.casefold() for word in words] |
There was a problem hiding this comment.
str.casefold() ≠ tf_text.case_fold_utf8 (NFKC_CaseFold). "FOX fox" → Python [UNK, fox], TF [fox, fox]. Fix: unicodedata.normalize("NFKC", word.casefold())
There was a problem hiding this comment.
Updated the Python case-folding logic to apply NFKC normalization via unicodedata.normalize("NFKC", word.casefold()), matching the behavior of tf_text.case_fold_utf8.
| vocabulary_set, | ||
| unknown_token, | ||
| suffix_indicator, | ||
| max_input_chars_per_word=200, |
There was a problem hiding this comment.
Python bails at 200 chars; FastWordpieceTokenizer defaults to max_bytes_per_word=100. A 120-byte word tokenizes on Python, [UNK] on TF. Fix: len(word.encode("utf-8")) > 100
There was a problem hiding this comment.
Replaced character length bounds with UTF-8 byte-length checks (len(word.encode("utf-8")) > 100) in the pure Python WordPiece loop.
| output = tokenizer(np.bytes_("the quick")) | ||
| self.assertAllEqual(output, [1, 2, 3]) | ||
|
|
||
| def test_python_workflow_with_seq_len_returns_dense_output(self): |
There was a problem hiding this comment.
missing WordPieceTokenizerTFTest subclass re-running the suite with _allow_python_workflow=False
There was a problem hiding this comment.
Added the WordPieceTokenizerTFTest test case subclass to the test suite to ensure the entire test set runs and passes with _allow_python_workflow=False.
| output = tokenizer.detokenize([1, 2, 3]) | ||
| self.assertAllEqual(output, "the quick") | ||
|
|
||
| def test_python_and_tf_workflows_match(self): |
There was a problem hiding this comment.
parity test too thin (add CJK, accents, special tokens, unicode whitespace, >100-byte words)
There was a problem hiding this comment.
Expanded test_python_and_tf_workflows_match to validate parity across CJK characters, accent stripping, special tokens, unicode whitespace(\u00a0), and >100-byte words.
| # workflow (`_tokenize_python`/`_detokenize_python`), which does not | ||
| # require `tensorflow_text` to be installed. | ||
| self._vocabulary_set = set(self.vocabulary) | ||
| self._token_to_id_python = { |
There was a problem hiding this comment.
duplicate-token token_to_id semantics
| else: | ||
| tokens = batched_tokens | ||
| if output_is_int and self.sequence_length: | ||
| return np.asarray(tokens, dtype=self.compute_dtype) |
There was a problem hiding this comment.
np.asarray → backend tensor via call
There was a problem hiding this comment.
Replaced raw NumPy conversion (np.asarray) with Keras tensor (keras.ops.convert_to_tensor).
This PR adds a Python workflow for
WordPieceTokenizer, allowing tokenization and detokenization to run without requiringtensorflow_text.Fixes: #2948
Changes :
tensorflow_text.tensorflow_textoptional for the Python workflow.tensorflow_textworkflow.tf.function.detokenization.
.numpy()values frombytesto Pythonstr.Tests :
tensorflow_text.Checklist