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

AiiDA (Node) is not thread-safe #5765

Open
unkcpz opened this issue Nov 16, 2022 · 2 comments
Open

AiiDA (Node) is not thread-safe #5765

unkcpz opened this issue Nov 16, 2022 · 2 comments
Labels

Comments

@unkcpz
Copy link
Member

unkcpz commented Nov 16, 2022

In AiiDAlab QeApp aiidalab/aiidalab-qe#279 we launch threading to query the Node from the DB and use it afterward which lead to the issue of not persistent within this Session.

The problem caused by that the threading will close and the DB query session will close along with the threading, then the Node instance can not be used anymore since in the new design it is bound to the SQL query session. Here is a minimal example to reproduce the exception, using a list as the mutable object to mimic traitlets of QeApp widgets.

I am curious about whether is this a limitation of the design introduced in the new DB backend. @chrisjsewell I think you mentioned it is for making switching between the profile on the fly possible.

But we need to give a more detailed exception message when it is raised.

from aiida import orm
from aiida import load_profile
import threading

load_profile()

def _db_operation(mut: list):
    code = orm.load_code("doubler@localhost")
    mut.append(code)

def main():
    """Query the list of available codes."""

    mut = []
    thread = threading.Thread(target=_db_operation, args=(mut,))
    thread.start()
    thread.join()
    
    code = mut[0]
    print(code) # use the node, get attributes

    
if __name__ == "__main__":
    main()

End up with Instance '<DbNode at 0x7f714adcfdc0>' is not persistent within this Session.

@chrisjsewell
Copy link
Member

chrisjsewell commented Nov 16, 2022

Heya, I wouldn't really say this has anything to do with querying, I think you are misunderstanding what an sqlalchemy session is here; just a connection to the database

Database query is not thread-safe

Firstly, I would change this to "AiiDA is not threadsafe".
AiiDA contains a number of, non-thread-local, global variables, in particular the Manager singleton.
There's a lot less since #5330, but still you should not really be using AiiDA with threading.

Node's are also note thread safe ->

Node instance can not be used anymore since in the new design it is bound to the SQL query session

The Node is an ORM instance; it is effectively a "live" view of an object in the storage, that can fetch/mutate data on the storage.
Obviously to be live, it needs to have a link to the storage (and for psql_dos backend then the postgresql database).

The previous approach, of silently switching the connection for whatever was stored in a global variable, was absolutely not tractable with storage backend modularity and switching; a node instance is specific to a profile

If you are passing things between threads, then they need to be thread-safe, and probably "static" representations of data.

@chrisjsewell
Copy link
Member

chrisjsewell commented Nov 16, 2022

Note there is also a known problem here currently, that the psql_dos, via sqlalchemy, is generating thread safe sessions, but the backend instance itself is not threadsafe; really only aiida should be generating thread local objects.

@unkcpz unkcpz changed the title Database query is not thread-safe AiiDA is not thread-safe Nov 16, 2022
@unkcpz unkcpz changed the title AiiDA is not thread-safe AiiDA (Node) is not thread-safe Nov 16, 2022
unkcpz added a commit to aiidalab/aiidalab-widgets-base that referenced this issue Nov 16, 2022
AiiDA is not thread-safe, for the node queried from DB, it will live at the end of DB session. 
In the AiiDAlab widget, we use threading for the widget since we don't want to block the whole notebook in some long running operation. 
However, the some traitlets in the widget implementation are AiiDA node, which lead to the issue that when the threading is closed the session also finalized thereafter the node is expired and invalid to be used.
For more details of discussion about theread-local problem: check aiidateam/aiida-core#5765

Here, we break the API by changing all the traitlets that use AiiDA node to the static UUID, only loading the node and consuming it in the local scope. 
The PR involves all the widgets that explicitly using threading for the AiiDA node. There are possibilities that the other thread trigger the traitlets change and having AiiDA node in the extra threading, which also cause the exactly same issue. One of the example is the `ComputationalResourceWidget`, which has AiiDA code node as traitlets but has no threading. The issue happened when it is called from QeApp by additional threading from code setup widget.
In the future, we will change all the traitlets to UUID.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants