Skip to content

Conversation

@pavoljuhas
Copy link
Member

Use atexit for cleanup of Python-extended prototypes

Use standard atexit handler to remove Python-extended prototype
objects from C++ registry. Ensure cleanup is truly executed
instead of relying on garbage collector to call __del__.

This prevents the following error message on Python exit

Fatal Python error: PyThreadState_Get: the function must be called
with the GIL held, but the GIL is released (the current Python
thread state is NULL)

Use standard atexit handler to remove Python-extended prototype
objects from C++ registry.  Ensure cleanup is truly executed
instead of relying on garbage collector to call `__del__`.

This prevents the following error message on Python exit

    Fatal Python error: PyThreadState_Get: the function must be called
    with the GIL held, but the GIL is released (the current Python
    thread state is NULL)
Can as well test with the builtin complex here.
@pavoljuhas pavoljuhas requested a review from sbillinge August 15, 2024 23:16
@pavoljuhas pavoljuhas mentioned this pull request Aug 15, 2024
@Sparks29032
Copy link

Sparks29032 commented Aug 15, 2024

Thank you so much.

@pavoljuhas
Copy link
Member Author

@Sparks29032 - are you still getting the GIL error?

Here is how I check on my side on Debian Linux in a fresh conda environment for Python 3.10 -

# create fresh conda environment "tgil" with Python 3.10
conda create -n tgil "python==3.10"
conda activate tgil
conda install boost gsl numpy scons setuptools
pip install diffpy.structure periodictable pycifrw

# tell c++ compiler to use tgil libraries
P="$(conda info --json | grep default_prefix | cut -d\" -f4)"
export CPATH="$P/include"
export LIBRARY_PATH="$P/lib"
export LDFLAGS="-Wl,-rpath,$P/lib"

# install libdiffpy to the tgil
mkdir /tmp/repos
cd /tmp/repos
git clone https://github.com/diffpy/libdiffpy.git
cd libdiffpy
git checkout main
scons -j4 prefix=$P install

# build srreal in development mode and have Python use repo sources
cd /tmp/repos
git clone https://github.com/diffpy/diffpy.srreal.git
cd diffpy.srreal
git checkout main
export PYTHONPATH="${PWD}/src:${PYTHONPATH}"
scons -j4 develop

# these commands give "Fatal Python error ... GIL is released"
python -m diffpy.srreal.tests.run
python -c "import diffpy.srreal.atomradiitable"

# the same commands show no GIL error at this PR
git fetch origin refs/pull/28/head:pr28
git checkout pr28
python -m diffpy.srreal.tests.run
python -c "import diffpy.srreal.atomradiitable"

@Sparks29032
Copy link

Sparks29032 commented Aug 16, 2024

@pavoljuhas Nope, this fixed it.

Tested on my Ubuntu (local) and had conda-forge build and test (importing diffpy.srreal.srreal_ext and diffpy.srreal.atomradiitable) for OSX. Windows tests are still skipped.

Failures on the conda-forge are due to use of numpy.distutils and occur only for Python 3.12. Once we merge this PR, let's fix that and do a release!

Again, thank you so much!

@sbillinge sbillinge merged commit 9f09b73 into diffpy:main Aug 16, 2024
@sbillinge
Copy link
Contributor

Thanks @pavoljuhas @Sparks29032 I feel like it is my birthday and Christmas all rolled in one!

@pavoljuhas pavoljuhas deleted the fix-cleanup-at-exit branch August 16, 2024 20:27
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