Avoid implicit commits

[fabiansinz]

• created a decorator @not_in_transaction that throws a TransactionError if the decorated function is called during a transaction
• If an error is thrown, the Transaction object rolls back the current transaction and hands the error to the next level
• populate explicitly catches the TransactionError. The TransactionError object holds the function and the parameters that caused the error. It uses that to provide a function resolve which just calls the function again with the same arguments. So if populate catches a TransactionError, it can just call resolve (because it is not in a transaction anymore), resolve the error, and restart the transaction.

[dimitri-yatsenko]

This error probably does not need to be included in the error list

[fabiansinz]

I thought about that. But then again I think it is good to have all the error that occurred. Also, I am using this list at the moment to test, whether the handling of TransactionErrors is correct.

[dimitri-yatsenko]

The returned error list is for the user to debug and it should only include unresolved problems. If the error is resolved, it should not be returned.

[dimitri-yatsenko]

If the error list is not empty, it means something failed to populate.

[fabiansinz]

That's an implicit assumption I did not know about. However, I think

• The user might want to know about it, since she causes a transaction to rollback simply because she did not declare a table. That might cost computational time and is easy to avoid.
• If she uses it for debugging/error resolving, she can just ignore TransactionErrors. So I don't see the problem for debugging. It is, after all, still an error.

[eywalker]

I noticed that this pull-request has already been merged, but wasn't sure if we resolved this issue surrounding the error list. Are we going with the solution provided by @fabiansinz ? or should we discuss this separately as an issue?

[fabiansinz]

No matter what solution we agree on, I think it should have two features:

• There should be a way to check whether TransactionErrors have been raised (and dealt with). I think this is important information for the user (I guess no one likes to compute something again just because he forgot to declare a table) and I'd like to have that feature for testing.
• We should make it very clear that only unhandled errors are part of the error list. Otherwise I think the current behaviour is unexpected. That's why I decided to add the errors to the list. That said, if errors are not suppressed, then currently the user does not see the error at all. I don't like that part of the current solution. I actually favour always returning an error list with the possibility that it is empty. That error list should then also contain the TransactionErrors.

[coveralls]

Coverage remained the same at 74.96% when pulling 0d5c907 on fabiansinz:master into e39aebc on datajoint:master.

[eywalker]

minor point, but the comment should read # key is not populated yet now to reflect the change in code

[fabiansinz]

Good point. I'll change it directly in the master branch.

[dimitri-yatsenko]

Sorry I rushed to merge this. I think these new changes are overly complex and buggy. Too many things happening behind the scenes across multiple locations in the code.

I am all for using python's clever features as long as they make things clearer, more efficient and explicit.

I recommend removing the not_in_transaction decorator. Its function is to replace the simple and clear line:

if self.conn.in_transaction:
    raise TransactionError("this function cannot be called from within a transaction")

in two places in the code with something much more elaborate and distributed. It makes the code harder to read and understand. It also adds an extra dependency, complicates the call stack, and makes debugging more difficult. It takes more time to understand and use than the original constructs.

I am not sure about the Transaction class. It adds some 60 lines of code with no added functionality or clarity. It's currently buggy: it changes the internal state of the connection object. Connection's start_transaction starts a transaction but does not change the status.

[fabiansinz]

Please specify where it is buggy.

[dimitri-yatsenko]

In the current master 2f35539, Connection._start_transaction does not change the status to _in_transaction. Transaction.__enter__ does, so calling Connection._start_transaction directly will produce an invalid state. If _start_transaction is not mean to be called directly, then it shouldn't be there in the first place.

[dimitri-yatsenko]

I fixed these bugs in my pull request (issue #91)

[fabiansinz]

I agree that it is not the most elegant way and that setting the boolean belongs in the other methods, but it is not a bug. Arguing that _start_transaction is not meant to be called is basically arguing that we shouldn't have private/protected methods. Having a protected method that causes inconsistent behaviour when called from outside is not a bug.

I can't see that your request got closed.

[fabiansinz]

Concerning the complexity:

The decorator:

• I wrote that decorator without knowing how many functions actually need that check. My goal was to avoid to too much boilerplate code. If we are sure that these are the only two functions, let's replace it.
• If we replace it, I still think the TransactionError should include who caused it so populate can resolve the error by calling it after it caught the error.

[dimitri-yatsenko]

If you are calling Connection._start_transaction() then let it set its internal state. The bug is that if someone else calls _start_transaction() bypassing Transaction, it causes an invalid state, triggering errors.

[dimitri-yatsenko]

No matter how many times @not_in_transaction is used, I feel it's clearer to do the check inside the function.

Sorry, I didn't quite understand how this resolve thing works. Would you explain?

[fabiansinz]

The function that throws the TransactionError passes itself and its arguments to the error.

raise TransactionError(
      "Operation not allowed to avoid implicit commits."., f, args, kwargs)

This allows the function that caused the transaction to catch the error, rollback, and then resolve it by calling TransactionError.resolve which calls f again with its arguments.

[eywalker]

I think it would make more sense for us to take this discussion offline. In general, it looks like we need to sync up on how some of these implementations should work out based on expected use cases. Having classes like Transaction makes sense to support the context (with) statement, but we should also decide on whether users should be provided with direct method of opening and closing transactions. Use of a decorator might be an overkill if we don't expect to have any other function that needs to be outside of a transaction, but again this depends on what we would expect.

[dimitri-yatsenko]

Okay, I understand. You want to invoke the DDL method again outside the transaction before attempting the transaction again. It's clever but I don't feel good about control flow now spanning three files. I would feel better about throwing a simple unhandled exception to the user saying "Sorry, please declare all your tables first before populating them." I don't feel good about the running and failing make_tuples multiple times either. We should think of a way to ensure that all the computed tables and their subtables are already declared by the time make_tuples is called.

[dimitri-yatsenko]

Yes, the with statement I am almost okay with if we fix the bugs and get rid of the transaction methods in Connection. It seems bloated now, but we can clean it up.

[fabiansinz]

I agree with @eywalker. See my email to you two.

[eywalker]

I know we decided to take this offline, but I would like to put a general reminder as I see this fit in closing the discussion here. An implementation is not a bug in so far as that's the expected behavior - if a feature has been implemented under different understanding about what's expected, we should be careful not to call it a bug, but rather take it as a sign that, as a team, we have to more carefully discuss and agree upon what's the expected behavior. We've been getting quite a bit done with fun, so let's plan to keep it that way.