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

Fix for Service Serialization #796

Merged
merged 12 commits into from
May 18, 2022

Conversation

ItamarGoldman
Copy link
Contributor

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.

@ItamarGoldman ItamarGoldman self-assigned this May 4, 2022
@ItamarGoldman ItamarGoldman requested a review from gadial May 4, 2022 12:52
qiskit_experiments/database_service/db_experiment_data.py Outdated Show resolved Hide resolved
# 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(
Copy link
Collaborator

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

Copy link
Contributor Author

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?

qiskit_experiments/database_service/db_experiment_data.py Outdated Show resolved Hide resolved
@chriseclectic
Copy link
Collaborator

Forgot to add in the review that you should also add some tests

@chriseclectic chriseclectic added backport stable potential The issue or PR might be minimal and/or import enough to backport to stable Changelog: Bugfix Include in the "Fixed" section of the changelog labels May 4, 2022
Copy link
Contributor

@gadial gadial left a 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.

Copy link
Collaborator

@chriseclectic chriseclectic left a 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

qiskit_experiments/database_service/db_experiment_data.py Outdated Show resolved Hide resolved
qiskit_experiments/database_service/db_experiment_data.py Outdated Show resolved Hide resolved
@chriseclectic chriseclectic merged commit 82561ac into Qiskit-Extensions:main May 18, 2022
chriseclectic added a commit to chriseclectic/qiskit-experiments that referenced this pull request May 18, 2022
* 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 <cjwood@us.ibm.com>
chriseclectic added a commit to chriseclectic/qiskit-experiments that referenced this pull request May 18, 2022
* 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 <cjwood@us.ibm.com>
@ItamarGoldman ItamarGoldman deleted the Issue_714 branch July 7, 2022 10:58
paco-ri pushed a commit to paco-ri/qiskit-experiments that referenced this pull request Jul 11, 2022
* 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 <cjwood@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport stable potential The issue or PR might be minimal and/or import enough to backport to stable Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants