providers/options: implement Mapping

[airwoodix]

Summary

This is an option for resolving #9686 by implementing the collections.abc.MutableMapping interface for the Options class. This automatically provides implementations of the suggested items() and of the existing get() methods.

Details and comments

As a MutableMapping, Options instances behave like mutable dictionaries with the extra validator feature. The __setitem__ implementation goes through update_options to make sure that the validator is running. This is inconsistent with __setattr__ but I understand that the namespace interface is there for backwards compatibility and running the validator may break use cases (although this would arguably be a logic error).

A side effect of implementing MutableMapping is that there's yet another way of accessing the options (indexing) which is however natural for an object that exposes a items() method.

Resolves #9686

[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

[mtreinish]

Overall this LGTM, thanks for doing this! I have one inline comment about the docstring (it was preexisting but maybe we can clean it up while we're editing the text for clarity). The only other thing that's missing here is a release note. If you could add a release note for this new feature that would be great so we document this as part of 0.24.0. The process for doing this is documented here:

https://github.com/Qiskit/qiskit-terra/blob/main/CONTRIBUTING.md#release-notes

[coveralls]

Pull Request Test Coverage Report for Build 4461035815

• 15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
• 6 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.03%) to 85.384%

---

Files with Coverage Reduction	New Missed Lines	%
qiskit/extensions/quantum_initializer/squ.py	2	80.0%
crates/accelerate/src/sabre_swap/layer.rs	4	97.32%

Totals
Change from base Build 4450882470:	0.03%
Covered Lines:	68012
Relevant Lines:	79654

---

💛 - Coveralls

[airwoodix]

Thanks for the positive review! I added the release notes.

[mtreinish]

Hmm, I know this is part of the mutable mapping abstract interface so it needs to be implemented (which is why I skipped over this in my earlier review). But on reflection I'm not sure deletion is actually part of the workflow for this class. The typical workflow is the provider package creates the Options object at backend initialization and it contains the canonical set of fields for users to set along with their default values. Deletion here doesn't make a ton of sense because nothing should ever be deleting the fields after it's created as they contain the current default behavior for running on a backend.

I'm not 100% sure the best way to proceed with this. My first thought would be to just make this raise an exception. But, that's not great either because it would show methods like pop() and clear() as being available which they shouldn't be. I'm thinking the best path forward might be to change MutableMapping to just Mapping and keep the __setitem__ definition so we support assignment. What do you think?

[airwoodix]

Thanks for the review and apologies for the delayed feedback.

I feel like the actual usage of Options is rather that of a (non-frozen) dataclass than a dict, isn't it? Option fields are defined by the backend implementation but the values can be edited. This would however be a quite massive change I guess.

Implementing __setitem__ however allows to dynamically add options (potentially by mistake in case of a typo), and by symmetry, it would be surprising not to implement the full MutableMapping interface but something in-between the standard ABCs. To keep the learning curve smooth, I think that it's easier to say that Options is a MutableMapping with optional custom validators and no extra peculiarities.

I'd therefore rather be in favor of implementing the full MutableMapping interface, in order not to surprise users. Do you find this reasonable?

[jakelishman]

I don't think it's surprising if we say "it fulfills Mapping, but you can also change the values" - it can fulfil part of one interface without fulfilling all of the next one in the stack, just like numpy.array fulfils Iterable but not Sequence (because they don't want to implement the named methods that Sequence requires).

[jakelishman]

As in, there's nothing about implementing only Mapping that means you can't be mutable. It just means that you provide a particular ABC's definition of "view" semantics around your keys and values.

[airwoodix]

Fair enough. I updated the branch to have Options implement Mapping.

it fulfills Mapping, but you can also change the values

and add keys. This is why I find it strange to keep to asymmetry by not providing an interface to delete keys. But I see that it's not the wanted usage of the datastructure.

[mtreinish]

This LGTM now, thanks for doing this and the quick updates. Sorry for taking a while to circle back to this.