Skip to content

Conversation

ngoldbaum
Copy link

@ngoldbaum ngoldbaum commented Jun 23, 2025

c.f. #1784.

This PR updates the tests to be runnable in a thread pool under pytest-run-parallel. It then makes use of pytest-run-parallel to do multithreaded testing on the free-threaded build in CI.

The updates to the tests are mostly to avoid sharing filesystem paths between test runners. I also marked all the multiprocessing tests as thread-unsafe because they rely on fork and they rely on mutating os.environ.

To avoid running into runtime borrow-checker errors, I wrapped the internal state of the PyEncoder pyclass in a Mutex. I don't think it's possible for any panics to happen while mutexes are locked that weren't already possible and in all cases I think it's correct to immediately propagate panics with unlock() rather than trying to handle poisoned mutexes.

@ngoldbaum
Copy link
Author

HI all, anything I can do here to move this along? I'll go ahead and rebase in case that helps.

@ArthurZucker
Copy link
Collaborator

Sorry for the delay!

@ngoldbaum
Copy link
Author

Sorry for the delay!

No worries. I might look at adding 3.14t as well in the meantime...

@WSL0809
Copy link

WSL0809 commented Aug 25, 2025

any plans to add support 3.14t ?

@ngoldbaum
Copy link
Author

any plans to add support 3.14t ?

That would probably have to wait on at least pytorch and/or jax adding 3.14t support.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

be runnable in a thread pool under pytest-run-parallel.

I don't know if this is a strong enough motivations but happy to work towards thread free! I am quite surprised that we only need the mutex on the encodings, which are kinda the output, after everything was run !

@ngoldbaum
Copy link
Author

That would probably have to wait on at least pytorch and/or jax adding 3.14t support.

Argh, sorry, I thought I was commenting on the safetensors repo...

That said, installing on 3.14t is still broken. It looks like hf-xet still needs to do a release that bumps the pyo3 version to prevent a build error during the tokenizers install. If you disable the hf-xet dependency then everything seems to build fine.

I don't know if this is a strong enough motivations but happy to work towards thread free!

IMO it's important to have at least some multithreaded test coverage over a good chunk of the API while adding free-threaded support. pytest-run-parallel is an imperfect but automated way to do that.

You can do things like set up a Python and/or Rust build with thread sanitizer instrumentation to get finer-grained thread safety checking. Unfortunately it's not as straightfoward to use TSAN with rust code as it is for a pure C/C++ extension because the Rust toolchain has its own vendored LLVM toolchain.

I am quite surprised that we only need the mutex on the encodings, which are kinda the output, after everything was run !

I don't think I tried to intentionally break tokenizers like that, I'm only adding mutexes in spots that I triggered runtime borrow-checker errors when I did the pytest-run-parallel mulithreaded testing exercise. Looking closer at the other pyclasses, I bet there are lots more opportunities to trigger issues.

One thing we can do is try to construct some multithreaded tests that look like real workflows where you expect users to use Python threads. We can also try to write some stress tests if there are any scenarios you're worried about where multithreaded mutation of shared state would possibly break things or where you want tokenizers to be robust.

Do you have any scenarios like that in mind?

@ngoldbaum
Copy link
Author

I'll try to find a quick contribution elsewhere to make so that CI runs in this PR automatically 🙃

@ngoldbaum
Copy link
Author

4971dc2 does a little bit more of an extensive change on top of what was already here and marks some of the pyclasses as frozen. See https://pyo3.rs/v0.25.1/class.htmlfrozen-classes-opting-out-of-interior-mutability for more about this.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

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.

4 participants