From af6ad42e00656ad24dfa3bf1f8d3f30195d36e29 Mon Sep 17 00:00:00 2001 From: Scott Ernst Date: Mon, 26 Apr 2021 09:28:00 -0500 Subject: [PATCH] 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 | 92 ++++++++-------- cauldron/cli/server/run.py | 25 ----- cauldron/runner/__init__.py | 102 ++++++++++++------ cauldron/settings.json | 2 +- cauldron/test/runner/test_reload_libraries.py | 49 +++++++++ cauldron/ui/__init__.py | 23 ---- deployment.md | 2 + 7 files changed, 171 insertions(+), 124 deletions(-) create mode 100644 cauldron/test/runner/test_reload_libraries.py diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5ba749dd..c24906ed 100644 --- a/.gitlab-ci.yml +++ b/.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 diff --git a/cauldron/cli/server/run.py b/cauldron/cli/server/run.py index 2c661306..325e0ec0 100644 --- a/cauldron/cli/server/run.py +++ b/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} diff --git a/cauldron/runner/__init__.py b/cauldron/runner/__init__.py index 8af433c1..a7f4771f 100644 --- a/cauldron/runner/__init__.py +++ b/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,10 +84,76 @@ 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() @@ -96,36 +161,13 @@ def reload_libraries(library_directories: list = None): 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 ] diff --git a/cauldron/settings.json b/cauldron/settings.json index cc39ced8..fcdc70c0 100644 --- a/cauldron/settings.json +++ b/cauldron/settings.json @@ -1,4 +1,4 @@ { - "version": "1.0.6", + "version": "1.0.7", "notebookVersion": "v1" } diff --git a/cauldron/test/runner/test_reload_libraries.py b/cauldron/test/runner/test_reload_libraries.py new file mode 100644 index 00000000..f6ebdc06 --- /dev/null +++ b/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)]) diff --git a/cauldron/ui/__init__.py b/cauldron/ui/__init__.py index d307e1ed..74bbad42 100644 --- a/cauldron/ui/__init__.py +++ b/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. diff --git a/deployment.md b/deployment.md index 1e11de39..e60c2a7b 100644 --- a/deployment.md +++ b/deployment.md @@ -15,6 +15,7 @@ $ python3 setup.py sdist bdist_wheel $ twine upload dist/cauldron* $ docker run --rm -it -v $(pwd):/cauldron continuumio/anaconda3 /bin/bash $ cd /cauldron +$ conda config --set anaconda_upload yes $ python3 conda-recipe/conda-builder.py ``` @@ -28,6 +29,7 @@ PS> rm dist -r -fo > twine upload dist/cauldron* > docker run --rm -it -v ${pwd}:/cauldron continuumio/anaconda3 /bin/bash > cd /cauldron +> conda config --set anaconda_upload yes > python conda-recipe/conda-builder.py ```