Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use OnceLock in imports. #13776

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kevinhartman
Copy link
Contributor

@kevinhartman kevinhartman commented Jan 31, 2025

Summary

It appears that GILOnceCell is not sufficient for module import and initialization. When running free-threaded Python in unit tests, CPython throws a _DeadLockError during module import. Even though GILOnceCell allows threads to race and the first one to finish gets the initialization, it appears that the import process itself is not reentrant.

Details and comments

This may also require a bug report on PyO3, since its implementation of GILOnceCell::import does what we were doing previously.

This issue was discovered by @raynelfss when using Rust PyO3 tests on Linux.

It appears that GILOnceCell is not sufficient for module import
and initialization. When running free-threaded Python in unit
tests, CPython throws a _DeadLockError during module import.
Even though GILOnceCell allows threads to race and the first
one to finish gets the initialization, it appears that the
import process itself is not reentrant.

This may also require a bug report on PyO3, since its
implementation of GILOnceCell::import does what we were
doing previously.
@kevinhartman kevinhartman requested a review from a team as a code owner January 31, 2025 17:39
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 13077893147

Details

  • 12 of 13 (92.31%) changed or added relevant lines in 4 files are covered.
  • 28 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.03%) to 88.8%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/imports.rs 8 9 88.89%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/two_qubit_decompose.rs 1 92.08%
crates/accelerate/src/unitary_synthesis.rs 1 92.97%
crates/qasm2/src/lex.rs 2 92.73%
crates/qasm2/src/parse.rs 24 96.22%
Totals Coverage Status
Change from base Build 13072040346: -0.03%
Covered Lines: 79748
Relevant Lines: 89806

💛 - Coveralls

@mtreinish
Copy link
Member

Is there any performance impact with this change. We don't actually support free threaded python and I don't think it's something we have immediate plans for. So I'm curious if there are side effects doing and what if any tradeoffs there are.

@kevinhartman
Copy link
Contributor Author

@mtreinish I'll run some benchmarks and report back. The other thing we can do is run the Rust tests with --test-threads 1 by default if this approach has a negative perf impact.

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