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

Fix 1372 sqla attributes not current when read minimal #1379

Conversation

muhrin
Copy link
Contributor

@muhrin muhrin commented Apr 5, 2018

Fixes #1372 , the bug we found regarding node attributes not updating when changed externally.

I've also added a reasonably extensive test, test_node_access_with_sessions, which checks this for the Nodes.

muhrin added 4 commits April 5, 2018 16:12
There was an issue where because a node was in a Session, if any of the attributes
of a Node were changed by an external process the getters would not see the change.

I've put in an session.expire from SQLA to handle these cases.  Also refactored
AbstractNode a little.
* Removing references to node.dbnode() as much as possible as this should
not be part of the public api
* Fixed SQLA test for loading node type that is closest to missing plugin type
* Added type attribute to concrete node classes
HOWEVER: This is only fixed on Nodes and not any other AiiDA type.  This will have
to be done later.
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fanfriggintastic

@szoupanos
Copy link
Contributor

@sphuber @muhrin @giovannipizzi
Thanks Martin!

This was merged too fast before I managed to interact.
It is not clear to me the transaction isolation level that we currently use and what is the default of SQLA
https://en.wikipedia.org/wiki/Isolation_(database_systems)
http://docs.sqlalchemy.org/en/latest/orm/session_transaction.html#setting-transaction-isolation-levels

It should be Read committed based on the behaviour and what is mentioned at the links above.
What is not clear to me is:

  • if what Martin did is what we want as a general practice in all our transactions - and not just for the daemon problem (seems yes, but I am not sure)
  • why SQLA cannot guarantee this behaviour by default? Is it a drawback of SQLA or something that we miss.

@muhrin muhrin deleted the fix_1372_sqla_attributes_not_current_when_read__minimal branch September 3, 2018 08:59
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