Tokenizer.Decode special-cases EnglishRoberta

This suggests there's something wrong with the Model abstraction, and it means that any other model of a similar ilk to EnglishRoberta could not be supported (or not supported efficiently, or whatever reason caused this to be special-cased here). The special-casing should be removed and the abstraction fixed to make the special-casing unnecessary.

machinelearning/src/Microsoft.ML.Tokenizers/Tokenizer.cs

Lines 203 to 216 in 4635a86

    
           if (Model is EnglishRoberta robertaModel) 
        
           { 
        
               foreach (int id in ids) 
        
               { 
        
                   tokens.Add(robertaModel.IdToFilteredToken(id, skipSpecialTokens) ?? ""); 
        
               } 
        
           } 
        
           else 
        
           { 
        
               foreach (int id in ids) 
        
               { 
        
                   tokens.Add(Model.IdToToken(id, skipSpecialTokens) ?? ""); 
        
               } 
        
           }

@tarekgh

cc: @tarekgh

@stephentoub

@stephentoub this was in the abstraction and I removed it from there because the functionality was specific to this tokenizer only. This is why I made it this way. I have no idea ever any other tokenizers will need that. If we see cases like this in the future we can decide to add it to the abstraction. But for now I suggest not adding it.

@michaelgsharp

The other idea is to have Model.IdToToken always return the filtered token in EnglishRoberta tokenizer.

CC @michaelgsharp @LittleLittleCloud

@stephentoub

@stephentoub this was in the abstraction and I removed it from there because the functionality was specific to this tokenizer only. This is why I made it this way. I have no idea ever any other tokenizers will need that. If we see cases like this in the future we can decide to add it to the abstraction. But for now I suggest not adding it.

There are only three tokenizers in the library. If 1/3rd of them needs whatever this is, seems likely another will as well. And if someone wanted to provide their own EnglishRoberta equivalent (maybe faster, or with subtly different semantics), how would they?

I just noticed that Tiktoken is also special-cased by Decode. That's two models that Decode is special-casing. Certainly seems like there's something wrong with the abstraction.

Ok, will think about these two cases.

Thanks :)

Navigation Menu

Repository navigation

Tokenizer.Decode special-cases EnglishRoberta #7010

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if (Model is EnglishRoberta robertaModel)
	{
	foreach (int id in ids)
	{
	tokens.Add(robertaModel.IdToFilteredToken(id, skipSpecialTokens) ?? "");
	}
	}
	else
	{
	foreach (int id in ids)
	{
	tokens.Add(Model.IdToToken(id, skipSpecialTokens) ?? "");
	}
	}

Tokenizer.Decode special-cases EnglishRoberta #7010

Description

Activity

stephentoub commented on Feb 16, 2024

tarekgh commented on Feb 16, 2024

tarekgh commented on Feb 16, 2024

stephentoub commented on Feb 16, 2024

stephentoub commented on Feb 17, 2024

tarekgh commented on Feb 17, 2024

stephentoub commented on Feb 17, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions