-
Notifications
You must be signed in to change notification settings - Fork 224
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
Remove unserializable data from metadata in Log
records
#2469
Remove unserializable data from metadata in Log
records
#2469
Conversation
aiida/orm/logs.py
Outdated
@@ -58,8 +58,10 @@ def create_entry_from_record(record): | |||
|
|||
metadata = dict(record.__dict__) | |||
|
|||
# Get rid of the exc info because this is usually not serializable | |||
metadata['exc_info'] = None | |||
# Get rid of the `exc_info` and `args` if they exist in the metadata because they are often not serializable |
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.
would it be better to to a str
or them, so we don;'t completely lose the content?
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.
except the problem is that I am not sure if this guarantees that that is possible. I think args
can literally contain any python object and so it might be possible that even calling str
on it might fail right? Given that this leads to a database error I'd rather err on the safe side given that this information is more or less duplicated because it will often already be formatted into the message of the record itself.
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 think that any python object can be converted with str (unless you have an exception in the str method by mistake).then maybe it's useless if it only gives the class name, or it's redundant. do you have an example where you could check the str value in the relevant case, eg in the test you wrote, as well as the other Metadata 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.
For this test it works, but I just was a bit afraid that there might be other cases where that approach wouldn't work and given the resulting excepting is pretty nasty, i.e. if this happens in a daemon worker, the whole daemon worker becomes defunct as its database session becomes corrupted. Therefore I was playing on the safe side. But I changed it to stringify the content. Let's see how that goes
1ec6dae
to
24b5350
Compare
If unserializable data, often found in the `exc_info` or `args` keys, is not removed from the metadata of a log record, a database exception will be thrown.
24b5350
to
bc39e5a
Compare
@giovannipizzi let's merge it like this and I will report back when I encounter new issues with this approach |
Fixes #2363
If unserializable data, often found in the
exc_info
orargs
keys, isnot removed from the metadata of a log record, a database exception will
be thrown.