Module Reloading Enhancements (#77)

This PR improves the handling of module reloading to
prevent changed dependencies between modified modules
from preventing the reload process from completing
successfully. The failure mode comes about when changes
in one module are imported into another module and by
the previous sorting method, the importing module is
reloaded first.

.gitlab-ci.yml

@@ -1,79 +1,81 @@
 stages:
-  - check
-  - broadcast
+- check
+- broadcast
 
 pytest-py36:
   image: python:3.6
   stage: check
   script:
-    - export PYTHONPATH="${PYTHONPATH}:$(pwd)"
-    - pip install -r requirements.txt
-    - >
-      py.test
-      --verbose
-      --cov-report term
-      --cov=cauldron
-      .
+  - export PYTHONPATH="${PYTHONPATH}:$(pwd)"
+  - pip install -r requirements.txt
+  - >
+    py.test
+    --verbose
+    --cov-report term
+    --cov=cauldron
+    .
 
 pytest-py37:
   image: python:3.7
   stage: check
   script:
-    - export PYTHONPATH="${PYTHONPATH}:$(pwd)"
-    - pip install -r requirements.txt
-    - pip install codecov coveralls codacy-coverage
-    - >
-      py.test
-      --verbose
-      --cov-report xml:"$(pwd)/coverage.xml"
-      --cov-report term
-      --cov=cauldron
-      .
+  - export PYTHONPATH="${PYTHONPATH}:$(pwd)"
+  - pip install -r requirements.txt
+  - pip install codecov coveralls codacy-coverage
+  - >
+    py.test
+    --verbose
+    --cov-report xml:"$(pwd)/coverage.xml"
+    --cov-report term
+    --cov=cauldron
+    .
 
 pytest-py38:
   image: python:3.8
   stage: check
-  coverage: '/^TOTAL.*\s+\d+\s+\d+\s+(\d+)%/'
   script:
-    - export PYTHONPATH="${PYTHONPATH}:$(pwd)"
-    - pip install -r requirements.txt
-    - >
-      py.test
-      --verbose
-      --cov-report term
-      --cov=cauldron
-      .
+  - export PYTHONPATH="${PYTHONPATH}:$(pwd)"
+  - pip install -r requirements.txt
+  - >
+    py.test
+    --verbose
+    --cov-report term
+    --cov=cauldron
+    .
   artifacts:
     paths:
-      - .coverage
-      - coverage.xml
+    - .coverage
+    - coverage.xml
     expire_in: 1 day
 
 pytest-py39:
   image: python:3.9
   stage: check
+  coverage: '/^TOTAL.*\s+\d+\s+\d+\s+(\d+)%/'
   script:
-    - export PYTHONPATH="${PYTHONPATH}:$(pwd)"
-    - pip install -r requirements.txt
-    - >
-      py.test
-      --verbose
-      --cov-report term
-      --cov=cauldron
-      .
+  - export PYTHONPATH="${PYTHONPATH}:$(pwd)"
+  - pip install -r requirements.txt
+  - >
+    py.test
+    --verbose
+    --cov-report term
+    --cov=cauldron
+    .
 
 codecov:
   image: python:3.9
   stage: broadcast
+  allow_failure: true
   script:
-    - pip install codecov
-    - ls -la
-    - codecov
+  - pip install codecov
+  - ls -la
+  - codecov
 
 coveralls:
   image: python:3.9
   stage: broadcast
+  allow_failure: true
   script:
-    - pip install coveralls
-    - ls -la
-    - coveralls --verbose
+  - pip install coveralls
+  - ls -la
+  - coveralls --verbose

cauldron/cli/server/run.py

@@ -14,22 +14,6 @@
 from cauldron.render.encoding import ComplexFlaskJsonEncoder
 from cauldron.session import writing
 
-# Optional includes for APM monitoring during development.
-if os.environ.get('ENABLE_APM') is not None:  # pragma: no-cover
-    try:
-        from scout_apm.flask import ScoutApm
-    except ImportError:
-        ScoutApm = None
-
-    try:
-        # Optional include for APM monitoring during development.
-        import newrelic.agent
-        newrelic.agent.initialize()
-    except ImportError:
-        pass
-else:
-    ScoutApm = None
-
 APPLICATION = Flask('Cauldron')
 APPLICATION.json_encoder = ComplexFlaskJsonEncoder
 SERVER_VERSION = [0, 0, 1, 1]
@@ -188,15 +172,6 @@ def create_application(
 
     environ.modes.add(environ.modes.INTERACTIVE)
 
-    if ScoutApm is not None and os.environ.get('SCOUT_KEY'):
-        ScoutApm(APPLICATION)
-        APPLICATION.config['SCOUT_MONITOR'] = True
-        APPLICATION.config['SCOUT_KEY'] = os.environ['SCOUT_KEY']
-        APPLICATION.config['SCOUT_NAME'] = os.environ.get(
-            'SCOUT_NAME',
-            'cauldron-kernel',
-        )
-
     return {'application': APPLICATION, **server_data}
 
 

cauldron/runner/__init__.py

@@ -23,8 +23,7 @@ def add_library_path(path: str) -> bool:
         Whether or not the path was added. Only returns False if the path was
         not added because it doesn't exist
     """
-
-    if not os.path.exists(path):
+    if not path or not os.path.exists(path):
         return False
 
     if path not in sys.path:
@@ -85,47 +84,90 @@ def close():
     return True
 
 
-def reload_libraries(library_directories: list = None):
+def _reload_module(path: str, library_directory: str):
+    """
+    Reloads the module at the specified path within the package rooted at
+    the given library_directory.
+    """
+    path = os.path.dirname(path) if path.endswith('__init__.py') else path
+    start_index = len(library_directory) + 1
+    end_index = -3 if path.endswith('.py') else None
+    package_path = path[start_index:end_index]
+
+    module = sys.modules.get(package_path.replace(os.sep, '.'))
+    return importlib.reload(module) if module is not None else None
+
+
+def _reload_library(directory: str) -> list:
+    """
+    Carries out a reload action on the specified root library directory that is
+    assumed to contain a python local package with potential module changes.
+
+    :param directory:
+        Root directory of the library package to reload.
+    """
+    if not add_library_path(directory):
+        # If the library wasn't added because it doesn't exist, remove it
+        # in case the directory has recently been deleted and then return
+        # an empty result
+        remove_library_path(directory)
+        return []
+
+    glob_path = os.path.join(os.path.realpath(directory), '**', '*.py')
+
+    # Force file paths to be sorted by hierarchy from deepest to shallowest,
+    # which ensures that changes are reloaded by children before any dependencies
+    # are encountered in parents.
+    found_file_paths = sorted(
+        glob.glob(glob_path, recursive=True),
+        key=lambda p: "{}--{}".format(str(p.count(os.sep)).zfill(4), p),
+        reverse=True,
+    )
+
+    # Iterate over imports multiple times in case there's a failed import as the
+    # result of dependency changes between multiple files. However, after 20
+    # iterations give up and fail.
+    outputs = []
+    last_error = None
+    for i in range(20):
+        for path in [*found_file_paths]:
+            try:
+                outputs.append(_reload_module(path, directory))
+                # Remove the path if the reload operation succeeded.
+                found_file_paths.remove(path)
+            except Exception as error:
+                # Ignore failures and hope they can be resolved in another pass.
+                last_error = error
+
+        if not found_file_paths:
+            # If there's nothing left to reload, return the reloaded modules.
+            return outputs
+
+    # If 20 attempts to reload modules fail, it's time to error out.
+    raise RuntimeError(
+        "Failed to reload modified modules. This could be due to a circular import."
+    ) from last_error
+
+
+def reload_libraries(library_directories: list = None) -> list:
     """
     Reload the libraries stored in the project's local and shared library
-    directories
+    directories to ensure that any modifications since the previous load/reload
+    have been refreshed.
     """
     directories = library_directories or []
     project = cauldron.project.get_internal_project()
     if project:
         directories += project.library_directories
 
     if not directories:
-        return
-
-    def reload_module(path: str, library_directory: str):
-        path = os.path.dirname(path) if path.endswith('__init__.py') else path
-        start_index = len(library_directory) + 1
-        end_index = -3 if path.endswith('.py') else None
-        package_path = path[start_index:end_index]
-
-        module = sys.modules.get(package_path.replace(os.sep, '.'))
-        return importlib.reload(module) if module is not None else None
-
-    def reload_library(directory: str) -> list:
-        if not add_library_path(directory):
-            # If the library wasn't added because it doesn't exist, remove it
-            # in case the directory has recently been deleted and then return
-            # an empty result
-            remove_library_path(directory)
-            return []
-
-        glob_path = os.path.join(directory, '**', '*.py')
-        return [
-            reload_module(path, directory)
-            for path in glob.glob(glob_path, recursive=True)
-        ]
+        return []
 
     return [
         reloaded_module
         for directory in directories
-        for reloaded_module in reload_library(directory)
-        if reload_module is not None
+        for reloaded_module in _reload_library(directory)
+        if reloaded_module is not None
     ]
 
 

cauldron/settings.json

@@ -1,4 +1,4 @@
 {
-  "version": "1.0.6",
+  "version": "1.0.7",
   "notebookVersion": "v1"
 }

cauldron/test/runner/test_reload_libraries.py

@@ -0,0 +1,49 @@
+import functools
+import pathlib
+import random
+from unittest.mock import MagicMock
+from unittest.mock import patch
+
+import pytest
+
+import cauldron
+from cauldron import runner
+
+
+def _mock_reload_module(history: dict, path: str, library_directory: str):
+    """Mocked version of the runner.__init__._reload_module function."""
+    output = {"path": path, "library_directory": library_directory}
+    if path not in history and random.random() > 0.5:
+        history[path] = output
+        raise ValueError("Faking that this did not go well for the first time.")
+
+    history[path] = output
+    return output
+
+
+@patch("cauldron.runner._reload_module")
+def test_reload(reload_module: MagicMock):
+    """Should reload as expected."""
+    history = {}
+    reload_module.side_effect = functools.partial(_mock_reload_module, history)
+    library_directory = pathlib.Path(cauldron.__file__).resolve().parent
+    output = runner.reload_libraries([None, str(library_directory)])
+
+    me = pathlib.Path(__file__).resolve()
+    root = pathlib.Path(cauldron.__file__).resolve()
+    keys = list(history.keys())
+    assert keys.index(str(me)) < keys.index(str(root)), """
+        Expecting deeper hierarchy to be reloaded first.
+        """
+
+    assert output, "Expect a non-empty list returned."
+
+
+@patch("cauldron.runner._reload_module")
+def test_reload_failure(reload_module: MagicMock):
+    """Should raise RuntimeError if imports fail after many retries."""
+    reload_module.side_effect = ValueError("Nope")
+    library_directory = pathlib.Path(cauldron.__file__).resolve().parent
+
+    with pytest.raises(RuntimeError):
+        runner.reload_libraries([None, str(library_directory)])

cauldron/ui/__init__.py

@@ -1,5 +1,4 @@
 import logging
-import os
 
 import flask
 import waitress
@@ -18,22 +17,6 @@
 from cauldron.ui.routes.apis import executions as executions_routes
 from cauldron.ui.routes.apis import statuses as statuses_routes
 
-# Optional includes for APM monitoring during development.
-if os.environ.get('ENABLE_APM') is not None:  # pragma: no-cover
-    try:
-        from scout_apm.flask import ScoutApm
-    except ImportError:
-        ScoutApm = None
-
-    try:
-        # Optional include for APM monitoring during development.
-        import newrelic.agent
-        newrelic.agent.initialize()
-    except ImportError:
-        pass
-else:
-    ScoutApm = None
-
 
 def create_application(
         port: int = None,
@@ -83,12 +66,6 @@ def create_application(
     app.register_blueprint(notebooks_routes.blueprint)
     app.register_blueprint(viewers_routes.blueprint)
 
-    if ScoutApm is not None and os.environ.get('SCOUT_KEY'):
-        ScoutApm(app)
-        app.config['SCOUT_MONITOR'] = True
-        app.config['SCOUT_KEY'] = os.environ['SCOUT_KEY']
-        app.config['SCOUT_NAME'] = os.environ.get('SCOUT_NAME', 'cauldron-ui')
-
     # Either used the specified port for the UI if one was given or
     # find the first available port in the given range and use that
     # one instead.