Merge lp:~allenap/maas/lazy-module-loading into lp:~maas-committers/maas/trunk

[Gavin Panella]

A lazy module loader to take away those circular import blues.

This has been sitting around since June and I finally got around to finishing it off when travelling home from Seattle. This contains the module importer and an example or two of its use. There's a follow-up branch that replaces most/all of the remaining circular imports with it.

[Mike Pontillo (mpontillo)]

I think this is a novel way to solve the problem, and I think it could be a step in the right direction.

However, I'm not sure we should land it this late in 1.9. It seems risky to introduce the extra logic now. It seems you are creating "proxy" objects, and I don't fully understand how all of the proxying logic works. For example, do the objects returned by the lazy importer obey "normal" python conventions, such as returning the appropriate type when isinstance() is called?

It seems that another way to solve the circular import problem would be to import modules rather than functions at the top level, so that the specific dependencies we need aren't resolved until the correct time. But that would lead to code like:

from maasserver.models import fabric

...

def foo():
    f = fabric.Fabric()
    # or...
    Fabric = fabric.Fabric
    f = Fabric()

... which seems less than ideal. (but perhaps no worse than the extra 'import' line we have now, or writing a few hundred lines of lazy-importing code!)

review: Needs Information

[Mike Pontillo (mpontillo)]

Just thought of another thing - this may throw off code introspection tools (such as IPython, PyCharm, etc), which gives me pause...

[Gavin Panella (allenap)]

> I think this is a novel way to solve the problem, and I think it could
> be a step in the right direction.
>
> However, I'm not sure we should land it this late in 1.9. It seems
> risky to introduce the extra logic now. It seems you are creating
> "proxy" objects, and I don't fully understand how all of the proxying
> logic works. For example, do the objects returned by the lazy importer
> obey "normal" python conventions, such as returning the appropriate
> type when isinstance() is called?

https://docs.python.org/2/reference/datamodel.html#customizing-instance-and-subclass-checks
says that isinstance() and issubclass() are implemented by calling
__instancecheck__() and __subclasscheck__() on the type being checked
against, i.e.:

    isintance(thing, TypeA) --> TypeA.__instancecheck__(thing)
    issubclass(TypeA, TypeB) --> TypeB.__subclasscheck__(TypeA)

The Stub instances that this branch deposits override __getattribute__()
to forward attribute look-ups to the lazily-loaded object. This method
always called, even when accessing preexisting attributes. The similar
__getattr__() method is only called for attributes that don't exist.

The Stub instances also override __setattr__(), __delattr__(), and
__call__() but it's possible these are not actually required. I will try
removing them. I will also add isinstance() and issubclass() checks.

It's possible that the one thing that these Stub instances won't fool is
an `is` check, e.g. `stub is Something`, because `is` is not implemented
as a call to a __double_underscore__() method.

>
> It seems that another way to solve the circular import problem would
> be to import modules rather than functions at the top level, so that
> the specific dependencies we need aren't resolved until the correct
> time. But that would lead to code like:

This is a good point. It works up to a point. Subclassing, for example,
demands that we dereference names within the module. However, the code
in this branch can't help that situation either.

We may be able to solve all circular reference problems by only ever
importing modules. We might need to avoid code in packages' __init__.py
too.

This has a few downsides:

* Lint checks will catch fewer naming issues; none currently check that
  a name exists in the target module.

* Patching in tests, while not an ideal practice, would have to be done
  in the owning module rather than in the module under test. This can be
  problematic (patching too much) but nothing we can't work around.

* Readability. It helps to work with short names and we'd lose that.

* A small performance penalty, though nothing we ought to care about.

None of those are particularly bad. Your perspective has made me realise
that lazy-importing is (only?) useful for:

(a) Using short names for convenience and readability.

(b) Deferring work to increase start-up performance.

Now, (b) is irrelevant to us, and (a) might not be a big enough win to
justify this branch's code.

>
> from maasserver.models import fabric
>
> ...
>
> def foo():
> f = fabric.Fabric()
> # or...
> Fabric = fabric.Fabric
> f = Fabric()
>
> ... which seems less than ideal. (but perhaps no worse than th...

[MAAS Lander (maas-lander)]

Transitioned to Git.

lp:maas has now moved from Bzr to Git.
Please propose your branches with Launchpad using Git.

git clone https://git.launchpad.net/maas

Unmerged revisions

4018. By Gavin Panella on 2015-10-14

Merge trunk, resolving minor conflicts.

4017. By Gavin Panella on 2015-06-30

Update tests and importer()'s docstring.

4016. By Gavin Panella on 2015-06-30

Actually add the tests.

4015. By Gavin Panella on 2015-06-30

Extract much of update_references() into gen_reference_updates().

4014. By Gavin Panella on 2015-06-30

Remove talk of changing frames; local variables can be read but not written.

4013. By Gavin Panella on 2015-06-30

Tests for lazy.find() and lazy.update_references().

4012. By Gavin Panella on 2015-06-11

Move the lazy module importer to a module of its own.

4011. By Gavin Panella on 2015-06-10

lazy() helper for circular imports and deferred loading, work in progress.

=== modified file 'src/provisioningserver/utils/__init__.py'
--- src/provisioningserver/utils/__init__.py	2015-06-29 13:42:10 +0000
+++ src/provisioningserver/utils/__init__.py	2015-10-14 14:00:38 +0000
@@ -48,6 +48,7 @@
     NoConnectionsAvailable,
     NodeAlreadyExists,
 )
+from provisioningserver.utils import lazy
 from provisioningserver.utils.twisted import (
     asynchronous,
     pause,
@@ -66,6 +67,11 @@
 )
 
 
+region = lazy.importer("provisioningserver.rpc.region")
+ClusterConfiguration = lazy.importer(
+    "provisioningserver.config.ClusterConfiguration")
+
+
 maaslog = get_maas_logger("utils")
 
 
@@ -113,10 +119,6 @@
     :param power_parameters: The power parameters for the node, as a
         dict.
     """
-    # Avoid circular dependencies.
-    from provisioningserver.rpc.region import CreateNode
-    from provisioningserver.config import ClusterConfiguration
-
     with ClusterConfiguration.open() as config:
         cluster_uuid = config.cluster_uuid
 
@@ -139,7 +141,7 @@
     macs = sorted(set(macs))
     try:
         response = yield client(
-            CreateNode,
+            region.CreateNode,
             cluster_uuid=cluster_uuid,
             architecture=arch,
             power_type=power_type,
@@ -181,9 +183,6 @@
     :param system_id: system_id of node to commission.
     :param user: user for the node.
     """
-    # Avoid circular dependencies.
-    from provisioningserver.rpc.region import CommissionNode
-
     for elapsed, remaining, wait in retries(15, 5, reactor):
         try:
             client = getRegionClient()
@@ -197,7 +196,7 @@
 
     try:
         yield client(
-            CommissionNode,
+            region.CommissionNode,
             system_id=system_id,
             user=user)
     except CommissionNodeFailed as e:
 
=== added file 'src/provisioningserver/utils/lazy.py'
--- src/provisioningserver/utils/lazy.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/utils/lazy.py	2015-10-14 14:00:38 +0000
@@ -0,0 +1,137 @@
+# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Doing things lazily, like importing modules."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    "importer",
+]
+
+from collections import (
+    MutableMapping,
+    MutableSequence,
+    MutableSet,
+)
+import ctypes
+from functools import partial
+from gc import get_referrers
+from importlib import import_module
+from operator import setitem
+from threading import RLock
+
+# A Python `cell`, used internally to implement variables referenced by
+# multiple scopes. See https://docs.python.org/2/c-api/cell.html.
+PyCell = type((lambda o: lambda: o)(0).func_closure[0])
+
+# Mechanism to delve into Python's brain to *set* the contents of cells.
+PyCell_Set = ctypes.pythonapi.PyCell_Set
+
+
+def find(name):
+    """Find the given `name` as a module or object within."""
+    try:
+        return import_module(name)
+    except ImportError:
+        name, _, last = name.rpartition(".")
+        if len(name) > 0:
+            try:
+                return getattr(find(name), last)
+            except AttributeError:
+                raise ImportError(
+                    "Cannot import %s from %s" % (last, name))
+        else:
+            raise
+
+
+def gen_reference_updates(placeholder, actual):
+    """Generate the updates needed to replace `placeholder` with `actual`.
+
+    Currently this can only replaces references within mutable mappings,
+    lists, and sets, and within cells. This returns no-argument callables;
+    these should be called only *after* exhausting this generator.
+    """
+    for referrer in get_referrers(placeholder):
+        if isinstance(referrer, MutableMapping):
+            for key, value in referrer.viewitems():
+                if value is placeholder:
+                    yield partial(setitem, referrer, key, actual)
+        elif isinstance(referrer, MutableSequence):
+            for index, value in enumerate(referrer):
+                if value is placeholder:
+                    yield partial(setitem, referrer, index, actual)
+        elif isinstance(referrer, MutableSet):
+            yield partial(referrer.discard, placeholder)
+            yield partial(referrer.add, actual)
+        elif isinstance(referrer, PyCell):
+            # This was an attempt to get updates in dicts working, but it does
+            # not appear to have any effect in that regard.
+            yield partial(
+                PyCell_Set, ctypes.py_object(referrer),
+                ctypes.py_object(actual))
+        else:
+            # Don't yet know how to handle this. For example, this might be a
+            # frame (it's not possible to update locals without major surgery)
+            # or an immutable container.
+            pass
+
+
+def update_references(placeholder, actual):
+    """Update references to `placeholder` with `actual`.
+
+    See `gen_reference_updates` for limitations.
+    """
+    updates = gen_reference_updates(placeholder, actual)
+    for update in list(updates):
+        update()
+
+
+def importer(name):
+    """Import the given module lazily.
+
+    This returns a stub object that proxies attribute access and calls through
+    to the lazily imported object. On first attribute access, or on the first
+    call, the stub will be replaced in modules, dicts, lists, and sets
+    throughout the running interpreter.
+
+    There are some limitations. It can't replace references in local
+    variables. It can't replace references to objects without a ``__dict__``
+    attribute when that reference is within a dict.
+
+    :param name: The dotted path to a module or object within that module,
+        e.g. "provisioningserver.config.ClusterConfiguration".
+    """
+
+    def load(store=[], lock=RLock()):
+        with lock:
+            if len(store) == 0:
+                thing = find(name)
+                update_references(stub, thing)
+                store.append(thing)
+        return store[0]
+
+    class Stub:
+
+        def __getattribute__(self, name):
+            return getattr(load(), name)
+
+        def __setattr__(self, name, value):
+            return setattr(load(), name, value)
+
+        def __delattr__(self, name):
+            return delattr(load(), name)
+
+        def __call__(self, *args, **kwargs):
+            return load()(*args, **kwargs)
+
+    stub = Stub()
+
+    return stub
 
