From adb69635238aa65f9485277d78a7f82e0fd9b035 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 7 Sep 2026 12:02:07 +0100 Subject: [PATCH] fix(rename): address review on the package rename - CI installs the backend requirements from docsgpt/; the old cd into application/ silently installed nothing. - The root .dockerignore re-admits only application/__init__.py. An upgraded checkout may still hold gitignored application/{inputs,indexes,vectors,.env} from the old layout, and the directory rule shipped them into the image. - The compose files keep the host bind mounts on application/{indexes,inputs, vectors}, so an upgrade does not start with empty data. The move comes with the packaging work, together with an upgrade note. - The alias loader puts the real docsgpt spec back on the shared module object after import (the import machinery stamped the alias spec on it, which made importlib.reload rename the module and skip re-execution) and delegates get_code/get_source/get_filename to the target loader, so python -m application. runs. - Each legacy application.* task name is registered as its own task object, a subclass carrying the old name. Registering the same object under two keys made Celery's tracer log every run under whichever name it built last. - The redbeat key prefix stays redbeat:docsgpt:; the three schedule_syncs entries get stable names instead. redbeat tracks its static entries and deletes the ones that vanish from beat_schedule at start-up, and rewrites the task path of named entries in place, so neither a prefix bump nor a cleanup pass is needed (checked against redbeat 2.4.2 with a seeded Redis). --- .dockerignore | 4 +- .github/workflows/pytest.yml | 2 +- application/__init__.py | 42 +++++++++---- deployment/docker-compose-azure.yaml | 9 ++- deployment/docker-compose-hub.yaml | 18 ++++-- deployment/docker-compose.yaml | 18 ++++-- docs/content/Deploying/Docker-Deploying.mdx | 4 +- docs/content/upgrading.mdx | 20 +++++++ docsgpt/Dockerfile | 2 +- docsgpt/api/user/tasks.py | 3 + docsgpt/celery_init.py | 22 ++++--- docsgpt/celeryconfig.py | 5 +- tests/api/user/test_tasks.py | 13 +++++ tests/test_package_rename.py | 65 +++++++++++++++------ 14 files changed, 168 insertions(+), 59 deletions(-) diff --git a/.dockerignore b/.dockerignore index d9df5730..0521228c 100644 --- a/.dockerignore +++ b/.dockerignore @@ -3,7 +3,9 @@ # what the image needs; everything else (frontend, docs, tests, venvs) stays out. * !docsgpt/ -!application/ +# Only the alias file: an upgraded checkout may still hold gitignored +# application/{inputs,indexes,vectors,.env} from the old layout. +!application/__init__.py # Inside the package: caches, local runtime data and secrets never ship. **/__pycache__/ diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index 8b563bdb..3e31db75 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -20,7 +20,7 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - cd application + cd docsgpt if [ -f requirements.txt ]; then pip install -r requirements.txt; fi cd ../tests if [ -f requirements.txt ]; then pip install -r requirements.txt; fi diff --git a/application/__init__.py b/application/__init__.py index 1863e1c8..49ce5dc8 100644 --- a/application/__init__.py +++ b/application/__init__.py @@ -3,8 +3,9 @@ Every ``import application.x.y`` resolves to the already-imported ``docsgpt.x.y`` module object, so there is exactly one settings object, one Celery app and one Flask app however a process refers to them. Entry points such as -``celery -A application.app.celery`` and ``uvicorn application.asgi:asgi_app`` -keep working; update them to ``docsgpt.…`` before the alias is removed. +``celery -A application.app.celery``, ``uvicorn application.asgi:asgi_app`` and +``python -m application.scripts.`` keep working; update them to +``docsgpt.…`` before the alias is removed. """ from __future__ import annotations @@ -20,16 +21,37 @@ _NEW = "docsgpt" class _AliasLoader(importlib.abc.Loader): - """Hand back the ``docsgpt`` module instead of executing anything.""" + """Hand back the ``docsgpt`` module object; delegate code access to its real loader.""" - def __init__(self, target: str) -> None: + def __init__(self, target: str, target_spec) -> None: self._target = target + self._target_spec = target_spec + self._original_spec = None def create_module(self, spec): - return importlib.import_module(self._target) + module = importlib.import_module(self._target) + self._original_spec = module.__spec__ + return module def exec_module(self, module) -> None: - return None + # The import machinery stamps the alias spec on the shared module + # object; put the real one back so importlib.reload and __spec__-based + # lookups keep addressing the module by its docsgpt name. + if self._original_spec is not None: + module.__spec__ = self._original_spec + + # runpy (``python -m application.x``) reads the code through the loader. + def get_code(self, fullname): + return self._target_spec.loader.get_code(self._target) + + def get_source(self, fullname): + return self._target_spec.loader.get_source(self._target) + + def get_filename(self, fullname): + return self._target_spec.loader.get_filename(self._target) + + def is_package(self, fullname): + return self._target_spec.submodule_search_locations is not None class _AliasFinder(importlib.abc.MetaPathFinder): @@ -39,12 +61,10 @@ class _AliasFinder(importlib.abc.MetaPathFinder): if name != _OLD and not name.startswith(_OLD + "."): return None new_name = _NEW + name[len(_OLD):] - spec = importlib.util.find_spec(new_name) - if spec is None: + target_spec = importlib.util.find_spec(new_name) + if target_spec is None: return None - return importlib.util.spec_from_loader( - name, _AliasLoader(new_name), is_package=spec.submodule_search_locations is not None - ) + return importlib.util.spec_from_loader(name, _AliasLoader(new_name, target_spec)) warnings.warn( diff --git a/deployment/docker-compose-azure.yaml b/deployment/docker-compose-azure.yaml index 10badc24..97b19295 100644 --- a/deployment/docker-compose-azure.yaml +++ b/deployment/docker-compose-azure.yaml @@ -44,9 +44,12 @@ services: ports: - "7091:7091" volumes: - - ../docsgpt/indexes:/app/docsgpt/indexes - - ../docsgpt/inputs:/app/docsgpt/inputs - - ../docsgpt/vectors:/app/docsgpt/vectors + # Host data stays under application/ (the old package directory) for this + # release so an upgraded checkout keeps its indexes; it moves with the + # packaging work, together with an upgrade note. + - ../application/indexes:/app/docsgpt/indexes + - ../application/inputs:/app/docsgpt/inputs + - ../application/vectors:/app/docsgpt/vectors depends_on: redis: condition: service_started diff --git a/deployment/docker-compose-hub.yaml b/deployment/docker-compose-hub.yaml index f31e5b1a..0f2a8a80 100644 --- a/deployment/docker-compose-hub.yaml +++ b/deployment/docker-compose-hub.yaml @@ -44,9 +44,12 @@ services: ports: - "7091:7091" volumes: - - ../docsgpt/indexes:/app/indexes - - ../docsgpt/inputs:/app/inputs - - ../docsgpt/vectors:/app/vectors + # Host data stays under application/ (the old package directory) for this + # release so an upgraded checkout keeps its indexes; it moves with the + # packaging work, together with an upgrade note. + - ../application/indexes:/app/indexes + - ../application/inputs:/app/inputs + - ../application/vectors:/app/vectors depends_on: redis: condition: service_started @@ -68,9 +71,12 @@ services: - CACHE_REDIS_URL=redis://redis:6379/2 - POSTGRES_URI=postgresql://docsgpt:docsgpt@postgres:5432/docsgpt volumes: - - ../docsgpt/indexes:/app/indexes - - ../docsgpt/inputs:/app/inputs - - ../docsgpt/vectors:/app/vectors + # Host data stays under application/ (the old package directory) for this + # release so an upgraded checkout keeps its indexes; it moves with the + # packaging work, together with an upgrade note. + - ../application/indexes:/app/indexes + - ../application/inputs:/app/inputs + - ../application/vectors:/app/vectors depends_on: redis: condition: service_started diff --git a/deployment/docker-compose.yaml b/deployment/docker-compose.yaml index 3fd4b6d6..0eadaece 100644 --- a/deployment/docker-compose.yaml +++ b/deployment/docker-compose.yaml @@ -58,9 +58,12 @@ services: ports: - "7091:7091" volumes: - - ../docsgpt/indexes:/app/indexes - - ../docsgpt/inputs:/app/inputs - - ../docsgpt/vectors:/app/vectors + # Host data stays under application/ (the old package directory) for this + # release so an upgraded checkout keeps its indexes; it moves with the + # packaging work, together with an upgrade note. + - ../application/indexes:/app/indexes + - ../application/inputs:/app/inputs + - ../application/vectors:/app/vectors depends_on: redis: condition: service_started @@ -93,9 +96,12 @@ services: - CACHE_REDIS_URL=redis://redis:6379/2 - POSTGRES_URI=postgresql://docsgpt:docsgpt@postgres:5432/docsgpt volumes: - - ../docsgpt/indexes:/app/indexes - - ../docsgpt/inputs:/app/inputs - - ../docsgpt/vectors:/app/vectors + # Host data stays under application/ (the old package directory) for this + # release so an upgraded checkout keeps its indexes; it moves with the + # packaging work, together with an upgrade note. + - ../application/indexes:/app/indexes + - ../application/inputs:/app/inputs + - ../application/vectors:/app/vectors depends_on: redis: condition: service_started diff --git a/docs/content/Deploying/Docker-Deploying.mdx b/docs/content/Deploying/Docker-Deploying.mdx index dfa75e99..00932c35 100644 --- a/docs/content/Deploying/Docker-Deploying.mdx +++ b/docs/content/Deploying/Docker-Deploying.mdx @@ -69,8 +69,8 @@ checkout. ## Using the Source Checkout With a clone of the repository, `deployment/docker-compose-hub.yaml` runs the -same pre-built images while keeping your data in `docsgpt/indexes`, -`docsgpt/inputs` and `docsgpt/vectors`, and `deployment/docker-compose.yaml` +same pre-built images while keeping your data in `application/indexes`, +`application/inputs` and `application/vectors`, and `deployment/docker-compose.yaml` builds the images from your working tree (for local changes, or a build with extra packages: `EXTRAS=docling` in `.env`). diff --git a/docs/content/upgrading.mdx b/docs/content/upgrading.mdx index a122dfcf..4e634525 100644 --- a/docs/content/upgrading.mdx +++ b/docs/content/upgrading.mdx @@ -90,6 +90,26 @@ need attention: Staying on `all-mpnet-base-v2` is a supported choice — it remains in the model registry and in `setup.sh`. You only need this section if you want to move to granite. +## Backend package renamed to `docsgpt` + +The backend's Python package is `docsgpt` (it was `application`), the name it +will carry on PyPI. For one release the old name keeps working through an +alias, so nothing breaks on upgrade, but update these before the alias goes: + +- Entry points: `celery -A docsgpt.app.celery worker`, + `uvicorn docsgpt.asgi:asgi_app`, `python -m docsgpt.scripts.`. The + `application.…` spellings still run and print a `FutureWarning`. The + compose files, Kubernetes manifests and setup scripts in the repository are + already updated; only custom copies need editing. +- Local image builds: the build context is the repository root, so use + `docker build -f docsgpt/Dockerfile .` (or the compose files, which do this). +- Celery task names changed with the package (`docsgpt.api.user.tasks.ingest` + and so on). A worker on this release also accepts the old names, so tasks + queued before the upgrade still run, and beat rewrites the periodic + schedule in Redis on start-up. Nothing to do. +- Data directories do not move: the compose files keep your indexes, inputs + and vectors under `application/` in the checkout, where they already are. + ## Check your version ```bash diff --git a/docsgpt/Dockerfile b/docsgpt/Dockerfile index ace3c1c2..02c36636 100644 --- a/docsgpt/Dockerfile +++ b/docsgpt/Dockerfile @@ -138,7 +138,7 @@ RUN if python -c "import docling" 2>/dev/null; then \ COPY --chown=appuser:appuser docsgpt /app/docsgpt # One-release alias so `-A application.app.celery` style entry points keep working. -COPY --chown=appuser:appuser application /app/application +COPY --chown=appuser:appuser application/__init__.py /app/application/__init__.py # Runtime data directories, owned by the process user so a named volume # mounted on them (docker-compose-standalone.yaml) inherits that ownership diff --git a/docsgpt/api/user/tasks.py b/docsgpt/api/user/tasks.py index 8e822f7b..d4307b33 100644 --- a/docsgpt/api/user/tasks.py +++ b/docsgpt/api/user/tasks.py @@ -558,14 +558,17 @@ def setup_periodic_tasks(sender, **kwargs): sender.add_periodic_task( timedelta(days=1), schedule_syncs.s("daily"), + name="schedule-syncs-daily", ) sender.add_periodic_task( timedelta(weeks=1), schedule_syncs.s("weekly"), + name="schedule-syncs-weekly", ) sender.add_periodic_task( timedelta(days=30), schedule_syncs.s("monthly"), + name="schedule-syncs-monthly", ) # Replaces Mongo's TTL index on pending_tool_state.expires_at. sender.add_periodic_task( diff --git a/docsgpt/celery_init.py b/docsgpt/celery_init.py index eacd101e..02da0711 100644 --- a/docsgpt/celery_init.py +++ b/docsgpt/celery_init.py @@ -176,19 +176,27 @@ def register_legacy_task_names(app: Celery) -> int: """Make every ``docsgpt.*`` task answer to its old ``application.*`` name too. Messages queued by the previous release carry the old names; without the - alias a worker on this release rejects them as unregistered. Kept for one - release, together with the ``application`` import alias. + alias a worker on this release rejects them as unregistered. Each alias is + a distinct task object (a subclass carrying the old name), not the same + object under a second key: Celery builds its execution tracer per task + object, and one object under two names would log every run under + whichever name was traced last. Kept for one release, together with the + ``application`` import alias. Returns: The number of aliases added. """ added = 0 for name, task in list(app.tasks.items()): - if name.startswith("docsgpt."): - legacy = LEGACY_TASK_PREFIX + name[len("docsgpt."):] - if legacy not in app.tasks: - app.tasks[legacy] = task - added += 1 + if not name.startswith("docsgpt."): + continue + legacy = LEGACY_TASK_PREFIX + name[len("docsgpt."):] + if legacy in app.tasks: + continue + base = type(task) + legacy_cls = type(base.__name__, (base,), {"name": legacy, "__module__": base.__module__, "__doc__": base.__doc__}) + app.register_task(legacy_cls()) + added += 1 return added diff --git a/docsgpt/celeryconfig.py b/docsgpt/celeryconfig.py index 25e702aa..ee50bc08 100644 --- a/docsgpt/celeryconfig.py +++ b/docsgpt/celeryconfig.py @@ -50,10 +50,7 @@ task_queues = tuple( beat_scheduler = "redbeat.RedBeatScheduler" redbeat_redis_url = broker_url -# v2: the task names changed with the package rename; a new prefix leaves the -# schedule entries the previous release wrote in Redis unread instead of firing -# the old names alongside the new ones. -redbeat_key_prefix = "redbeat:docsgpt:v2:" +redbeat_key_prefix = "redbeat:docsgpt:" redbeat_lock_timeout = 90 # Survive worker SIGKILL/OOM without silently dropping in-flight tasks. diff --git a/tests/api/user/test_tasks.py b/tests/api/user/test_tasks.py index afc4a18f..a593bbb5 100644 --- a/tests/api/user/test_tasks.py +++ b/tests/api/user/test_tasks.py @@ -278,6 +278,19 @@ class TestIngestConnectorTask: class TestSetupPeriodicTasks: + @pytest.mark.unit + def test_every_entry_has_a_stable_name(self): + """Unnamed entries get keyed by the task path, which redbeat cannot update in place when it changes.""" + from docsgpt.api.user.tasks import setup_periodic_tasks + + sender = MagicMock() + setup_periodic_tasks(sender) + + names = [call.kwargs.get("name") for call in sender.add_periodic_task.call_args_list] + assert all(names), names + assert len(set(names)) == len(names), names + assert names[:3] == ["schedule-syncs-daily", "schedule-syncs-weekly", "schedule-syncs-monthly"] + @pytest.mark.unit def test_registers_periodic_tasks(self): from docsgpt.api.user.tasks import setup_periodic_tasks diff --git a/tests/test_package_rename.py b/tests/test_package_rename.py index 1b02707a..b448a987 100644 --- a/tests/test_package_rename.py +++ b/tests/test_package_rename.py @@ -1,30 +1,59 @@ """The ``application`` alias and the legacy Celery task names survive the rename to ``docsgpt``.""" import importlib +import runpy import sys import warnings import pytest +def _quiet_import(name): + with warnings.catch_warnings(): + warnings.simplefilter("ignore", FutureWarning) + return importlib.import_module(name) + + class TestApplicationAlias: def test_old_import_is_the_same_module_object(self): - with warnings.catch_warnings(): - warnings.simplefilter("ignore", FutureWarning) - old = importlib.import_module("application.vectorstore.model_registry") + old = _quiet_import("application.vectorstore.model_registry") new = importlib.import_module("docsgpt.vectorstore.model_registry") assert old is new assert sys.modules["application.vectorstore.model_registry"] is new def test_old_package_is_the_new_package(self): - with warnings.catch_warnings(): - warnings.simplefilter("ignore", FutureWarning) - import application # noqa: F401 + _quiet_import("application") assert sys.modules["application"] is importlib.import_module("docsgpt") + def test_alias_keeps_the_real_spec(self): + """Importing through the alias must not rename the shared module object.""" + module = _quiet_import("application.vectorstore.model_registry") + assert module.__spec__.name == "docsgpt.vectorstore.model_registry" + assert module.__name__ == "docsgpt.vectorstore.model_registry" + + def test_reload_works_on_the_shared_module(self): + # docsgpt.version has no shared constants, so re-executing it in place is harmless. + _quiet_import("application.version") + module = importlib.import_module("docsgpt.version") + reloaded = importlib.reload(module) + assert reloaded is module + assert reloaded.__spec__.name == "docsgpt.version" + assert reloaded.get_version() == module.__version__ + + def test_python_dash_m_through_the_alias(self): + """``python -m application.x`` goes through runpy, which needs the loader's get_code. + + A real ``python -m`` starts a fresh interpreter, so the target module is + not yet in ``sys.modules`` under the alias name when runpy looks it up; + drop any entry an earlier test left so the lookup reaches the finder. + """ + _quiet_import("application") + sys.modules.pop("application.vectorstore.model_registry", None) + globals_ = runpy.run_module("application.vectorstore.model_registry", run_name="__main__", alter_sys=True) + assert "DEFAULT_NEW_INSTALL" in globals_ + def test_alias_warns_when_first_imported(self): """The shim warns once per process: on the import that executes it.""" - # Undo a previous import so the shim module body runs again. sys.meta_path[:] = [f for f in sys.meta_path if type(f).__name__ != "_AliasFinder"] for name in [m for m in sys.modules if m == "application" or m.startswith("application.")]: del sys.modules[name] @@ -33,26 +62,28 @@ class TestApplicationAlias: assert sys.modules["application"] is importlib.import_module("docsgpt") def test_missing_module_still_raises(self): - with warnings.catch_warnings(): - warnings.simplefilter("ignore", FutureWarning) - with pytest.raises(ModuleNotFoundError): - importlib.import_module("application.no_such_module") + _quiet_import("application") + with pytest.raises(ModuleNotFoundError): + importlib.import_module("application.no_such_module") class TestLegacyTaskNames: - def test_every_task_answers_to_its_old_name(self): + def test_every_task_answers_to_its_old_name_as_its_own_object(self): from docsgpt.celery_init import LEGACY_TASK_PREFIX, celery, register_legacy_task_names importlib.import_module("docsgpt.api.user.tasks") importlib.import_module("docsgpt.vectorstore.embeddings_tasks") - added = register_legacy_task_names(celery) + register_legacy_task_names(celery) new_names = [n for n in celery.tasks if n.startswith("docsgpt.")] assert new_names, "no docsgpt.* tasks registered" for name in new_names: - legacy = LEGACY_TASK_PREFIX + name[len("docsgpt."):] - assert celery.tasks[legacy] is celery.tasks[name] + legacy_name = LEGACY_TASK_PREFIX + name[len("docsgpt."):] + canonical, legacy = celery.tasks[name], celery.tasks[legacy_name] + assert legacy is not canonical, "an alias must be its own task object (its own tracer)" + assert isinstance(legacy, type(canonical)) + assert legacy.name == legacy_name + assert canonical.name == name assert register_legacy_task_names(celery) == 0, "second call must be idempotent" - assert added <= len(new_names) def test_routes_and_reclaim_list_use_new_names(self): from docsgpt import celeryconfig @@ -60,4 +91,4 @@ class TestLegacyTaskNames: assert all(k.startswith("docsgpt.") for k in celeryconfig.task_routes) assert all(k.startswith("docsgpt.") for k in _NO_RECLAIM_TASKS) - assert celeryconfig.redbeat_key_prefix == "redbeat:docsgpt:v2:" + assert celeryconfig.redbeat_key_prefix == "redbeat:docsgpt:", "the prefix must stay: redbeat updates named entries in place"