Change QuantumCircuit.metadata to always be a dict

[ihincks]

Summary

The QuantumCircuit.metadata return annotation was previously just dict, whereas None is a valid value. This means that static type checkers would have no problems with statements like circuit.metadata.get("name", 0).

Edit:

The scope of this PR has changed. Instead of changing the annotation type to match the implementation, it now changes the implementation to match the annotation. In particular, the metadata attribute is always a dictionary now, and trying to set it to None emits a warning.

[qiskit-bot]

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:

• @Qiskit/terra-core

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[coveralls]

Pull Request Test Coverage Report for Build 4693325326

• 10 of 10 (100.0%) changed or added relevant lines in 4 files are covered.
• 21 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.02%) to 85.752%

---

Files with Coverage Reduction	New Missed Lines	%
qiskit/assembler/assemble_circuits.py	1	98.89%
crates/qasm2/src/lex.rs	2	91.14%
crates/qasm2/src/parse.rs	18	96.65%

Totals
Change from base Build 4692324070:	-0.02%
Covered Lines:	70278
Relevant Lines:	81955

---

💛 - Coveralls

[jakelishman]

tbh I think it might be more appropriate to fix QuantumCircuit to initialise metadata to {} if it's not given. It's a bit of a nuisance that we don't already do that.

[ihincks]

That would be my preference too, I just didn't want to stir anything up. Is there any concern at all about the extra few bytes/circuit this would incur? If not I can make the change.

[jakelishman]

Thanks for this - I'll be very happy to see the back of this typing weirdness, and from internal discussions, so will many other people.

Could you also add a release note with a "deprecations" note about the setter and an "upgrade" note about the narrowing of the getter return type?

(The command is reno new --edit quantumcircuit-metadata or similar)

[jakelishman]

The jillions of test failures are suggesting that we need to make the same change to DAGCircuit.metadata and DAGDependency.metadata, I'm fairly sure. Anywhere doing an explicit .metadata = None (there's a couple, e.g. in dagdependency.py and dagcircuit.py) will want updating to be .metadata = {}.

I just searched for .\metadata = in the repo to turn up the bad cases.

[ihincks]

Sure I can change the other two classes, too. I made the latest commits before seeing your comment.

[ihincks]

This aer line is setting a bunch of metadata to None: https://github.com/Qiskit/qiskit-aer/blob/f0d02e4e5a49c48e4557de01895f627df1c4c9b7/qiskit_aer/backends/aerbackend.py#L435

[ihincks]

Is there anything I need to be doing with this PR right now to move it along?

[jakelishman]

Oh, sorry Ian. The last run failed some tests (though the logs have since been cleaned) and there's currently a merge conflict. Do you happen to remember what the failing tests were about? If it's Aer, then we'll possibly need to put in a temporary warning catch for cases where that warning is being triggered specifically by Aer. We're not able to deploy any bugfixes to it at the moment because we've hit our storage limit on PyPI (though we're in the process of trying to get the cap raised for us).

[ihincks]

All of the ones that I have seen failing are of the flavour

testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/tmp/terra-tests/test/python/algorithms/optimizers/test_optimizer_aqgd.py", line 78, in test_list
    result = vqe.compute_minimum_eigenvalue(operator=self.qubit_op)
  File "/home/vsts/work/1/s/qiskit/algorithms/minimum_eigen_solvers/vqe.py", line 548, in compute_minimum_eigenvalue
    fun=energy_evaluation, x0=initial_point, jac=gradient, bounds=bounds
  File "/home/vsts/work/1/s/qiskit/algorithms/optimizers/aqgd.py", line 330, in minimize
    objval, gradient = self._compute_objective_fn_and_gradient(params, fun)
  File "/home/vsts/work/1/s/qiskit/algorithms/optimizers/aqgd.py", line 156, in _compute_objective_fn_and_gradient
    values = np.array(obj(param_sets_to_eval.reshape(-1)))
  File "/home/vsts/work/1/s/qiskit/algorithms/minimum_eigen_solvers/vqe.py", line 621, in energy_evaluation
    sampled_expect_op = self._circuit_sampler.convert(expect_op, params=param_bindings)
  File "/home/vsts/work/1/s/qiskit/opflow/converters/circuit_sampler.py", line 213, in convert
    sampled_statefn_dicts = self.sample_circuits(circuit_sfns=circs, param_bindings=p_b)
  File "/home/vsts/work/1/s/qiskit/opflow/converters/circuit_sampler.py", line 333, in sample_circuits
    ready_circs, had_transpiled=self._transpile_before_bind
  File "/home/vsts/work/1/s/qiskit/utils/quantum_instance.py", line 725, in execute
    max_job_retries=self._max_job_retries,
  File "/home/vsts/work/1/s/qiskit/utils/run_circuits.py", line 282, in run_circuits
    results.append(job.result())
  File "/home/vsts/work/1/s/test-job/lib/python3.7/site-packages/qiskit_aer/jobs/utils.py", line 41, in _wrapper
    return func(self, *args, **kwargs)
  File "/home/vsts/work/1/s/test-job/lib/python3.7/site-packages/qiskit_aer/jobs/aerjob.py", line 106, in result
    return self._future.result(timeout=timeout)
  File "/opt/hostedtoolcache/Python/3.7.16/x64/lib/python3.7/concurrent/futures/_base.py", line 428, in result
    return self.__get_result()
  File "/opt/hostedtoolcache/Python/3.7.16/x64/lib/python3.7/concurrent/futures/_base.py", line 384, in __get_result
    raise self._exception
  File "/opt/hostedtoolcache/Python/3.7.16/x64/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/home/vsts/work/1/s/test-job/lib/python3.7/site-packages/qiskit_aer/backends/aerbackend.py", line 463, in _execute_circuits_job
    circ.metadata = metadata
  File "/home/vsts/work/1/s/qiskit/circuit/quantumcircuit.py", line 471, in metadata
    DeprecationWarning,
DeprecationWarning: Setting metadata to None was deprecated in Terra 0.24.0 and this ability will be removed in a future release. Instead, set metadata to an empty dictionary.

[mtreinish]

That error is from aer. This will be blocked until qiskit-aer 0.12.1 is released, the only option is to filter the warning (since it's in 479 place we'd probably want to do it globally for all aer calls). Normally I'd be opposed to doing this globally since doing that means we're essentially ignoring the downstream impact of the deprecation. But since you've already fixed it in aer it probably is ok in this case.

[jakelishman]

This is fine by me now, but since I pushed the last couple of commits to sort out the warning filters in the test and the stacklevel (after checking with Ian), I'll wait for a second approver to merge.

[mtreinish]

LGTM

[ihincks]

🎉 thanks for all of your help