=== added file 'src/provisioningserver/utils/tests/test_lazy.py'
--- src/provisioningserver/utils/tests/test_lazy.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/utils/tests/test_lazy.py	2015-10-14 14:00:38 +0000
@@ -0,0 +1,145 @@
+# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `provisioningserver.utils.lazy`."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+import sys
+
+from maastesting.factory import factory
+from maastesting.testcase import MAASTestCase
+from provisioningserver.utils import lazy
+from testtools.matchers import Contains
+
+
+class TestFind(MAASTestCase):
+    """Tests for `provisioningserver.utils.lazy.find`."""
+
+    def test_finds_builtin_module(self):
+        self.assertIs(sys, lazy.find("sys"))
+
+    def test_finds_module_on_sys_path(self):
+        self.assertIs(lazy, lazy.find("provisioningserver.utils.lazy"))
+
+    def test_finds_attribute_in_builtin(self):
+        self.assertIs(sys.exit, lazy.find("sys.exit"))
+
+    def test_finds_attribute_in_module_on_sys_path(self):
+        self.assertIs(lazy.find, lazy.find(
+            "provisioningserver.utils.lazy.find"))
+
+    def test_raises_ImportError_when_module_cannot_be_found(self):
+        name = factory.make_name()
+        error = self.assertRaises(ImportError, lazy.find, name)
+        self.assertEqual("No module named " + name, unicode(error))
+
+    def test_raises_ImportError_when_attribute_cannot_be_found(self):
+        name = factory.make_name("attr")
+        error = self.assertRaises(ImportError, lazy.find, "sys." + name)
+        self.assertEqual("Cannot import %s from sys" % name, unicode(error))
+
+
+class TestUpdateReferences(MAASTestCase):
+    """Tests for `provisioningserver.utils.lazy.update_references`."""
+
+    def test_does_not_update_locals(self):
+        ref = object()
+        lazy.update_references(ref, sys)
+        self.assertIsNot(ref, sys)
+
+    def test_updates_reference_in_module(self):
+        global ref_in_module
+        ref_in_module = object()
+        self.assertIsNot(sys, ref_in_module)
+        lazy.update_references(ref_in_module, sys)
+        self.assertIs(sys, ref_in_module)
+
+    def test_updates_reference_in_dict_value(self):
+        thing = type(b"Thing", (object,), {})()
+        ref_in_dict = {None: thing}
+        self.assertIsNot(sys, ref_in_dict[None])
+        lazy.update_references(thing, sys)
+        self.assertIs(sys, ref_in_dict[None])
+
+    def test_does_not_update_reference_in_dict_key(self):
+        thing = type(b"Thing", (object,), {})()
+        ref_in_dict = {thing: None}
+        self.assertThat(ref_in_dict, Contains(thing))
+        lazy.update_references(thing, sys)
+        self.assertThat(ref_in_dict, Contains(thing))
+
+    def test_does_not_update_reference_to_dictless_object_in_dict_value(self):
+        thing = type(b"Thing", (object,), {b"__slots__": ()})()
+        ref_in_dict = {None: thing}
+        self.assertIsNot(sys, ref_in_dict[None])
+        lazy.update_references(ref_in_dict[None], sys)
+        self.assertIsNot(sys, ref_in_dict[None])
+
+    def test_does_not_update_reference_to_dictless_object_in_dict_key(self):
+        thing = type(b"Thing", (object,), {b"__slots__": ()})()
+        ref_in_dict = {thing: None}
+        self.assertThat(ref_in_dict, Contains(thing))
+        lazy.update_references(thing, sys)
+        self.assertThat(ref_in_dict, Contains(thing))
+
+    def test_updates_reference_in_list(self):
+        ref_in_list = [object()]
+        self.assertIsNot(sys, ref_in_list[0])
+        lazy.update_references(ref_in_list[0], sys)
+        self.assertIs(sys, ref_in_list[0])
+
+    def test_updates_reference_in_set(self):
+        ref_in_set = {object()}
+        self.assertIsNot(sys, list(ref_in_set)[0])
+        lazy.update_references(list(ref_in_set)[0], sys)
+        self.assertIs(sys, list(ref_in_set)[0])
+
+
+class TestImporter(MAASTestCase):
+    """Tests for `provisioningserver.utils.lazy.importer`."""
+
+    def test_returns_stub(self):
+        stub = lazy.importer("sys")
+        self.assertIsNot(stub, sys)
+        self.assertEqual("Stub", type(stub).__name__)
+
+    def test_does_not_update_import_in_locals(self):
+        stub_in_locals = lazy.importer("sys")
+        self.assertIsNot(sys, stub_in_locals)
+        stub_in_locals.exit  # Look-up an attribute.
+        self.assertIsNot(sys, stub_in_locals)
+
+    def test_updates_import_in_module(self):
+        global stub_in_module
+        stub_in_module = lazy.importer("sys")
+        self.assertIsNot(sys, stub_in_module)
+        stub_in_module.exit  # Look-up an attribute.
+        self.assertIs(sys, stub_in_module)
+
+    def test_updates_import_in_dict(self):
+        stub_in_dict = {None: lazy.importer("sys")}
+        self.assertIsNot(sys, stub_in_dict[None])
+        stub_in_dict[None].exit  # Look-up an attribute.
+        self.assertIs(sys, stub_in_dict[None])
+
+    def test_updates_import_in_list(self):
+        stub_in_list = [lazy.importer("sys")]
+        self.assertIsNot(sys, stub_in_list[0])
+        stub_in_list[0].exit  # Look-up an attribute.
+        self.assertIs(sys, stub_in_list[0])
+
+    def test_updates_import_in_set(self):
+        stub_in_set = {lazy.importer("sys")}
+        self.assertIsNot(sys, list(stub_in_set)[0])
+        list(stub_in_set)[0].exit  # Look-up an attribute.
+        self.assertIs(sys, list(stub_in_set)[0])
 
=== modified file 'src/provisioningserver/utils/tests/test_utils.py'
--- src/provisioningserver/utils/tests/test_utils.py	2015-06-29 13:42:10 +0000
+++ src/provisioningserver/utils/tests/test_utils.py	2015-10-14 14:00:38 +0000
@@ -487,9 +487,8 @@
         system_id = factory.make_name("system-id")
         protocol.CreateNode.return_value = defer.succeed(
             {"system_id": system_id})
-        get_cluster_uuid = self.patch(
-            provisioningserver.utils, 'get_cluster_uuid')
-        get_cluster_uuid.return_value = 'cluster-' + factory.make_UUID()
+        test_uuid = factory.make_UUID()
+        self.useFixture(ClusterConfigurationFixture(cluster_uuid=test_uuid))
 
         uuid = 'node-' + factory.make_UUID()
         macs = sorted(factory.make_mac_address() for _ in range(3))
@@ -513,7 +512,6 @@
         system_id = factory.make_name("system-id")
         protocol.CreateNode.return_value = defer.succeed(
             {"system_id": system_id})
-
         test_uuid = factory.make_UUID()
         self.useFixture(ClusterConfigurationFixture(cluster_uuid=test_uuid))