feat: rework python bindings - #2391
Conversation
6f591e2 to
20cd29c
Compare
|
I could confirm that pickling, multiprocessing and pickle-dump-hash determinism are all good wrt I can try e2e with |
| pipeline: Pipeline, | ||
| // Needs a mutex so a concurrent thread can access the value while | ||
| // encode is running. | ||
| padding: Mutex<Option<PaddingParams>>, |
There was a problem hiding this comment.
fine, its also fine if its just copied across threads tbf
| tokenizer.save("./path/to/directory/my-bpe.tokenizer.json") | ||
| from tokenizers import Padding, Tokenizer | ||
|
|
||
| tokenizer = Tokenizer.from_file("tokenizer.json") |
There was a problem hiding this comment.
needs from pretrained + a big disclaimer that this is v1 only!
| with multiprocessing.get_context("spawn").Pool(4) as pool: | ||
| encoded = pool.starmap(encode, [(tokenizer, chunk) for chunk in chunks]) |
There was a problem hiding this comment.
we need to check, potentially disabling the parallelism automatically when in subprocess -> user wants to handle it himself
|
Hey @ArthurZucker I addressed some of your concerns:
PTAL at those changes 🤗 |
…at/rework-python-bindings
…at/rework-python-bindings
…at/rework-python-bindings
cf1c073 to
8145032
Compare
There was a problem hiding this comment.
Mmm interesting that this is no longer needed
There was a problem hiding this comment.
There was a problem hiding this comment.
😢 hahah all the efforts I did to get this right I am sadddd
🚨 Workflow security review — 4 critical, 3 high, 3 medium, 4 low, 1 informationalScanned 1 workflow file(s) with zizmor, pinact, OSV/GHSA, Claude, in full.
|
ArthurZucker
left a comment
ArthurZucker
left a comment
There was a problem hiding this comment.
One thing that we want to keep API wise are the getters like:
#[getter]
fn get_ids(&self) -> Vec<u32> {
self.encoding.get_ids().to_vec()
}and then in python tok.encode(["hey", "heyyyyyy"]).ids.
Also, if we don't need anything else than ids. we could also have a "id_only" -> no allocation for attention mask, no encoding, no type ids -> could be faster!
For the repr i'd say let's have \n !
Interestingly we can't do tokenize anymore! That's something we'll add back in v1, but it can be as easy as just taking the vocab-decoded id (no decoder). I'll add it IMO it can be nice.
Someting missing in decode is to take list of list. Only supports numpy list, but not batch. SO either decode batch, or just support both.
role_to_tokenneeds to get exposed! probably just overwrite the getter !
this is also fine!
There was a problem hiding this comment.
Let's maybe show an example of how transformers does this instead since that's where it lives!
| from numpy import integer, uint32 | ||
| from numpy.typing import NDArray |
There was a problem hiding this comment.
mmm numpy should not be a hard dep!!!! not sure how easy that's to do 😐
3 participants
TL;DR
Rework of the Python bindings to make them slimmer (minimalistic API)
Some caveats: