Skip to content

feat: Implement tokenize() / decode_tokens() - #2417

Open
SBrandeis wants to merge 4 commits into
feat/train_encode_splitfrom
feat/python-tokenize
Open

SBrandeis wants to merge 4 commits into
feat/train_encode_splitfrom
feat/python-tokenize

Conversation

@SBrandeis

Copy link
Copy Markdown
Contributor

No description provided.

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit too many unitary tests! Python should not test the rust layer, just the python layer.
Test should be quite minal please!

juste making sure the outputs are tokens, not string!

Comment on lines +188 to +189
/// Encodes the given text and returns the string representation of each token.
///

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make it return both the ids maybe? so they are not "thrown" away?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 46e0ade for an alternative

}
}

/// Decode a slice of token ids to a Vec of the tokens' string representation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect

Comment on lines +92 to +101
if !self.is_byte_level() {
return self.vocab.id_to_token(id);
}
let bytes = self.id_to_token_bytes(id)?;
Some(
bytes
.iter()
.map(|&b| BYTES_CHAR_LOOKUP[b as usize])
.collect(),
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MMM okay, you're making sure they are printable I guess.
K, makes sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matches the legacy .tokenize()

Comment thread bindings/python/src/tokenizer.rs Outdated
SBrandeis and others added 3 commits September 17, 2026 13:44
Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants