Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Celery: upgrade to latest version #8952

Merged
merged 9 commits into from
Mar 1, 2022
Merged

Celery: upgrade to latest version #8952

merged 9 commits into from
Mar 1, 2022

Conversation

humitos
Copy link
Member

@humitos humitos commented Feb 21, 2022

  • upgrade celery to 5.2.3
  • before_start is now a method recognized by this version
  • migrate some class-based tasks to function-based tasks to avoid bug when registering them

See celery/celery#7172 (comment)

@humitos humitos requested a review from a team as a code owner February 21, 2022 18:16
@humitos
Copy link
Member Author

humitos commented Feb 21, 2022

I wasn't able to register class-based tasks properly with Celery 5.2.3. In fact, I'm thinking the usage of class-based tasks are de-motivated and they should only be used only when some particular task behavior needs to be overwritten: https://docs.celeryproject.org/en/stable/userguide/tasks.html#custom-task-classes

So, probably a better approach here would be to migrate class-based task to function-based for most of our tasks that don't require particular behavior.

In the meanwhile, I asked here celery/celery#7172 (reply in thread) if there is a way to register class-based task with Celery 5.2.3 and see if we ca workaround this issue for now.

@agjohnson
Copy link
Contributor

Huh, well glad you checked before we put this on this next release.

It seems like that would be an odd change, the tasks are still classes underneath, regardless of how they are defined in code. Hopefully this doesn't affect us much and there is an easy solution as the task handler pattern is a far nicer implementation.

@humitos
Copy link
Member Author

humitos commented Feb 22, 2022

It seems like that would be an odd change, the tasks are still classes underneath, regardless of how they are defined in code.

Yeah, Celery explains what happens under the hood when you create a function-based task: https://docs.celeryproject.org/en/master/userguide/tasks.html#custom-task-classes

However, I don't think using class-based task is not supported anymore, but it seems that "registering a class-based task is broken in some fashion".

Hopefully this doesn't affect us much and there is an easy solution as the task handler pattern is a far nicer implementation.

Task handlers are not affected, even if we migrate them to function-based class. In fact, we are currently using function-based tasks with task handlers when using a class as a base= argument when defining the task. See


So, the work required here would be to migrate the class-based task into function-based tasks in all our codebase. This is:

@humitos
Copy link
Member Author

humitos commented Feb 22, 2022

I QAed this locally and it seems to work properly. Hopefully, it does not break in production 😄

readthedocs/settings/base.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Feb 23, 2022

Hrm... It seems that before_start (and other handlers) are not being called when triggering the task from tests. I'd assume this is because it's not supported when using ALWAYS_EAGER?

@humitos humitos requested a review from a team February 23, 2022 17:11
@humitos
Copy link
Member Author

humitos commented Feb 23, 2022

It seems that before_start (and other handlers) are not being called when triggering the task from tests

I was wrong. I was not installing the latest version of Celery locally 😓

I QAed this locally and it seems it works properly with this PR applied. I don't think we should experiment with any issue in production as far as I can tell.

@humitos humitos merged commit b220bc6 into master Mar 1, 2022
@humitos humitos deleted the humitos/celery-upgrade branch March 1, 2022 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants