search

LCA-ActivityBrowser / activity-browser

GUI for brightway2
GNU Lesser General Public License v3.0
134 stars 50 forks source link

Segmentation fault on python 3.10 when accessing sqlite database from QThread #1124

Closed haasad closed 6 months ago

haasad commented 7 months ago

This is a write-up for a bug that I've been trying to track down for a long time. The bug leads to a segmentation fault when accessing a brightway2 database from a QThread when the thread is finished. This bug is currently blocking the AB from using more recent python versions, see also #726 and #727.

In all likelihood it is the same bug discussed in #867 and I'm reasonably certain that the updates to the AB logging system (#1011) unfortunately didn't fix it. However it is still possible that the print calls from QThreads also used to cause a similar problem under certain circumstances.

As mentioned by @Pan6ora in #867 the segmentation fault only happens rarely with the current AB and only on some combinations of hardware + operating system. This makes it extremely hard to debug and fix.

However with python 3.10 the segmentation fault happens every time and on all operating systems. It affects (at least) the following operations in the AB:

I'll summarize my findings here in this issue.

haasad commented 7 months ago

Steps to reproduce:

haasad commented 7 months ago

I have created a repository https://github.com/haasad/AB_issue1124 where I can run some tests on all operating systems with github actions.

I'm trying to isolate the problematic operations with a minimal example to reproduce the error. Current state is as follows:

Remarks:

haasad commented 7 months ago

I was able to narrow it down further. Having an open db connection with peewee.SqliteDatabase in a QThread causes the segfault, but only on python 3.10.

import sys

from peewee import SqliteDatabase
from PySide2.QtCore import QThread
from PySide2 import QtWidgets

db = SqliteDatabase('my_database.db')

class DefaultBiosphereThread(QThread):
    def run(self):
        db.connect()

app = QtWidgets.QApplication(sys.argv)
thread = DefaultBiosphereThread()
thread.finished.connect(app.exit)
thread.start()
sys.exit(app.exec_())

https://github.com/haasad/AB_issue1124/actions/runs/6870147083

marc-vdm commented 7 months ago

This bug is currently blocking the AB from using more recent python versions

I just had a try (#1146) and it seems that py 3.11 works just fine.

For 3.12, AB won't work, as bw2io and other BW packages are set to numpy==1.23, which only supports up to 3.11. I don't expect to see new bw legacy updates soon, so we shouldn't count on this being resolved for a while. In the mid-term, we're also hoping to move to bw25, so I don't see lacking support for 3.12 as a big issue.

Would there be objections to only supporting py 3.11 in AB?

haasad commented 7 months ago

Depends what exactly you mean by this :smile:

Would there be objections to only supporting py 3.11 in AB?

If we're just talking about skipping 3.10, then I think it's safe enough to try. It bothers me that I couldn't find the root cause and maybe there is some underlying issue that we're missing but so far everything points to some freak interaction that only happens with 3.10.

If you mean also dropping support for 3.8 and 3.9 like in the MR you linked then I have a pretty big objection. AFAIK conda update --all and conda update activity-browser won't change the python version of an environment. So all users with existing installations will be stuck with the last AB version before the switch to 3.11, unless we can convince them to re-install the AB. We had a lot of these issues when we dropped support for 3.7. See #704 and related issues.

marc-vdm commented 7 months ago

If you mean also dropping support for 3.8 and 3.9 like in the MR you linked then I have a pretty big objection. AFAIK conda update --all and conda update activity-browser won't change the python version of an environment. So all users with existing installations will be stuck with the last AB version before the switch to 3.11, unless we can convince them to re-install the AB. We had a lot of these issues when we dropped support for 3.7. See #704 and related issues.

Right, I had forgotten about this. That wasn't great.

AFAIK conda update --all and conda update activity-browser won't change the python version

This is true, the only way to update within an env seems to be conda install python=3.11 (https://stackoverflow.com/a/76207918) -and I just tried, to be sure-.

For the short term, how about this?: I will add support for 3.11 in addition to 3.8, 3.9, --> I'll ask for your review on #1146

Mid/Long-term, 3.8 and 3.9 will stop receiving security updates. While I don't see immediate risks with AB as open-source software that runs locally, keeping support for these versions may not be best practice.

However, we are looking at a big change in -what I expect to be- the same time-line in the switch to bw2.5. I think we should re-evaluate the versions and perhaps just bite the bullet and switch by then.

Luckily, that's a problem for later-us to figure out.

haasad commented 7 months ago

Yes, once 3.8 is EOL it makes sense to stop supporting it. But there's no reason to do so earlier in my opinion.

If there's a big breaking change with a new major release for the AB (as there definitely will be with bw2.5) I think it makes sense to stop supporting older python versions. Also there's no real benefit for AB to support multiple python versions, maybe it makes sense to then decide to only support 3.11 (or 3.12 if all deps are ready).

marc-vdm commented 7 months ago

Alright then we just support 3.8 until EOL and only support the newest compatible python version once we move over to BW25 (I'm assuming 3.12 as BW25 already supports that, but perhaps by the time this is done 3.n is supported).

haasad commented 6 months ago

As we decided to skip supporting 3.10 at all I'm closing this issue