-
Notifications
You must be signed in to change notification settings - Fork 91
Avoid implicit commits #90
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,9 @@ | ||
| from .relational_operand import RelationalOperand | ||
| from . import DataJointError | ||
| from . import DataJointError, TransactionError | ||
| import abc | ||
| import logging | ||
|
|
||
| #noinspection PyExceptionInherit,PyCallingNonCallable | ||
| # noinspection PyExceptionInherit,PyCallingNonCallable | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
@@ -39,31 +39,41 @@ def target(self): | |
|
|
||
| def populate(self, restriction=None, suppress_errors=False, reserve_jobs=False): | ||
| """ | ||
| rel.populate() calls rel._make_tuples(key) for every primary key in self.pop_rel | ||
| rel.populate() calls rel._make_tuples(key) for every primary key in self.populate_relation | ||
| for which there is not already a tuple in rel. | ||
|
|
||
| :param restriction: restriction on rel.populate_relation - target | ||
| :param suppress_errors: suppresses error if true | ||
| :param reserve_jobs: currently not implemented | ||
| """ | ||
| assert not reserve_jobs, NotImplemented # issue #5 | ||
| assert not reserve_jobs, NotImplemented # issue #5 | ||
|
|
||
| error_list = [] if suppress_errors else None | ||
|
|
||
| if not isinstance(self.populate_relation, RelationalOperand): | ||
| raise DataJointError('Invalid populate_relation value') | ||
| self.conn._cancel_transaction() # rollback previous transaction, if any | ||
|
|
||
| unpopulated = (self.populate_relation - self.target) & restriction | ||
|
|
||
| for key in unpopulated.project(): | ||
| self.conn._start_transaction() | ||
| if key in self.target: # already populated | ||
| self.conn._cancel_transaction() | ||
| else: | ||
| logger.info('Populating: ' + str(key)) | ||
| try: | ||
| self._make_tuples(key) | ||
| except Exception as error: | ||
| self.conn._cancel_transaction() | ||
| if not suppress_errors: | ||
| raise | ||
| else: | ||
| print(error) | ||
| error_list.append((key, error)) | ||
| try: | ||
| while True: | ||
| try: | ||
| with self.conn.transaction(): | ||
| if not key in self.target: # already populated | ||
| logger.info('Populating: ' + str(key)) | ||
| self._make_tuples(dict(key)) | ||
| break | ||
| except TransactionError as tr_err: | ||
| if suppress_errors: | ||
| error_list.append((key,tr_err)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error probably does not need to be included in the error list
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the error list is not empty, it means something failed to populate.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's an implicit assumption I did not know about. However, I think
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No matter what solution we agree on, I think it should have two features:
|
||
| tr_err.resolve() | ||
| logger.info('Resolved transaction error raised by {0:s}.'.format(tr_err.culprit)) | ||
| except Exception as error: | ||
| if not suppress_errors: | ||
| raise | ||
| else: | ||
| self.conn._commit_transaction() | ||
| print(error) | ||
| error_list.append((key, error)) | ||
| logger.info('Done populating.') | ||
| return error_list | ||
| return error_list | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| from decorator import decorator | ||
| from . import DataJointError, TransactionError | ||
|
|
||
|
|
||
| def _not_in_transaction(f, *args, **kwargs): | ||
| if not hasattr(args[0], '_conn'): | ||
| raise DataJointError(u"{0:s} does not have a member called _conn".format(args[0].__class__.__name__, )) | ||
| if not hasattr(args[0]._conn, 'in_transaction'): | ||
| raise DataJointError( | ||
| u"{0:s}._conn does not have a property in_transaction".format(args[0].__class__.__name__, )) | ||
| if args[0]._conn.in_transaction: | ||
| raise TransactionError( | ||
| u"{0:s} is currently in transaction. Operation not allowed to avoid implicit commits.".format( | ||
| args[0].__class__.__name__), f, args, kwargs) | ||
| return f(*args, **kwargs) | ||
|
|
||
|
|
||
| def not_in_transaction(f): | ||
| """ | ||
| This decorator raises an error if the function is called during a transaction. | ||
| """ | ||
| return decorator(_not_in_transaction, f) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,3 +4,4 @@ networkx | |
| matplotlib | ||
| sphinx_rtd_theme | ||
| mock | ||
| decorator | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor point, but the comment should read
# key is not populated yetnow to reflect the change in codeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll change it directly in the master branch.