Skip to content
This repository was archived by the owner on Jul 28, 2023. It is now read-only.

Fix issue with parameters in metadata #1065

Merged
merged 22 commits into from
Dec 1, 2021
Merged

Fix issue with parameters in metadata #1065

merged 22 commits into from
Dec 1, 2021

Conversation

rafal-pracht
Copy link
Contributor

Summary

When we have a Parameter in circuit metadata then during the run of the job it crashes.
For example, the circuit below before the fix throw the error: "TypeError: keys must be str, int, float, bool or None, not Parameter"

qr = QuantumRegister(1)
cr = ClassicalRegister(1)
my_circ_str = 'test_metadata'
my_circ = QuantumCircuit(qr, cr, name=my_circ_str, metadata={Parameter('φ'): 0.2})
job = backend.run(my_circ, shots=1024)

This issue was reported by @nbronn.

Details and comments

The solution: in _submit_job method after converting the Qobj to the dictionary we check if in the dictionary are only the valid JSON key, if not the key is converted to a string.

@jyu00
Copy link
Collaborator

jyu00 commented Oct 28, 2021

@rafal-pracht Thank you for reporting this issue. We already use the IQXJsonEncoder to serialize the Qobj when sending it to the server, so that'd be a better place to do additional processing.

Note that while converting the Parameter to a string would allow the Qobj be serialized, the string will not be converted back to a Parameter when you retrieve it later.

@rafal-pracht
Copy link
Contributor Author

@jyu00 Thanks for your review and sorry for the delay. I've updated the IQXJsonEncoder, and now it encodes the Parameter properly.

Regarding losing the Parameter during encoding. I've discussed this issue with @nbronn and this will not be an issue (we use the Parameter in metadata only for description purposes).

But it will not be an issue for me to write a code that doesn't lose the Parameter during encoding if you want. If yes, could you give me an example or point to the python file where we save a custom object as JSON in qiskit

@rafal-pracht
Copy link
Contributor Author

rafal-pracht commented Nov 11, 2021

Hi @jyu00
I've spent some time fixing the unit test issue. And because I've no idea what happened, I've rollback all my changes. So now there are no changes in my branch, but the unit tests still fail with "Exception: Unable to locate valid credentials.". Do you have any hints for me?

https://github.com/rafal-pracht/qiskit-ibmq-provider/actions/runs/1449448748

@jyu00
Copy link
Collaborator

jyu00 commented Nov 11, 2021

@rafal-pracht The tests require some setup to run because they need IBM Quantum credentials. You'll need to define these environment variables in Github actions:

        * QE_TOKEN: default token to use.
        * QE_URL: default url to use (set this to https://auth.quantum-computing.ibm.com/api)
        * QE_HGP: default hub/group/project to use.
        * QE_DEVICE: default device to use.

There are also some known failures that we are still working on, so don't spend too much time if you see failed tests.

@rathishcholarajan we should add the env variables needed for testing to the contribution guide.

@rafal-pracht
Copy link
Contributor Author

Thanks @jyu00, I've pushed my changes once again. It would be great if you can look and give me feedback.

In meanwhile I'll look at the GitHub environment setup.

@jyu00
Copy link
Collaborator

jyu00 commented Nov 12, 2021

@rafal-pracht can you add some test cases?

@rafal-pracht
Copy link
Contributor Author

@jyu00 I've added some test cases.

@rafal-pracht
Copy link
Contributor Author

@jyu00 Thanks for your feedback, I've just done the changes.

@rafal-pracht rafal-pracht requested a review from jyu00 November 25, 2021 09:57
Copy link
Collaborator

@jyu00 jyu00 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jyu00 jyu00 merged commit aa3db14 into Qiskit:master Dec 1, 2021
@rathishcholarajan rathishcholarajan added Changelog: Bugfix Include in the Fixed section of the changelog backport potential The bug might be minimal and/or import enough to be port to stable labels Dec 8, 2021
rathishcholarajan pushed a commit to rathishcholarajan/qiskit-ibmq-provider that referenced this pull request Dec 8, 2021
* Fix issue with parameters in metadata

* Move encode method to json_encoder.

* Fix lint issue: Using type() instead of isinstance() for a typecheck

* Fix lint issue: E501 line too long (115 > 100 characters)

* Fix lint issue.

* Rewrie the encode method

* Change the parameter name.

* Convert to string only for Parameter.

* Test if test pass

* Fix, metadata issue

* Add test for Parameter in metadata

* Fix lint issue in test

* Fix lint issue in test

* Fix lint issue in test

* Move IQXJsonEncoder tests to test_serialization file

* Fix lint issue in test

* Fix lint issue in test

Co-authored-by: Jessie Yu <jessieyu@us.ibm.com>
rathishcholarajan added a commit that referenced this pull request Dec 8, 2021
* Fix issue with parameters in metadata (#1065)

* Fix issue with parameters in metadata

* Move encode method to json_encoder.

* Fix lint issue: Using type() instead of isinstance() for a typecheck

* Fix lint issue: E501 line too long (115 > 100 characters)

* Fix lint issue.

* Rewrie the encode method

* Change the parameter name.

* Convert to string only for Parameter.

* Test if test pass

* Fix, metadata issue

* Add test for Parameter in metadata

* Fix lint issue in test

* Fix lint issue in test

* Fix lint issue in test

* Move IQXJsonEncoder tests to test_serialization file

* Fix lint issue in test

* Fix lint issue in test

Co-authored-by: Jessie Yu <jessieyu@us.ibm.com>

* Add release note

Co-authored-by: Rafał Pracht <55279376+rafal-pracht@users.noreply.github.com>
Co-authored-by: Jessie Yu <jessieyu@us.ibm.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the Fixed section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants