Strange implementation of convert_tokens_to_string in albert tokenizer.

[PhilipMay]

Hi,

the albert tokenizer implements the convert_tokens_to_string function:

transformers/src/transformers/models/albert/tokenization_albert.py

Lines 222 to 223 in ba0d50f

	out_string = "".join(tokens).replace(SPIECE_UNDERLINE, " ").strip()
	return out_string

While the deberta v2 and some other tokenizer just delegate this to the sentencepiece tokenizer:

transformers/src/transformers/models/deberta_v2/tokenization_deberta_v2.py

Line 146 in ba0d50f

return self._tokenizer.decode(tokens)

IMO it would be better to always delegate to the sentencepiece tokenizer. What do you think?

PS:

Some more examples here

transformers/src/transformers/models/barthez/tokenization_barthez.py

Lines 251 to 252 in ba0d50f

	out_string = "".join(tokens).replace(SPIECE_UNDERLINE, " ").strip()
	return out_string

transformers/src/transformers/models/camembert/tokenization_camembert.py

Lines 251 to 252 in ba0d50f

	out_string = "".join(tokens).replace(SPIECE_UNDERLINE, " ").strip()
	return out_string

transformers/src/transformers/models/m2m_100/tokenization_m2m_100.py

Lines 187 to 188 in ba0d50f

	out_string = "".join(tokens).replace(SPIECE_UNDERLINE, " ").strip()
	return out_string

transformers/src/transformers/models/mbart/tokenization_mbart50.py

Lines 208 to 209 in ba0d50f

	out_string = "".join(tokens).replace(SPIECE_UNDERLINE, " ").strip()
	return out_string

transformers/src/transformers/models/speech_to_text/tokenization_speech_to_text.py

Lines 169 to 173 in ba0d50f

	out_string = "".join(tokens).replace(SPIECE_UNDERLINE, " ").strip()
	if self.do_upper_case:
	out_string = out_string.upper()
	return out_string

transformers/src/transformers/models/xlm_prophetnet/tokenization_xlm_prophetnet.py

Lines 264 to 265 in ba0d50f

	out_string = "".join(tokens).replace(SPIECE_UNDERLINE, " ").strip()
	return out_string

[LysandreJik]

Indeed, you're probably right! When updating the ALBERT tokenizer to use the sentencepiece.decode instead of the manual handling - do all tests pass? Even the integration test?

Makes me think we really should have integration tests for all tokenizers, as scenarios like this one are bound to happen.

[PhilipMay]

Well yes. While "adding subword regularization in more tokenizers": #11417
I recognized that the tokenizers could benefit from some bigger refactoring.
Pulling commen functions into a base class would be nice. And while doing this adding tests....
There is lot of duplicate code there...

I might do this as a PR the next days (weeks) - we will see.

[PhilipMay]

PR with a fix started: #11716

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

[PhilipMay]

I am still working on this...

[PhilipMay]

Fixed in #11716 closing here.