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 pickle deserialization of ExperimentData #1326

Merged
merged 10 commits into from
Jan 9, 2024

Conversation

wshanks
Copy link
Collaborator

@wshanks wshanks commented Nov 18, 2023

Previously, ExperimentData objects could be serialized and
deserialized using Python's pickle module, but deserialized objects
were not completely restored and an exception would be raised when doing
some operations like running analysis on the restored object.

Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'll approve once lint error is resolved.

@wshanks wshanks added this to the Release 0.6 milestone Dec 20, 2023
@wshanks
Copy link
Collaborator Author

wshanks commented Dec 21, 2023

@nkanazawa1989 I got sidetracked from this for a while.... The reason I had marked this as a draft was that I was not sure about the changes that I made to the test code.

The actual code fix is straightforward. I wanted to add a test though to verify the fix and check for future regressions. The test I added runs analysis on an experiment data object, then serializes/deserializes the experiment data through pickle, and runs analysis on the new experiment data object. I wanted to compare the two experiment data objects. I found that a simple comparison of the experiment data object contents would not give equality because results store their creation times as a field. Additionally, every result gets a result ID which is derived from a uuid which is also time-based. These result IDs are generated internally, so really there is no way to generate two experiment data objects with results that are identical.

To allow for my test to work, I added a **kwargs to is_equivalent which allows an extra option like ignore_result_id to be passed down to the different checkers. I was uncertain about a few things:

  1. Does ignore_result_id need to be an option? Maybe for is_equivalent result IDs should just always be ignored? Is there a case where they should not be? I think you would need to be testing serialization/deserialization without additional processing for the IDs to stay the same. Maybe you could just use __eq__ in that case though if you really cared about the IDs matching.
  2. I pick out ignore_result_id in _check_service_analysis_results and _check_result_table. Maybe I should keep propagating it?
  3. In _check_result_table, I turn the data frame into a list for comparison. This assumes that the results are in the same order for each table. Should I try to sort them?

I decided to always ignore the result IDs (making my second question irrelevant) and do sorting on the table entries. I am open to other suggestions.

@wshanks wshanks marked this pull request as ready for review December 21, 2023 15:19
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wshanks new validation logic seems reasonable.

@nkanazawa1989 nkanazawa1989 added this pull request to the merge queue Jan 9, 2024
Merged via the queue into qiskit-community:main with commit a6c9e53 Jan 9, 2024
11 checks passed
nkanazawa1989 pushed a commit to nkanazawa1989/qiskit-experiments that referenced this pull request Jan 17, 2024
Previously, `ExperimentData` objects could be serialized and
deserialized using Python's `pickle` module, but deserialized objects
were not completely restored and an exception would be raised when doing
some operations like running analysis on the restored object.
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.

2 participants