silx-kit / silx

silx toolkit
http://www.silx.org/doc/silx/latest/
MIT License
119 stars 70 forks source link

tests failures against Pytest 8.2.0 #4128

Closed stanislavlevin closed 3 weeks ago

stanislavlevin commented 1 month ago

As of Pytest 8.2.0 (bisected to https://github.com/pytest-dev/pytest/pull/12096) silx' tests suite fails, for example:

(.run_venv) [builder@localhost python3-module-silx-2.1.0]$ python run_tests.py --installed -ra -Wignore --low-mem .run_venv/lib/python3/site-packages/silx/io/test/test_specfilewrapper.py::TestSpecfilewrapper::test_number_of_scans
================================ test session starts =================================
platform linux -- Python 3.12.2, pytest-8.2.0.dev25+g0dc036035, pluggy-1.5.0
rootdir: /usr/src/RPM/BUILD/python3-module-silx-2.1.0/.run_venv/lib/python3/site-packages/silx
configfile: ../../../../../pytest.ini
plugins: mock-3.14.0, xvfb-3.0.0
collected 1 item                                                                     

.run_venv/lib/python3/site-packages/silx/io/test/test_specfilewrapper.py .     [100%]

================================= 1 passed in 0.04s ==================================
AttributeError: 'Specfile' object has no attribute 'close'
Exception ignored in: 'silx.io.specfile.SpecFile.__dealloc__'
AttributeError: 'Specfile' object has no attribute 'close

Not sure if this is silx issue and anything can be done on its side.

vallsv commented 1 month ago

Sounds like it's a call during the termination phase of python.

At this stage, access to object/module/package attributes are unpredictable. Because this objects was potentiall already released. That's a problem on it's own with the Specfile object. I mean __dealloc__ should maybe be protected against that.

But it also means this tests from test_specfilewrapper also do not clean up properly the resources. The resources should not be alive after the tests.

stanislavlevin commented 1 month ago

This is what docs say about __dealloc__: https://cython.readthedocs.io/en/latest/src/userguide/special_methods.html#finalization-methods-dealloc-and-del

You need to be careful what you do in a dealloc() method. By the time your dealloc() method is called, the object may already have been partially destroyed and may not be in a valid state as far as Python is concerned, so you should avoid invoking any Python operations which might touch the object. In particular, don’t call any other methods of the object or do anything which might cause the object to be resurrected. It’s best if you stick to just deallocating C data.

t20100 commented 1 month ago

The SpecFile.__dealloc__ code is calling a method, so clearly out of the specification:

https://github.com/silx-kit/silx/blob/755fa6d3cbe35e1078f65557f6e9e8352b826da5/src/silx/io/specfile.pyx#L661-L671

Calling close from __del__ instead of __dealloc__ should fix this.

https://cython.readthedocs.io/en/latest/src/userguide/special_methods.html#finalization-methods-dealloc-and-del

Python 3.4 made it possible for extension types to safely define finalizers for objects. When running a Cython module on Python 3.4 and higher you can add a __del__() method to extension types in order to perform Python cleanup operations. When the __del__() is called the object is still in a valid state (unlike in the case of __dealloc__()), permitting the use of Python operations on its class members. On Python <3.4 __del__() will not be called.

t20100 commented 1 month ago

Thanks for the bug report!

PR #4129 should fix it.