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 for issue #883 #889

Merged
merged 6 commits into from
Nov 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions aiida/backends/sqlalchemy/models/workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,19 +246,22 @@ def get_or_create(self, **kwargs): # this is to emulate the django method

def set_value(self, arg):
from aiida.orm import Node
from aiida.backends.sqlalchemy import get_scoped_session
try:
if isinstance(arg, Node) or issubclass(arg.__class__, Node):
if arg.pk is None:
raise ValueError("Cannot add an unstored node as an attribute of a Workflow!")
self.aiida_obj = arg.dbnode
sess = get_scoped_session()
self.aiida_obj = sess.merge(arg.dbnode, load=True)
self.value_type = wf_data_value_types.AIIDA
self.save()
else:
self.json_value = json.dumps(arg)
self.value_type = wf_data_value_types.JSON
self.save()
except:
raise ValueError("Cannot set the parameter {}".format(self.name))
except Exception as ex:
raise ValueError("Cannot set the parameter {}\n".format(self.name)
+ ex.message)

def get_value(self):
if self.value_type == wf_data_value_types.JSON:
Expand Down
47 changes: 46 additions & 1 deletion aiida/backends/sqlalchemy/tests/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
from aiida.common.utils import get_configured_user_email



class TestSessionSqla(AiidaTestCase):
"""
The following tests check that the session works as expected in some
Expand Down Expand Up @@ -181,3 +180,49 @@ def test_session_update_and_expiration_4(self):
code.store()

self.drop_connection()

Copy link
Member

Choose a reason for hiding this comment

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

I would also add a test without any SQL specific command (in this case expunge_all etc), as this is what the user would do (this could be a generic test) that would have triggered the error before.

Copy link
Contributor Author

@szoupanos szoupanos Nov 3, 2017

Choose a reason for hiding this comment

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

This is a good point.

The problem is that I have not managed to find until today (and it has been a while that I investigate this) a way to isolate the problem at that level (which more or less implies to follow the DbNode object generation in @nmounet workflows - that run by a Daemon that I don't control by my debugger).

Maybe an idea is if I can easily advance the steps of the daemon manually to emulate the daemon steps and see if I have the same behaviour. And if it is true to check what happens at every step with the session and the dbnodes. But this will not be a 10-20 mins job.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. For now, then, at least add the same identical test (but without any sqla command) as a general test. Then for me we can merge. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will.

Just keep in mind that it will be a test that doesn't test anything. I.e. that test would not fail even with the problematic code of set_value() that was corrected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created the test but I did not push it yet. Let's have a short chat before I add it to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I see. Let's merge this, then.

def test_session_wfdata(self):
"""
This test checks that the
aiida.backends.sqlalchemy.models.workflow.DbWorkflowData#set_value
method works as expected. There were problems with the DbNode object
that was added as a value to a DbWorkflowData object. If there was
an older version of the dbnode in the session than the one given to
to the DbWorkflowData#set_value then there was a collision in the
session and SQLA identity map.
"""
from aiida.orm.node import Node
from aiida.workflows.test import WFTestSimpleWithSubWF
from aiida.backends.sqlalchemy import get_scoped_session
from aiida.orm.utils import load_node

# Create a node and store it
n = Node()
n.store()

# Keep some useful information
n_id = n.dbnode.id
old_dbnode = n.dbnode

# Get the session
sess = get_scoped_session()
# Remove everything from the session
sess.expunge_all()

# Create a workflow and store it
wf = WFTestSimpleWithSubWF()
wf.store()

# Load a new version of the node
n_reloaded = load_node(n_id)

# Remove everything from the session
sess.expunge_all()

# Add the dbnode that was firstly added to the session
sess.add(old_dbnode)

# Add as attribute the node that was added after the first cleanup
# of the session
# At this point the following command should not fail
wf.add_attribute('a', n_reloaded)
3 changes: 2 additions & 1 deletion aiida/orm/implementation/sqlalchemy/workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
from aiida.backends.sqlalchemy.models.workflow import DbWorkflow, DbWorkflowStep
from aiida.backends.utils import get_automatic_user
from aiida.common import aiidalogger
from aiida.common.datastructures import wf_states, wf_exit_call
from aiida.common.datastructures import (wf_states, wf_exit_call,
wf_default_call)
from aiida.common.exceptions import (InternalError, ModificationNotAllowed,
NotExistent, ValidationError,
AiidaException)
Expand Down