Fixes for some issues with rose_ana KGO database #2047
Conversation
|
Okay passing the built-in tests - I'll run it on a proper case in our UM rose suite as well to make sure it solves the problems we were seeing before. If possible it would be great if this went on (to at least |
…hat one write happens at the beginning and one at the end!
|
Apologies for changing it after the review request - it's a trivial change as I spotted that the message in |
lib/python/rose/apps/rose_ana.py
Outdated
| # Prepend the task_name onto each entry, to try and ensure it is | ||
| # unique (the individual comparison names may not be, but the rose | ||
| # task name + the comparison task name should) | ||
| sql_args = [self.task_name + " - " + app_task, |
There was a problem hiding this comment.
You're adding self.task_name to app_task here, but they're set to the same thing on line 105 aren't they?
There was a problem hiding this comment.
I can see that it looks that way, but despite the argument names both being app_task, one is set by rose_ana itself to the Rose task name (the one which is saved to self.task_name) but the other one (passed to enter_comparison) is supplied by the user, and can be anything - though by convention it uses the section name from the app... I could perhaps rename the arguments to make this more obvious?
| @@ -101,7 +112,8 @@ def database_lock(self, lockfile, reporter=None): | |||
| if reporter is not None: | |||
| reporter("Acquired DB lock at: " + time.asctime()) | |||
| lock.write("{0}".format(os.getpid())) | |||
| reporter("Writing results to KGO Database...") | |||
| if reporter is not None: | |||
| reporter("Writing to KGO Database...") | |||
There was a problem hiding this comment.
Is this for debugging only?
There was a problem hiding this comment.
No I think this is as intended (I just realised it ought to be protected in the same way the other statements are)
Two slight problems have been identified with this functionality (though the impact is limited, only to those making use of the KGO database). Two different problems exist:
1. The
rose_anare-write moved to using a buffer for the statment (to avoid many repeated calls to locking and/or commits to the database. Unfortunately this defeats the point of thetasksdatabase table - the original idea was that this table would initially be set to1for each task, and updated to0when the task finished (allowing scripts interfacing with the database the means to understand when something unexpected had stopped the task completing)... Due to the use of the buffer everything is done in one go, meaning an unexpected error results in nothing being written instead. The solution is to have 2 calls perrose_anatask instead (one to set thetaskstable entry at the start)2. At some point during the writing of the analysis modules both externally and for the
grepper.pyclass, the_kgo_db_add_file_pairmethod was somehow forgotten/overlooked and tasks instead directly calledself.parent.kgo_db.add_comparison... this meant losing the behaviour of making the table key unique by appending therose_anatask name. Rather than fix the call ingrepper.pyand elsewhere it seems a little redundant having the_kgo_db_*methods at all in the app class, so these are now dropped