Skip to content

feat: rework python bindings - #2391

Open
SBrandeis wants to merge 26 commits into
feat/train_encode_splitfrom
feat/rework-python-bindings
Open

feat: rework python bindings#2391
SBrandeis wants to merge 26 commits into
feat/train_encode_splitfrom
feat/rework-python-bindings

Conversation

@SBrandeis

@SBrandeis SBrandeis commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Rework of the Python bindings to make them slimmer (minimalistic API)

Some caveats:

  • No pickle / mutli process support for now
  • Unclear whether this runs properly on free-threaded python

@SBrandeis
SBrandeis force-pushed the feat/rework-python-bindings branch from 6f591e2 to 20cd29c Compare September 3, 2026 11:13
@SBrandeis
SBrandeis marked this pull request as ready for review September 3, 2026 12:18
@SBrandeis SBrandeis changed the title python bindings v2 feat: rework python bindings Sep 3, 2026
@lhoestq

lhoestq commented Sep 3, 2026

Copy link
Copy Markdown
Member

I could confirm that pickling, multiprocessing and pickle-dump-hash determinism are all good wrt datasets.

I can try e2e with transformers tokenizers once ready !

Comment thread bindings/python/python/tokenizers/tokenizers.pyi
pipeline: Pipeline,
// Needs a mutex so a concurrent thread can access the value while
// encode is running.
padding: Mutex<Option<PaddingParams>>,

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.

fine, its also fine if its just copied across threads tbf

Comment thread bindings/python/tests/test_tokenizer.py Outdated
Comment thread bindings/python/tests/test_tokenizer.py Outdated
Comment thread bindings/python/tools/stub-gen/Cargo.toml
Comment thread bindings/python/Makefile
Comment thread bindings/python/README.md
tokenizer.save("./path/to/directory/my-bpe.tokenizer.json")
from tokenizers import Padding, Tokenizer

tokenizer = Tokenizer.from_file("tokenizer.json")

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.

needs from pretrained + a big disclaimer that this is v1 only!

Comment thread bindings/python/README.md
Comment thread bindings/python/README.md
Comment on lines +64 to +65
with multiprocessing.get_context("spawn").Pool(4) as pool:
encoded = pool.starmap(encode, [(tokenizer, chunk) for chunk in chunks])

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.

we need to check, potentially disabling the parallelism automatically when in subprocess -> user wants to handle it himself

@SBrandeis

Copy link
Copy Markdown
Contributor Author

Hey @ArthurZucker I addressed some of your concerns:

  • verbose tests
  • from_pretrained
  • stub_gen built as a runnable binary

PTAL at those changes 🤗

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.

3 participants