-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 serialization of primitives #9076
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
@combine(backend=BACKENDS) | ||
def test_serialization(self, backend): | ||
"""Test of serialize and deserialize""" | ||
estimator = BackendEstimator(backend=backend) | ||
serialization = pickle.dumps(estimator) | ||
_ = pickle.loads(serialization) | ||
|
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'm hesitant to add a test for this and treat this as part of the API contract for BackendEstimator
and BackendSampler
. As I said in #9033 Backend
objects are never guaranteed to be serializable and therefore these backend wrapping primitive classes can't have this guarantee either. This test only works because of the particulars of how the fake backends in terra are implemented
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.
Thanks. I got it. I will remove them.
releasenotes/notes/fix-serialization-primitives-c1e44a37cfe7a32a.yaml
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 3487912961
💛 - Coveralls |
…2a.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
3e1e3c0
to
a21aed2
Compare
@adekusar-drl Can you try this PR with your code? |
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.
LGTM, just a small suggestion for clarity in the release
releasenotes/notes/fix-serialization-primitives-c1e44a37cfe7a32a.yaml
Outdated
Show resolved
Hide resolved
@t-imamichi Looks good, thanks! |
…2a.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
* fix serialization of primitives * Update releasenotes/notes/fix-serialization-primitives-c1e44a37cfe7a32a.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * rm tests * Update releasenotes/notes/fix-serialization-primitives-c1e44a37cfe7a32a.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> (cherry picked from commit a4a6be0)
* fix serialization of primitives * Update releasenotes/notes/fix-serialization-primitives-c1e44a37cfe7a32a.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * rm tests * Update releasenotes/notes/fix-serialization-primitives-c1e44a37cfe7a32a.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> (cherry picked from commit a4a6be0) Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
* fix serialization of primitives * Update releasenotes/notes/fix-serialization-primitives-c1e44a37cfe7a32a.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * rm tests * Update releasenotes/notes/fix-serialization-primitives-c1e44a37cfe7a32a.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
Fix #9033
Details and comments