From 9a28e2e61b43ddf4ae0b053a22c3ae5348a74223 Mon Sep 17 00:00:00 2001 From: Jake Lishman Date: Thu, 2 Dec 2021 19:23:13 +0000 Subject: [PATCH 1/2] Hack in SimpleNamespace-like functionality to Options This is an awful hack to support various SimpleNamespace-like operations that qiskit-dynamics are using. We need to transition downstream packages to the new Options API, but in the meantime, this makes the Options class behave as much like a SimpleNamespace as it can, while still keeping the new instance attributes and methods. --- qiskit/providers/options.py | 74 +++++++++++++++++++++-- test/python/providers/test_options.py | 87 +++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 6 deletions(-) diff --git a/qiskit/providers/options.py b/qiskit/providers/options.py index 0f8f1f66e9e4..476d0a0d78da 100644 --- a/qiskit/providers/options.py +++ b/qiskit/providers/options.py @@ -26,12 +26,69 @@ class Options: options. """ - _fields = {} + # Here there are dragons. + + # This class preamble is an abhorrent hack to make `Options` work similarly to a + # SimpleNamespace, but with its instance methods and attributes in a separate namespace. This + # is required to make the initial release of Qiskit Terra 0.19 compatible with already released + # versions of Qiskit Experiments, which rely on both of + # options.my_key = my_value + # transpile(qc, **options.__dict__) + # working. + # + # Making `__dict__` a property which gets a slotted attribute solves the second line. The + # slotted attributes are not stored in a `__dict__` anyway, and `__slots__` classes suppress the + # creation of `__dict__`. That leaves it free for us to override it with a property, which + # returns the options namespace `_fields`. + # + # We need to make attribute setting simply set options as well, to support statements of the + # form `options.key = value`. We also need to ensure that existing uses do not override any new + # methods. We do this by overriding `__setattr__` to purely write into our `_fields` dict + # instead. This has the highly unusual behavior that + # >>> options = Options() + # >>> options.validator = "my validator option setting" + # >>> options.validator + # {} + # >>> options.get("validator") + # "my validator option setting" + # This is the most we can do to support the old interface; _getting_ attributes must return the + # new forms where appropriate, but setting will work with anything. All options can always be + # returned by `Options.get`. To initialise the attributes in `__init__`, we need to dodge the + # overriding of `__setattr__`, and upcall to `object.__setattr__`. + # + # To support copying and pickling, we also have to define how to set our state, because Python's + # normal way of trying to get attributes in the unpickle will fail. + # + # This is a terrible hack, and is purely to ensure that Terra 0.19 does not break versions of + # other Qiskit-family packages that are already deployed. It should be removed as soon as + # possible. + + __slots__ = ("_fields", "validator") + + @property + def __dict__(self): + return self._fields + + def __setattr__(self, key, value): + self._fields[key] = value + + def __getstate__(self): + return (self._fields, self.validator) + + def __setstate__(self, state): + _fields, validator = state + super().__setattr__("_fields", _fields) + super().__setattr__("validator", validator) def __init__(self, **kwargs): - self._fields = {} - self._fields.update(kwargs) - self.validator = {} + super().__setattr__("_fields", kwargs) + super().__setattr__("validator", {}) + + # The eldritch horrors are over, and normal service resumes below. Beware that while + # `__setattr__` is overridden, you cannot do `self.x = y` (but `self.x[key] = y` is fine). This + # should not be necessary, but if _absolutely_ required, you must do + # super().__setattr__("x", y) + # to avoid just setting a value in `_fields`. def __repr__(self): items = (f"{k}={v!r}" for k, v in self._fields.items()) @@ -121,14 +178,19 @@ def update_options(self, **fields): self._fields.update(fields) def __getattr__(self, name): + # This does not interrupt the normal lookup of things like methods or `_fields`, because + # those are successfully resolved by the normal Python lookup apparatus. If we are here, + # then lookup has failed, so we must be looking for an option. If the user has manually + # called `self.__getattr__("_fields")` then they'll get the option not the full dict, but + # that's not really our fault. `getattr(self, "_fields")` will still find the dict. try: return self._fields[name] except KeyError as ex: - raise AttributeError(f"Attribute {name} is not defined") from ex + raise AttributeError(f"Option {name} is not defined") from ex def get(self, field, default=None): """Get an option value for a given key.""" - return getattr(self, field, default) + return self._fields.get(field, default) def __str__(self): no_validator = super().__str__() diff --git a/test/python/providers/test_options.py b/test/python/providers/test_options.py index 64c61c7a7a8e..7ab36ca44e5a 100644 --- a/test/python/providers/test_options.py +++ b/test/python/providers/test_options.py @@ -13,6 +13,9 @@ # pylint: disable=missing-class-docstring,missing-function-docstring # pylint: disable=missing-module-docstring +import copy +import pickle + from qiskit.providers import Options from qiskit.qobj.utils import MeasLevel @@ -105,3 +108,87 @@ def test_hasattr(self): options = Options(shots=1024) self.assertTrue(hasattr(options, "shots")) self.assertFalse(hasattr(options, "method")) + + +class TestOptionsSimpleNamespaceBackwardCompatibility(QiskitTestCase): + """Tests that SimpleNamespace-like functionality that qiskit-experiments relies on for Options + still works.""" + + def test_unpacking_dict(self): + kwargs = {"hello": "world", "a": "b"} + options = Options(**kwargs) + self.assertEqual(options.__dict__, kwargs) + self.assertEqual({**options.__dict__}, kwargs) + + def test_setting_attributes(self): + options = Options() + options.hello = "world" + options.a = "b" + self.assertEqual(options.get("hello"), "world") + self.assertEqual(options.get("a"), "b") + self.assertEqual(options.__dict__, {"hello": "world", "a": "b"}) + + def test_overriding_instance_attributes(self): + """Test that setting instance attributes and methods does not interfere with previously + defined attributes and methods. This produces an inconsistency where + >>> options = Options() + >>> options.validators = "hello" + >>> options.validators + {} + >>> options.get("validators") + "hello" + """ + options = Options(get="a string") + options.validator = "another string" + setattr(options, "update_options", "not a method") + options.update_options(_fields="not a dict") + options.__dict__ = "also not a dict" + + self.assertEqual( + options.__dict__, + { + "get": "a string", + "validator": "another string", + "update_options": "not a method", + "_fields": "not a dict", + "__dict__": "also not a dict", + }, + ) + self.assertEqual( + options._fields, + { + "get": "a string", + "validator": "another string", + "update_options": "not a method", + "_fields": "not a dict", + "__dict__": "also not a dict", + }, + ) + self.assertEqual(options.validator, {}) + self.assertEqual(options.get("_fields"), "not a dict") + + def test_copy(self): + options = Options(shots=1024, method="auto", meas_level=MeasLevel.KERNELED) + options.set_validator("shots", (1, 1024)) + options.set_validator("method", ["auto", "statevector", "mps"]) + options.set_validator("meas_level", MeasLevel) + expected = """Options(shots=1024, method='auto', meas_level=) +Where: +\tshots is >= 1 and <= 1024 +\tmethod is one of ['auto', 'statevector', 'mps'] +\tmeas_level is of type \n""" + self.assertEqual(str(options), expected) + self.assertEqual(str(copy.copy(options)), expected) + + def test_pickle(self): + options = Options(shots=1024, method="auto", meas_level=MeasLevel.KERNELED) + options.set_validator("shots", (1, 1024)) + options.set_validator("method", ["auto", "statevector", "mps"]) + options.set_validator("meas_level", MeasLevel) + expected = """Options(shots=1024, method='auto', meas_level=) +Where: +\tshots is >= 1 and <= 1024 +\tmethod is one of ['auto', 'statevector', 'mps'] +\tmeas_level is of type \n""" + self.assertEqual(str(options), expected) + self.assertEqual(str(pickle.loads(pickle.dumps(options))), expected) From bdcdf646f260282b98347485774d24731f845080 Mon Sep 17 00:00:00 2001 From: Jake Lishman Date: Thu, 2 Dec 2021 21:10:54 +0000 Subject: [PATCH 2/2] Suppress pylint bad assignment error I was surprised at how little pylint complained about the actual implementation, but it complains about the usage. This could conceivably be called a pylint bug; because we override `__setattr__`, pylint should perhaps recognise that general attribute setting might be allowed. --- test/python/providers/test_options.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/python/providers/test_options.py b/test/python/providers/test_options.py index 7ab36ca44e5a..e50e90458be7 100644 --- a/test/python/providers/test_options.py +++ b/test/python/providers/test_options.py @@ -122,8 +122,8 @@ def test_unpacking_dict(self): def test_setting_attributes(self): options = Options() - options.hello = "world" - options.a = "b" + options.hello = "world" # pylint: disable=assigning-non-slot + options.a = "b" # pylint: disable=assigning-non-slot self.assertEqual(options.get("hello"), "world") self.assertEqual(options.get("a"), "b") self.assertEqual(options.__dict__, {"hello": "world", "a": "b"})