[LOWPRI] Why is Orangeboard executing Cypher statements in a transaction?

[saramsey]

Yao,

Do we need the begin_transaction() call in this line in Orangeboard? (line 429)

with session.begin_transaction() as tx:

Note: please do not modify Orangeboard without extensive testing including pushing a large graph from Orangeboard to a local Neo4j. And we are currently using ncats.saramsey.org and lysine.ncats.io for integration testing. So this Issue can wait until after the 11/29 demo.

Steve

[dkoslicki]

From SO:
Session.run() will actually create a transaction, execute the statement, and commit the transaction. Transaction.run() will leave the transaction open until you commit it, but the statement will still be sent and interpreted and executed, and results will be returned. However, any changes won't actually be persisted into the datastore, and won't be visible to queries outside of the transaction. You have to mark the transaction as successful and commit it or it will be rolled back.

You should try not to use transactions; open transactions prevent changes to indexes and constraints and increase memory usage. The only reason to use transactions is for the rollback potential; if you want to see what the results of the query are, and maybe undo it depending on those results, then use a transaction. Otherwise use a session.

[saramsey]

Thank you @dkoslicki . Fixed in Orangeboard.py.

[saramsey]

@dkoslicki I've updated Q1Utils.py to remove the transaction statement there. Hope that's OK.

[erikyao]

Hi Steve & David,

It's NOT OK to remove the transaction statement. We discussed this issue before.

Session.run() will actually create a transaction, execute the statement, and commit the transaction.

It's true. This is the so called auto-commit mode. HOWEVER, it is not guaranteed that the transaction will be created and committed before your program ends.

with-statements in python is where the context managers play their roles:

with session.begin_transaction() as tx:
    tx.run("query")

is equivalent to

try: 
    context_manager = session.begin_transaction()
    tx = context_manager.__enter__()
    tx.run("query")
finally:
    context_manager.__exit__()

Usually context_manager.__enter__() would return self so tx is actually the context-manager object here. And the __exit__() method is actually a callback to commit tx. So with this with-statement, every transaction is guaranteed to be committed.

Best,
Yao

[erikyao]

In worst case, we would lose some unbegun transactions when a python script runs to its end.

[saramsey]

Yao,

I switched to auto-commit as we were occasionally seeing some severe slowdowns in pushing content to Neo4j. These slowdowns have gone away after switching to auto-commit.

Let’s leave the code as it currently is and discuss transactioning, after Wednesday. It is fine that you opened this issue again, as we do want to (eventually) do what is technically correct. But I am not entirely convinced that we should be using a transaction.

Steve

[erikyao]

It's OK if we are not pushing tons of nodes into neo4j right now. However Zheng and I both reproduced errors of nodes not being pushed completely.

[dkoslicki]

On a related note, we should try transaction vs session in Q1Solution and Q2Solution. For example, when you run Q2Solution.py -h, it throws an error due to the Neo4j code. I’ll get you the details in a bit, but this might be related.

[dkoslicki]

@erikyao Do you have details about the nodes that weren’t pushed or error messages returned?

[erikyao]

To avoid the slowdowns, we can merge all queries into ONE manually managed transaction. Obviously, not for now.

[saramsey]

@dkoslicki can you attach a stack backtrace? Better yet maybe a new issue since it is not (yet) clear that it is due to auto-commit?

[erikyao]

@dkoslicki actually there won't be any error when a transaction is lost. In our previous tests of pushing nodes twice, the last time push was always lost.

[saramsey]

@erikyao Did you see the SO posting linked by @dkoslicki ? It specifically says not to use a Transaction unless you need the rollback potential.

If you have reproducible code showing lost node pushes to Neo4j, please post a MWE here.

[saramsey]

Oh, and make sure you do your testing by pushing nodes to your local Neo4j; we don't want to be modifying the Neo4j on ncats.saramsey.org.

[erikyao]

It's a very subtle bug to find. If you have some other function calls after the last push, and it takes enough time for the auto-commit transaction to be done, everything is OK.

[dkoslicki]

See issue #120 for the issue with Q1Solution.py -h (same with Q2).

[erikyao]

@saramsey Yes, I've read that SO thread. I issued a complaint to the python driver developers about the auto-commit mode, like 3 weeks ago. One suggestion I got is:

It is true that session.run will not ensure that your transaction is finished when run returns, while if you consume all your session run result or close your session, then it will be guaranteed to have your transaction finished. And it is highly recommended to always consume results and close sessions.

[dkoslicki]

@erikyao So it should show the error if we call the cypher query and then immediately exit, right? Should be straightforward to come up with a MWE then... Correct me if I'm wrong

[dkoslicki]

Ah, I see @erikyao, in the code I wrote, I always consume the session run results before closing the session (eg

res = session.run(query)
res = [i for i in res]

So that's why I haven't seen the issue you mention come up.

[saramsey]

@erikyao you said

actually there won't be any error when a transaction is lost. In our previous tests of pushing nodes twice, the last time push was always lost.

You said always lost. Now you're saying it is a subtle bug to trip. I am confused.

Furthermore, your quote from the driver devs:

then it will be guaranteed to have your transaction finished. And it is highly recommended to always consume results and close sessions.

... states that the transaction is guaranteed to post if we close the session. So we just need to add a python statement closing the session.... right?

[erikyao]

@saramsey sorry I made you confused. By always, I meant always in my specfic tests.

I have some doubt towards closing the session because the comment says:

Close the session. This will release any borrowed resources, such as connections, and will roll back any outstanding transactions.

[erikyao]

@dkoslicki you are right. If you consumed the result from your query, it would be fine.

[dkoslicki]

@erikyao so can you post the code that always caused the issue? Otherwise, I assume we could trip the error with something like:
session.run(“match p=()-[]-() return p”) or some other long-running command and exit before it’s returned or consumed.

[saramsey]

@erikyao Once again, if you have code that consistently trips this bug, please post it so we can reproduce the problem using local Neo4j.

[erikyao]

@saramsey @dkoslicki let me look for my commits first. Please wait a second.

[erikyao]

@saramsey @dkoslicki I am writing a new test since I cannot find my previous one. I'll test by myself first. Should be quick.

[erikyao]

Frustrated, I cannot reproduce this error with the latest code. Previously we had only one session to manage all the transactions (no matter manually or not); now we create a new session every time we run a query. I think it would suffer the same problem especially when the transaction is way too big and lagging behind. However I cannot run big tests like before on my workstation because of memory issue. I'll look deeper into it tonight.

[saramsey]

@erikyao any luck in tracking down this MWE?