launchpad-reviewers team mailing list archive

[Colin Watson <mp+418279@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:celery-bionic into launchpad:master.

Commit message:
Work around Celery job failures on Python 3.6

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/418279

The slightly odd hack that we had to integrate historical class-based tasks with Celery's modern preferences (see e.g. https://docs.celeryq.dev/en/stable/history/whatsnew-4.0.html#the-task-base-class-no-longer-automatically-register-tasks) no longer worked with Python 3.6.

`RunJob` and `CeleryRunJob` are partly "overriding general behaviour", which makes sense to do in a subclass of `Task`, but they also implement their own `run` methods.  `celery_app.task` creates a new `Task` instance using the decorated function as its `run` method, and so persuading those to use `CeleryRunJob.run` instead took some effort.  However, with Python 3.6 this fails because the running task instance doesn't seem to be an instance of `CeleryRunJob` according to `super()`.

Switching to `celery_app.register_task` avoids some unnecessary complexity, but the core problem remains.  For now, I've just switched to the old-style way of calling the superclass using `RunJob.run(self, job_id)`; inelegant though it is, it works on both Python 3.5 and 3.6.

I suspect that eventually we'll need to rethink how `lazr.jobrunner`'s Celery integration works based on modern Celery best practices in order to fix this properly.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:celery-bionic into launchpad:master.

diff --git a/lib/lp/services/job/celeryjob.py b/lib/lp/services/job/celeryjob.py
index fcc1c5f..f1ed921 100644
--- a/lib/lp/services/job/celeryjob.py
+++ b/lib/lp/services/job/celeryjob.py
@@ -49,7 +49,7 @@ celery_app = Celery()
 
 
 class CeleryRunJob(RunJob):
-    """The Celery Task that runs a job."""
+    """A Celery task that runs a job."""
 
     job_source = UniversalJobSource
 
@@ -67,20 +67,27 @@ class CeleryRunJob(RunJob):
         """
         self.dbuser = dbuser
         task_init(dbuser)
-        super().run(job_id)
+        # XXX cjwatson 2022-04-01: We'd normally use `super()` here, but
+        # that fails on Python 3.6 (and newer?) because running tasks don't
+        # seem to be instances of `CeleryRunJob`; it's not entirely clear
+        # why not.  Celery tends to steer users away from class-based tasks
+        # these days, so the right answer may be to rethink the interfaces
+        # provided by `lazr.jobrunner`.
+        RunJob.run(self, job_id)
 
     def reQueue(self, job_id, fallback_queue):
         self.apply_async(args=(job_id, self.dbuser), queue=fallback_queue)
 
 
-@celery_app.task(base=CeleryRunJob, bind=True)
-def celery_run_job(self, job_id, dbuser):
-    super(type(self), self).run(job_id, dbuser)
+class CeleryRunJobIgnoreResult(CeleryRunJob):
+    """A Celery task that runs a job and ignores its result."""
 
+    ignore_result = True
 
-@celery_app.task(base=CeleryRunJob, bind=True, ignore_result=True)
-def celery_run_job_ignore_result(self, job_id, dbuser):
-    super(type(self), self).run(job_id, dbuser)
+
+celery_run_job = celery_app.register_task(CeleryRunJob())
+celery_run_job_ignore_result = celery_app.register_task(
+    CeleryRunJobIgnoreResult())
 
 
 class FindMissingReady: