SEP-1551: Derive Celery include and beat seed prefixes from the app registry - #1121
Conversation
Apps declare their Celery module via App.celery_module_path, a three-state field mirroring api_router_path (auto-derive <module>.celery from a filesystem probe, explicit string override, or explicit null opt-out). The worker include list and beat task_name prefixes both derive from that single source, so a module rename can no longer desync the include from the seed. - set_include seeds only the static service base (app.tasks.celery); the registry-derived app modules are composed at a safe seam (build_celery_include, called from Celery-app assembly and worker start) because reaching the registry during core Settings construction re-enters the half-built lazy proxy. - An explicit celery_module_path override is filesystem-probed at settings construction so a typo fails at load rather than at worker import. - A system periodic-task schedule is emitted only when its owning app contributes a Celery module, so an absent or opted-out app yields no schedule instead of a None-prefixed task_name pointing at nothing the worker registers. No database migration: beat rows re-seed by their stable name.
There was a problem hiding this comment.
Pull request overview
This PR centralizes SEP’s Celery module discovery by introducing App.celery_module_path and deriving both the worker include list and SEP beat seed task_name prefixes from the same app-registry-derived source, preventing drift when app task modules move/rename.
Changes:
- Add
App.celery_module_path(three-state: omitted/explicit string/explicitnull) with filesystem-based validation during settings construction. - Introduce registry helpers (
app_celery_module_paths,app_celery_module_for,build_celery_include) and plumb them into Celery app assembly, worker bootstrap, and SEP beat seeding (including “no celery module → no schedule” gating). - Add/extend tests covering derivation, gating behavior, and invariants; add a changelog fragment.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
app/core/celery/config.py |
Introduces STATIC_CELERY_INCLUDE and narrows CeleryOptions.set_include to seed the static base only. |
app/celery.py |
Composes the final settings.CELERY.include via build_celery_include() at Celery app assembly time. |
app/main.py |
Updates worker bootstrap to use build_celery_include() instead of a hardcoded include list. |
app/sep/config.py |
Adds App.celery_module_path plus convention-based defaulting and filesystem existence validation. |
app/sep/apps/framework/registry.py |
Exposes ordered app celery module paths and provides build_celery_include() for shared composition. |
app/sep/db/seed.py |
Seeds app-owned periodic schedules only when the owning app contributes a Celery module path; refactors report schedules into a helper. |
tests/app/sep/test_config.py |
Adds coverage for the celery_module_path three-state behavior, including override validation and opt-out. |
tests/app/sep/apps/framework/test_registry.py |
Adds tests for registry-derived celery module lists and include composition. |
tests/app/sep/db/test_seed.py |
Adds tests ensuring schedules are omitted when a plugin is absent or opts out of Celery. |
tests/app/sep/test_celery_registration.py |
Extends invariant tests to assert include/seed prefixes derive from the registry. |
changelog.d/SEP-1551.changed.md |
Documents the behavioral change for Celery include list and seed prefix derivation. |
Address PR review: build_celery_include() now dedupes while preserving first-seen order so an app module colliding with the static base is dropped. Correct the test docstring to reference App.celery_module_path.
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
yyyyyyyan
left a comment
There was a problem hiding this comment.
@marcuscruz-percona — solid refactor. build_celery_include gives the include list one derivation that both app/celery.py and start_celery_worker compose, the test_configured_include_is_the_derived_single_source / test_app_owned_seed_prefixes_track_registry_modules invariants pin the seed↔include↔registration contract structurally, and the new App.celery_module_path three-state field faithfully mirrors api_router_path — the explicit-override branch even adds a fail-fast filesystem probe the router field defers to startup. The circular-re-entry deviation is clearly flagged in the description and the reasoning holds: nothing reads settings.CELERY.include before app.celery reassigns it. Validator ordering, path arithmetic, and the None-prefixed-task_name guard rails all check out.
Three Minor, non-blocking notes inline:
AppRegistry.celery_module_pathslooks unused by production — every consumer calls the standalone helpers, never the registry property.- The on-disk module-existence probe is duplicated verbatim between the two
config.pyvalidators. - The
_alerts_plugin_enabled/_report_plugin_enabledgates in the seed are redundant withapp_celery_module_for(...)and inconsistent with the snippets path.
Nothing blocking — approving. Happy to re-review if you rework any of these.
… stale seed flags - Extract _module_or_package_exists helper shared by both config validators - Drop unused AppRegistry.celery_module_paths property/param; standalone app_celery_module_paths remains the single source - Gate alerts/report seed schedules on app_celery_module_for alone, removing import-time plugin-enabled flags that could go stale on runtime APPS changes
… stale seed flags - Extract _module_or_package_exists helper shared by both config validators - Drop unused AppRegistry.celery_module_paths property/param; standalone app_celery_module_paths remains the single source - Gate alerts/report seed schedules on app_celery_module_for alone, removing import-time plugin-enabled flags that could go stale on runtime APPS changes
Summary
App.celery_module_path, a three-state field mirroringapi_router_path(auto-derive<module>.celeryfrom a filesystem probe, explicit string override, or explicitnullopt-out); the workerincludelist and beattask_nameprefixes both derive from that single source, so a module rename can no longer desync the include from the seed.AppRegistryexposes the ordered app-owned Celery module paths;build_celery_includecomposesstatic base + registry-derived app modulesand is the one seam both the Celery-app assembly and the worker bootstrap call.None-prefixedtask_name; an explicit override is filesystem-probed at settings construction so a typo fails at load, not at worker import.Deviation from AC (flagged):
CeleryOptions.set_includeseeds only the static base rather than composing the app modules itself. Reaching the app registry during coreSettingsconstruction forcessep_settings, which reads core settings back through the same un-guarded lazy proxy and re-enters a half-builtSettings. Composition therefore happens at a safe seam (build_celery_include) after core settings is fully built; nothing consumes Celery config beforeapp.celeryis imported, which is where composition runs.Tested
settings.CELERY.include == build_celery_include() == celery.conf.include==['app.tasks.celery', 'app.sep.apps.snippets.celery', 'app.sep.apps.alerts.celery', 'app.sep.apps.report.celery'].make checkmigrationsclean (no migration; beat rows re-seed by stablename).Checklist
make test) — ran targeted regression (160 passed), not the full suitemake run-pre-commit)make makemigrations) — N/A, no model changeApp.celery_module_pathdocumented in the field docstringchangelog.d/