Conversation
|
The tests work! They fail on the genlm backend import Token class since it's not updated yet! Once that is merged on the backend, both will pass. |
samuki
left a comment
samuki
left a comment
There was a problem hiding this comment.
Could we change the target backend in pyproject.toml to check that all the tests are passing? Otherwise, it looks good, we just need to benchmark the speed.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
pyproject.toml
Outdated
| authors = [ { name = "Ben LeBrun", email = "benlebrun1@gmail.com" }, { name = "Tim Vieira"} ] | ||
| dependencies = [ | ||
| "genlm-backend>=0.1.1", | ||
| "genlm-backend @ git+https://github.com/genlm/genlm-backend.git@samuki/refactor-token-bytes", |
There was a problem hiding this comment.
This is fine for now, but once we push the refactoring, we’ll need to change this in the new version.
genlm/bytes/trie.py
Outdated
| decode (list[Token]): List of Token objects representing the token vocabulary. | ||
| Each Token must have both token_id and byte_string attributes. | ||
| device (str, optional): Device to use for weight sum and max computations ('cpu' or 'cuda'). | ||
| atomic_tokens (list[bytes], optional): List of tokens that should be treated as atomic units rather than being split into bytes. |
There was a problem hiding this comment.
atomic_tokens is a list[bytes], not a list of tokens. Now that we are doing this refactoring, this may be a little bit confusing. Can we rename this to something like atomic_byte_str? In general, I think it is better to refer to tokens when we have token_ids and byte_str when we have bytes in this repo. Same applies to other parameters, for example eos_tokens and eot_token, it would be good to rename them
pyproject.toml
Outdated
| authors = [ { name = "Ben LeBrun", email = "benlebrun1@gmail.com" }, { name = "Tim Vieira"} ] | ||
| dependencies = [ | ||
| "genlm-backend>=0.1.1", | ||
| "genlm-backend @ git+https://github.com/genlm/genlm-backend.git@samuki/refactor-token-bytes", |
shepardxia
left a comment
shepardxia
left a comment
There was a problem hiding this comment.
Looks good to me! The only change needed before pushing is the pyproject.toml deps.
5 participants
Supporting duplicate tokens (using the Token class in genlm-backend on the branch: genlm/genlm-backend#57). Also supporting multiple extends when multiple EOT's are found.