From c4f21b6931f8c48bca613489fef21ee0e65afac6 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 25 Feb 2025 18:51:15 -0500 Subject: [PATCH 1/2] fix: remove custom startup to fix dev reloading The cms/startup.py and lms/startup.py files were created to allow us to do a lot of custom initialization around things like the ModuleStore, monkey-patching, adding MIME types to our process, etc. As far back as 2017, we recognized that this was a bad thing, marked these modules as "deprecated", and started removing things or putting them in the standard Django locations for them (0279181). In its current state, these startup modules no longer do any custom work, and just invoke django.startup(). But this is meant for running Django code in "standalone" usage, e.g. if you have a script that isn't a management command but needs some Django functionality. The "runserver" command used during development normally launches a child process to serve requests and knows how to kill and respawn that process when files are modified, so that changes are reflected. It can also normally handle the case where there's a SyntaxError in the child process, and fixing that error will reload the code again. Something about running django.startup() manually interferes with this functionality in "runserver". It still reloads the code in response to changes, but if the code gets into a broken state for any reason (like a syntax error), the master process itself dies. That causes the container to restart, only to die again shortly afterwards in a loop until the error is fixed. The container restarts will break any shell you had opened into the container, as well as any IDE integrations that connected to that container to access the files and Python instance. Getting rid of the custom startup code fixes this and moves us one small step closer to being a more normal Django project. --- cms/conftest.py | 10 ---------- cms/startup.py | 20 ------------------- cms/wsgi.py | 3 --- lms/startup.py | 20 ------------------- lms/wsgi.py | 3 --- lms/wsgi_apache_lms.py | 3 --- manage.py | 7 ------- .../core/djangoapps/monkey_patch/__init__.py | 8 +++----- 8 files changed, 3 insertions(+), 71 deletions(-) delete mode 100644 cms/startup.py delete mode 100644 lms/startup.py diff --git a/cms/conftest.py b/cms/conftest.py index dc360e3df14b..80d971070a83 100644 --- a/cms/conftest.py +++ b/cms/conftest.py @@ -6,10 +6,7 @@ only running cms tests. """ - -import importlib import logging -import os import pytest @@ -29,13 +26,6 @@ def pytest_configure(config): else: logging.info("pytest did not register json_report correctly") - if config.getoption('help'): - return - settings_module = os.environ.get('DJANGO_SETTINGS_MODULE') - startup_module = 'cms.startup' if settings_module.startswith('cms') else 'lms.startup' - startup = importlib.import_module(startup_module) - startup.run() - @pytest.fixture(autouse=True, scope='function') def _django_clear_site_cache(): diff --git a/cms/startup.py b/cms/startup.py deleted file mode 100644 index 5500dc6424e5..000000000000 --- a/cms/startup.py +++ /dev/null @@ -1,20 +0,0 @@ -""" -Module for code that should run during Studio startup (deprecated) -""" - - -import django -from django.conf import settings - -# Force settings to run so that the python path is modified -settings.INSTALLED_APPS # pylint: disable=pointless-statement - - -def run(): - """ - Executed during django startup - - NOTE: DO **NOT** add additional code to this method or this file! The Platform Team - is moving all startup code to more standard locations using Django best practices. - """ - django.setup() diff --git a/cms/wsgi.py b/cms/wsgi.py index 0ccd953d98dd..9c9af4299844 100644 --- a/cms/wsgi.py +++ b/cms/wsgi.py @@ -18,9 +18,6 @@ import os # lint-amnesty, pylint: disable=wrong-import-order, wrong-import-position os.environ.setdefault("DJANGO_SETTINGS_MODULE", "cms.envs.aws") -import cms.startup as startup # lint-amnesty, pylint: disable=wrong-import-position -startup.run() - # This application object is used by the development server # as well as any WSGI server configured to use this file. from django.core.wsgi import get_wsgi_application # lint-amnesty, pylint: disable=wrong-import-order, wrong-import-position diff --git a/lms/startup.py b/lms/startup.py deleted file mode 100644 index ed1ad1caba29..000000000000 --- a/lms/startup.py +++ /dev/null @@ -1,20 +0,0 @@ -""" -Module for code that should run during LMS startup (deprecated) -""" - - -import django -from django.conf import settings - -# Force settings to run so that the python path is modified -settings.INSTALLED_APPS # pylint: disable=pointless-statement - - -def run(): - """ - Executed during django startup - - NOTE: DO **NOT** add additional code to this method or this file! The Platform Team - is moving all startup code to more standard locations using Django best practices. - """ - django.setup() diff --git a/lms/wsgi.py b/lms/wsgi.py index 169b6929fbfa..0321c827ccf5 100644 --- a/lms/wsgi.py +++ b/lms/wsgi.py @@ -15,9 +15,6 @@ import os # lint-amnesty, pylint: disable=wrong-import-order, wrong-import-position os.environ.setdefault("DJANGO_SETTINGS_MODULE", "lms.envs.aws") -import lms.startup as startup # lint-amnesty, pylint: disable=wrong-import-position -startup.run() - # This application object is used by the development server # as well as any WSGI server configured to use this file. from django.core.wsgi import get_wsgi_application # lint-amnesty, pylint: disable=wrong-import-order, wrong-import-position diff --git a/lms/wsgi_apache_lms.py b/lms/wsgi_apache_lms.py index e40be055d493..d1c6013e2bfc 100644 --- a/lms/wsgi_apache_lms.py +++ b/lms/wsgi_apache_lms.py @@ -14,9 +14,6 @@ os.environ.setdefault("DJANGO_SETTINGS_MODULE", "lms.envs.aws") os.environ.setdefault("SERVICE_VARIANT", "lms") -import lms.startup as startup # lint-amnesty, pylint: disable=wrong-import-position -startup.run() - # This application object is used by the development server # as well as any WSGI server configured to use this file. from django.core.wsgi import get_wsgi_application # lint-amnesty, pylint: disable=wrong-import-order, wrong-import-position diff --git a/manage.py b/manage.py index 98bcc4071799..07fa6de2f6e1 100755 --- a/manage.py +++ b/manage.py @@ -12,7 +12,6 @@ """ # pylint: disable=wrong-import-order, wrong-import-position - from openedx.core.lib.logsettings import log_python_warnings log_python_warnings() @@ -20,7 +19,6 @@ from openedx.core.lib.safe_lxml import defuse_xml_libs # isort:skip defuse_xml_libs() -import importlib import os import sys from argparse import ArgumentParser @@ -51,7 +49,6 @@ def parse_args(): help_string=lms.format_help(), settings_base='lms/envs', default_settings='lms.envs.devstack_docker', - startup='lms.startup', ) cms = subparsers.add_parser( @@ -70,7 +67,6 @@ def parse_args(): settings_base='cms/envs', default_settings='cms.envs.devstack_docker', service_variant='cms', - startup='cms.startup', ) edx_args, django_args = parser.parse_known_args() @@ -99,8 +95,5 @@ def parse_args(): # This will trigger django-admin.py to print out its help django_args.append('--help') - startup = importlib.import_module(edx_args.startup) - startup.run() - from django.core.management import execute_from_command_line execute_from_command_line([sys.argv[0]] + django_args) diff --git a/openedx/core/djangoapps/monkey_patch/__init__.py b/openedx/core/djangoapps/monkey_patch/__init__.py index ed4b9fc26198..1a28af925797 100644 --- a/openedx/core/djangoapps/monkey_patch/__init__.py +++ b/openedx/core/djangoapps/monkey_patch/__init__.py @@ -6,9 +6,8 @@ * USE WITH CAUTION * No, but seriously, you probably never really want to make changes here. This module contains methods to monkey-patch [0] the edx-platform. -Patches are to be applied as early as possible in the callstack -(currently lms/startup.py and cms/startup.py). Consequently, changes -made here will affect the entire platform. +Patches are to be applied as early as possible in the callstack). Consequently, +changes made here will affect the entire platform. That said, if you've decided you really need to monkey-patch the platform (and you've convinced enough people that this is best @@ -25,8 +24,7 @@ - is_patched - patch - unpatch - - Add the following code where needed (typically cms/startup.py and - lms/startup.py): + - Add the following code where needed: ``` from openedx.core.djangoapps.monkey_patch import your_module your_module.patch() From d5aa834b6c7787e9d7d4bc6d9343081942499f29 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 28 Feb 2025 22:46:08 -0500 Subject: [PATCH 2/2] refactor: move get_user_model call out of method The get_service_user method used to do a local call to the get_user_model function because it was not guaranteed to be properly initialized at the time of import. This was partly due to how we did custom initialization using lms/startup.py, but it was also because when it was implemented (commit f318661), the platform was still running on Django 1.8.18. At that time, get_user_model was guaranteed to work only after Django has imported all models. In Django 1.11, the behavior of get_user_model was changed: https://docs.djangoproject.com/en/1.11/releases/1.11/#django-contrib-auth > get_user_model() can now be called at import time, > even in modules that define models. Now that lms/startup.py is gone and get_user_model is safe to call at the module level, I'm refactoring the catalog app's models.py file to follow the convention we use everywhere else in edx-platform with respect to get_user_model. --- openedx/core/djangoapps/catalog/models.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/catalog/models.py b/openedx/core/djangoapps/catalog/models.py index e44a7839b38a..98a012e3c365 100644 --- a/openedx/core/djangoapps/catalog/models.py +++ b/openedx/core/djangoapps/catalog/models.py @@ -1,6 +1,4 @@ """Models governing integration with the catalog service.""" - - from config_models.models import ConfigurationModel from django.conf import settings from django.contrib.auth import get_user_model @@ -9,6 +7,8 @@ from openedx.core.djangoapps.site_configuration import helpers +User = get_user_model() + class CatalogIntegration(ConfigurationModel): """ @@ -72,7 +72,4 @@ def get_internal_api_url(self): return helpers.get_value('COURSE_CATALOG_API_URL', settings.COURSE_CATALOG_API_URL) def get_service_user(self): - # NOTE: We load the user model here to avoid issues at startup time that result from the hacks - # in lms/startup.py. - User = get_user_model() # pylint: disable=invalid-name return User.objects.get(username=self.service_username)