Skip to content
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

Merged
merged 1 commit into from
Jan 18, 2019

Conversation

szoupanos
Copy link
Contributor

This PR adresses issue #1759

Copy link
Contributor

@sphuber sphuber left a 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/backends/tests/export_and_import.py Outdated Show resolved Hide resolved
aiida/backends/tests/export_and_import.py Outdated Show resolved Hide resolved
aiida/backends/tests/export_and_import.py Outdated Show resolved Hide resolved
aiida/backends/tests/export_and_import.py Outdated Show resolved Hide resolved
aiida/backends/tests/export_and_import.py Outdated Show resolved Hide resolved
@@ -57,7 +57,9 @@ def emit(self, record):
return

if not hasattr(record, 'objpk'):
Copy link
Contributor

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?

Copy link
Contributor Author

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)

Copy link
Contributor

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 theobjuuidandobjname` key in the record and if they exist, we know the entity is stored.

Copy link
Contributor Author

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!

aiida/orm/implementation/logs.py Outdated Show resolved Hide resolved
aiida/orm/importexport.py Outdated Show resolved Hide resolved
aiida/orm/importexport.py Outdated Show resolved Hide resolved
aiida/orm/logs.py Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 67.368% when pulling 5b04212b2ba595353eb0a8f9b0d53fa7f7ca8f41 on szoupanos:issue_1759_v3 into 487c112 on aiidateam:provenance_redesign.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 67.368% when pulling 5b04212b2ba595353eb0a8f9b0d53fa7f7ca8f41 on szoupanos:issue_1759_v3 into 487c112 on aiidateam:provenance_redesign.

@coveralls
Copy link

coveralls commented Jan 16, 2019

Coverage Status

Coverage increased (+0.02%) to 69.62% when pulling 4f34b24 on szoupanos:issue_1759_v3 into 423523c on aiidateam:provenance_redesign.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 67.368% when pulling 5b04212b2ba595353eb0a8f9b0d53fa7f7ca8f41 on szoupanos:issue_1759_v3 into 487c112 on aiidateam:provenance_redesign.

@szoupanos szoupanos force-pushed the issue_1759_v3 branch 2 times, most recently from 19f9b1d to 7644559 Compare January 17, 2019 18:14
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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants