-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Make PrimitiveJob serializable #12963
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
Conversation
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 14136417982Details
💛 - Coveralls |
It looks good overall. Could you add a reno to describe that both |
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. Could you ask someone else to review it too just in case?
…-terra into update-primitve-job
Co-authored-by: Takashi Imamichi <[email protected]>
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. Could you review it, @ihincks?
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.
The removal of the unimplemented __setattr__
method makes sense to me, but I have a question about the _prepare_dump
method.
qiskit/primitives/primitive_job.py
Outdated
def _prepare_dump(self): | ||
"""This method allows PrimitiveJob to be serialized""" | ||
_ = self.result() | ||
_ = self.status() | ||
self._future = None | ||
|
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.
When is this function called during serialization? I can't see anywhere where the method is called in the PR 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.
It would be called by the user if they want to serialize PrimitiveJob
- example here
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 make this happen in one of the serialization methods that python has built-in? I believe this could be executed on one of the methods, avoiding that function call. You could include the lines here and override one of the reserved methods for serialization: __reduce__
, __getstate__
, __getnewargs__
, depending on your needs.
Note: This might require you to also implement a __setstate__
method.
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.
Thank you for your suggestion. I think the following should work (to remove non-pickable object _future
) by replacing _prepare_dump
with the following.
def __getstate__(self):
_ = self.result()
_ = self.status()
state = self.__dict__.copy()
state["_future"] = None
return state
def __setstate__(self, state):
self.__dict__.update(state)
self._future = None
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 made a patch to this PR kt474#1.
Implement __getstate__ and __setstate__
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.
This looks much better! Thank you for these additions @kt474, @t-imamichi 🚀
releasenotes/notes/serialize-primitive-job-aa97d0bf8221ea99.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Takashi Imamichi <[email protected]>
Co-authored-by: Takashi Imamichi <[email protected]>
Summary
In
qiskit-ibm-runtime
we want to add job methods to ourQiskitRuntimeLocalService
(Issue Qiskit/qiskit-ibm-runtime#1607). In order to savePrimitiveJob
objects locally we need a way to serialize them. It's currently not serializable due to the use ofThreadPoolExecutor
:qiskit/qiskit/primitives/primitive_job.py
Lines 47 to 48 in 35d0954
Credit to @t-imamichi
Details and comments
Also fixes #12787