Add QPY module #250
Add QPY module #250
Conversation
* Move qpy to own module. qpy_serialization.py is splint into several files for maintenability. This commit also adds several bytes Enum classes for type keys in the header, and provides several helper functions. Some namedtuple class names are updated because, for example, INSTRUCTION will be vague when we add schedule, i.e. it's basically different program and has own instruction that has different data format. Basically CIRCUIT_ prefix is added to them. * manually cherry-pick #7584 with some cleanup - change qiskit.qpy.objects -> qiskit.qpy.binary_io - TUPLE -> SEQUENCE (we may use this for list in future) - add QpyError - add _write_register in circuit io to remove boilerplate code * respond to review comments - expose several private methods for backward compatibility - use options for symengine - rename alphanumeric -> value - rename write, read methods and remove alias - improve container read * remove import warning * replace alphanumeric with value in comments and messages. * private functions import Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…(#7550) * Add support for custom metadata serializers to QPY load() and dump() This commit adds a new argument to load(), metadata_serializer, and dump(), metadata_deserializer, which is used to specify custom serialization functions to use for serializing the python dictionary for each circuit's metadata attribute. By default QPY expects the serialized metadata payload to be encoded as a utf8 json string. But since there are no user constraints on the metadata dictionary's contents a simple json.dump() of the circuit's metadata attribute may not be feasible. To address this issue these new arguments are added to enable users to specify a custom function to encode the dictionary as binary data however is required for their circuit's metadata and then read that custom format as required when deserializing. The QPY format specification does not change here and still explicitly lists a UTF8 encoded JSON string as the contents. A fixed format is needed in the format specification for potential interoperability for other tools generating QPY data (none exist currently to my knowledge but nothing precludes it which is why the format is publicly documented). These new arguments are specific to only the qiskit implementation and should only be used when generating qpy files and loading them via qiskit. * Pivot to use JSONEncoder and JSONDecoder classes for new argument This commit pivots to leveraging custom JSONEncoder and JSONDecoder classes for the optional serializer and deserializer arguments instead of custom callables that return bytes. In the previous commit we used callables that would be passed the dictionary and returned bytes. However, while this was flexible it was pretty easy for a user to create QPY out of spec. While this was documented it is probably easier to just constrain the argument to always producing valid json. Using custom encoder and decoder classes let users still handle custom types but also restrict things to always be JSON. This should fix the potential risk in the previous iteration but still fix the underlying issue. * Update release note * Remove pylint ignores from release note example Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
rathishcholarajan
force-pushed
the
147-copy-qpy-module
branch
from
5c1c66f
to
ae05cd3
Compare
rathishcholarajan
force-pushed
the
147-copy-qpy-module
branch
from
ae05cd3
to
045e6f3
Compare
There was a problem hiding this comment.
I'm personally not a fan of this duplication for two reasons
- This comes with an overhead to keep the different implementations in sync. As we've learned with Deprecate auth parameter and add new channel parameter #226, there might be unexpected requirements that force us to change code that we thought can be copied across repositories to save efforts.
- Core maintainers of
qiskit-ibm-runtimeare not familiar with the copied code that now falls into their responsibilities (without dedicated test cases this lack of knowledge introduces a risk to introduce bugs)
Not saying I won't approve but wonder if we have carefully weighed these drawbacks against other options. Wouldn't it make sense to introduce a new python package for core functionality that multiple projects in the qiskit organization depend on?
As far as I understand this should allow us to overcome the timing issue that leads to the problems outlined in #147.
|
That core package is qiskit-terra. The problem is it moves at a different speed than server side deployment of that core package causing the sync issues. |
|
As for your specific point 2, the solution for that is you don't touch the qpy code here. Just treat it as a vendored dependency and any changes from it have to go through terra first. The point of vendoring it here is to match the qpy version used by the runtime client to something that is known compatible with the server side (which is using a fixed version of terra to deserialize it). |
The short term problem is the provider is importing private functions from terra now and with terra 0.20 (due today) we won't be able to import a couple of those private functions. If we don't do this users will have serialization issues when using parameterized circuits with Sampler or Estimator. So we need to copy this qpy module from qiskit in order to fix this issue short term. That way we are no longer relying on private functions from terra which I think is an improvement. The serialization related test cases that we currently have are passing when I run locally. So I believe that is good for now. In the next iteration I will copy over qpy related test cases from terra. I think like Matthew said the key is to not deviate too much in this fork and make any changes only through terra. |
|
@mtreinish @daka1510
Please correct me if my understanding is wrong. |
rathishcholarajan
added
the
Changelog: New Feature
Pull Request Test Coverage Report for Build 2072831594Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Summary
Copy QPY module from qiskit-terra. (CI works fine with terra main, this should be merged only after qiskit-terra 0.20.0 is released.)
Details and comments
Fixes #147