-
Notifications
You must be signed in to change notification settings - Fork 126
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
Fix for Service Serialization #796
Fix for Service Serialization #796
Conversation
# the attribute self._service in charge of the connection and communication with the | ||
# experiment db. It doesn't have meaning in the json format so there is no need to serialize | ||
# it. | ||
LOG.warning( |
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 was imagining this warning raised in __json_decode__
instead of __json_encode__
. I think either is ok, but if it's going to be in encode the message should be changed to be something like related to serialization like:
"Experiment service %s cannot be JSON serialized."
I also think raising a warning every time might be more annoying than useful so we could either just remove this warning entirely or change it to LOG.info
or LOG.debug
instead of LOG.warning
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.
Sound good. I will change it accordingly.
Regarding the location of the warning, I thought that the message will appear in the encode so that the user would not find out (by surprise) that the service was not serialized.
Should we raise it in __json_decode__
or is it better to raise it in both places?
Forgot to add in the review that you should also add some tests |
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.
Look good, but add tests.
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.
Question about if the _safe_serialized_data
is actually needed (since it will potentially have a pretty significant performance overhead), and if it is needed to better understand why
* Added warning for saving service class. * Added tests for experiment data serialization and bug fix * Serialization of numpy.number in json file Co-authored-by: Christopher J. Wood <[email protected]>
* Added warning for saving service class. * Added tests for experiment data serialization and bug fix * Serialization of numpy.number in json file Co-authored-by: Christopher J. Wood <[email protected]>
* Added warning for saving service class. * Added tests for experiment data serialization and bug fix * Serialization of numpy.number in json file Co-authored-by: Christopher J. Wood <[email protected]>
Summary
Fixing issue #714 .
Service value in json set to
None
as it is not serializable and isn't needed to re-create experiment results.