IBMQExperiment and FreeformDesign/CombinedExperimentDesign quality-of-life updates #379
Conversation
The goal is to add checkpointing, which will be facilitated by making this properly serializable.
Avoid pickling and don't write all_circuits_needing_data. Should cut on-disk space in half, and circuits can be reinitialized from keys (should also save on circ construction time)
Checkpointing is facilitated by moving this from a dict to a class that inherits from TreeNode and serializes more "pyGSTi-like". All major IMBQExperiment stages (transpile, submit, retrieve results) are accompanied by internal writes of the ibmqexperiment directory, which can be loaded from as checkpoints.
…rimentDesign The thought process is that in many cases, the all_circuits_needing_data for CombinedExperimentDesign is simply a union of subdesign lists. In this case, a user can opt out of saving this and just regenerate it on serialization (thereby saving 2x disk space and save/load time).
sserita
changed the title
|
@sserita, I got a ping about reviewing this due to being a code owner. From what I can see this doesn't touch code that I have prior experience with. Maybe a change in my team in the CODEOWNERS file is in order? |
pygsti/extras/ibmq/ibmqcore.py
Outdated
| self.auxfile_types['batch_results'] = 'pickle' | ||
|
|
||
| if not disable_checkpointing: | ||
| if self.checkpoint_path is None: |
There was a problem hiding this comment.
Since we're enabling checkpointing by default I think it would be worthwhile implementing a default path here for when the checkpoint_path kwarg isn't set by a user. This something we currently do in the GST protocol checkpointing code, fwiw, in case referencing how it is handled in that part of the code would be useful.
There was a problem hiding this comment.
I guess that is fair. One reason I did it this way is that the "checkpoint" here is actually a completely valid IBMQExperiment object that can be loaded using from_dir(), so in my experience so far it has been nice to have it match the dirname for write() so that the user doesn't have to change how they load from checkpoint vs a .write()... but it's a good point that a default path could simplify this for users. I'll think about how I want to incorporate that here.
pygsti/protocols/protocol.py
Outdated
| @@ -1268,7 +1313,7 @@ def from_edesign(cls, edesign, name): | |||
| raise ValueError("Cannot convert a %s to a %s!" % (str(type(edesign)), str(cls))) | |||
|
|
|||
| def __init__(self, sub_designs, all_circuits=None, qubit_labels=None, sub_design_dirs=None, | |||
| interleave=False): | |||
| interleave=False, skip_writing_all_circuits=False): | |||
| """ | |||
There was a problem hiding this comment.
I think the general idea of having the option to skip writing all_circuits is alright, but having this be something that is specified as an attribute of a class instance during the constructor stage feels suboptimal. This feels like something that would fit better as an optional flag passed into the write method. Of course, doing so would mean you'd need to implement an overriding version of write specific to CombinedExperimentDesign. Then again it looks like this is the tactic that was taken for the FreeformDesign, so maybe that isn't so bad?
There was a problem hiding this comment.
I actually tried that first, but it got a little nasty because a) it was doing a lot of manipulation of auxfile_types on the fly, and b) I really want to trigger that option as part of ProtocolData, IBMQExperiment, etc and that would mean plumbing up an option that was only used for CombinedExperimentDesign through everything that could possibly hold an edesign... Any suggestions on avoiding that plumbing are welcome, the class member was my first pass at it but I agree it's pretty awkward.
There was a problem hiding this comment.
Actually I'm not sure why I didn't think of this, but I can just check whether all_circuits_needing_data matches the set of all subdesigns... Duh. (Idea came from your assert comment below.)
|
Great work, @sserita! I posted a couple of code specific comments above, but had a couple of broader comments to go along with them. I think the main thing that this PR is missing is some unit tests. As far as I can tell we currently have no coverage of the IBMQExperiment in any of our test modules. More and more users are reliant on this code so this would be a good opportunity to at least start to fill that gap by adding some tests for the new checkpointing functionality. Thoughts on this? We obviously don't want to have the runners making a bunch of API calls to IBM's servers as part of testing, but I took a look at the code and at first glance it looks like the Relatedly, while we have some coverage on serialization for experiment designs, it looks like presently we don't have have anything for FreeformDesign (we actually only have one test that touched FreeformDesign period, and that is related to a qubit label mapping method), so this could be a good thing to add. Ditto with the new CombinedExperimentDesign serialization option (for skipping |
pygsti/protocols/protocol.py
Outdated
|
|
||
| # Don't save all_circuits_needing_data, it's redundant with aux_info keys | ||
| self.auxfile_types['all_circuits_needing_data'] = 'reset' | ||
| # Currently not jsonable, but will be fixed in write() |
There was a problem hiding this comment.
Would it be possible to add an assertion or other check here that check this is true? That is, that self.aux_info.keys() is equivalent to all_circuits_needing_data? This is definitely true initially upon construction, but it also looks like it is possible for these to become out of sync during the course of standard usage. For example, the base ExperimentDesign class has the method truncate_to_circuits which is a wrapper around _truncate_to_circuits_inplace. Looking at the implementation of _truncate_to_circuits_inplace this operates directly on all_circuits_needing_data and we never re-sync this with aux_info. There are a couple other methods where this happens as well.
There was a problem hiding this comment.
Good point. I think probably other things would break if they got out of sync too, but an assert here to check is very easy.
|
Thanks for the review @coreyostrove! I will go back and make these changes, and add a bunch of unit tests. However, testing IBMQExperiment is a bit of a problem - even the simulator hits the API. And I can make a test for transpile(), but that's really a relatively minor part of the code that could break in this class... Edit: Just found this: https://docs.quantum.ibm.com/api/qiskit/providers_fake_provider. IBMQ tests incoming 😄 |
This adds checks for Combined and FreeformDesigns to ensure we only skip all_circuits_needing_data when it can be regenerated. This also adds unit tests that (roughly) tests serialization/deserialization for all edesigns. That test would be improved by edesign equality code... but this is a first step.
This generalizes the previous changes to CombinedExperimentDesign and FreeformDesign. For classes that can create all_circuits_needing_data, it will be automatically not saved and regenerated in cases where it has not changed. Allows removal of from_dir and write from Combined and FreeformDesigns.
In addition to testing to/from_dataframe, this fixes (or at least clarifies) bugs when aux_info values are not dicts and when parsing circuit-str-json members with strings that are not circuits.
Also includes some QOL changes for debugging tests with VSCode and pytest.
Still needs some debugging, already fixed a few minor issues.
|
FYI, this is being delayed until 0.9.13 so that I can also incorporate the new IBMQ session workflow, which will eventually become the default way to run on IBMQ. |
This PR makes several quality-of-life improvements for
IBMQExperiment, including checkpointing for IBMQ experiments mentioned in #327, as well asFreeformDesignandCombinedExperimentDesignserialization.IBMQExperimentupdates:IBMQExperimentclass is changed from a dict to a full class that inherits fromTreeNode, making it commensurate with serialization of objects such asExperimentDesignandProtocolData.IBMQExperimentobjects. The API follows GST checkpointing wherecheckpoint_pathanddisable_checkpointingcan be passed to the constructor (checkpointing is on by default). Ifcheckpoint_pathis provided, thentranspile(),submit(), orretrieve_resultswill update the on-diskibmqexperimentwhen possible.bsonis installed, we can use thejson_utilhook to handle objects such asdatetime.datetimewhich are present insubmit_time_calibration_dataandbatch_resultsfrom the IBMQ server. Depending on the availability ofbson, we can serialize to all text/json files, but fall back to pickle still.FreeformDesignupdates:aux_infomember ofFreeformDesignis written as JSON instead of pickle. To do this, first we cast the Circuit keys as strings and then write the now-JSONable dictionary to file. We convert back to Circuits on deserialization.all_circuits_needing_dataforFreeformDesign. This field should match the keys ofaux_info, and with the size of these designs, it saves quite a lot of space to not double save them (almost 1 GB for some of my SVB use cases!).CombinedExperimentDesignupdates:skip_writing_all_circuitsflag. The intention is that in cases whereall_circuits_needing_datais simply the union of subdesigns, this can be regenerated on the fly. This saves space on disk and cuts the write/read times for the largest thing being written (cutting my 1 GB down to ~500 MB for my SVB use cases!).I've done my best to make it backwards-compatible; for example, we can still load old pickle-style
IBMQExperimentdirectories into the new object. However, theIBMQExperimentworkflow has to change a little to enable checkpointing - a minor pain point that I hope is well worth it for most users.Remaining Tasks
After a round of feedback, here's the current list of additional tasks: