-
Notifications
You must be signed in to change notification settings - Fork 192
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
Necessary changes to properly export and import log records. #2393
Necessary changes to properly export and import log records. #2393
Conversation
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.
Lots of dead commented code and a few places where I think objpk
is still referenced/used when it shouldn't
aiida/common/log.py
Outdated
@@ -57,7 +57,9 @@ def emit(self, record): | |||
return | |||
|
|||
if not hasattr(record, 'objpk'): |
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.
Does this logic still make sense? The record should no longer have a objpk
correct?
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.
You are right on this. The idea is to avoid storing log records of unstored nodes.
However this check is done in create_entry_from_record and for this reason we need the objpk in these methods (an unstored node doesn't have an objpk)
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.
Wouldn't it be better perhaps, to get rid of the objpk
completely and adapt the get_dblogger_extra
function to only return a dictionary (containing the name
and uuid
of the entity) if the entity is stored. The
emitmethod can then simply check for the presence of both the
objuuidand
objname` key in the record and if they exist, we know the entity is stored.
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.
I did this change too even if I believe that this approach is also sub-optimal. Thanks for all the comments!
Coverage decreased (-1.4%) to 67.368% when pulling 5b04212b2ba595353eb0a8f9b0d53fa7f7ca8f41 on szoupanos:issue_1759_v3 into 487c112 on aiidateam:provenance_redesign. |
1 similar comment
Coverage decreased (-1.4%) to 67.368% when pulling 5b04212b2ba595353eb0a8f9b0d53fa7f7ca8f41 on szoupanos:issue_1759_v3 into 487c112 on aiidateam:provenance_redesign. |
Coverage decreased (-1.4%) to 67.368% when pulling 5b04212b2ba595353eb0a8f9b0d53fa7f7ca8f41 on szoupanos:issue_1759_v3 into 487c112 on aiidateam:provenance_redesign. |
19f9b1d
to
7644559
Compare
The native logging of python is extended with a `DbLogHandler` that will, when it is configured for a specific AiiDA entity (a Node, Group or any other database entity), store a copy of the record in the `DbLog` table with a reference to the entity it belongs to. Since the entities do not necessarily live in the same table, a foreign key could not be used, so instead the primary key was stored as well. However, when exporting and importing entities and their log messages, the entities would get new primary keys, which would orphan the log messages. The attribute that is persistent across exporting and importing is the UUID. Therefore the reference attribute for log entries to their entities has been changed from the `pk` to the `uuid`.
7644559
to
4f34b24
Compare
This PR adresses issue #1759