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

fix: run Celery tasks asynchronously in dev mode #928

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kdmccormick
Copy link
Collaborator

In tutor dev, Celery tasks were running eagerly (in-process) rather than being delegated to the worker containers for asynchronous processing. This difference in behavior between tutor dev and tutor local meant that certain code paths and race-condition bugs would not surface when testing in dev.

The cause is that CELERY_ALWAYS_EAGER is set to True in the upstream devstack settings file. That's because Devstack does not support async LMS/CMS workers; it runs all those tasks in-process.

The fix is to simply override CELERY_ALWAYS_EAGER to False. We do this in the common settings file for simplicity, but the change should not affect production mode, which has always had CELERY_ALWAYS_EAGER equal False.

For unit tests, we override CELERY_ALAYS_EAGER back to True, because many of them are written to assume in-process tasks.

In `tutor dev`, Celery tasks were running eagerly (in-process) rather than being delegated to the worker containers for
asynchronous processing. This difference in behavior between
`tutor dev` and `tutor local` meant that certain code paths
and race-condition bugs would not surface when testing in dev.

The cause is that CELERY_ALWAYS_EAGER is set to True in the
upstream devstack settings file. That's because Devstack does
not support async LMS/CMS workers; it runs all those tasks
in-process.

The fix is to simply override CELERY_ALWAYS_EAGER to False.
We do this in the common settings file for simplicity, but the
change should not affect production mode, which has always
had CELERY_ALWAYS_EAGER equal False.

For unit tests, we override CELERY_ALAYS_EAGER back to True,
because many of them are written to assume in-process tasks.
@kdmccormick
Copy link
Collaborator Author

I needed this fix in order to test the upcoming Content Libraries V2 changes, which moves all of the update-from-library operations into Celery tasks.

FYI @ormsbee.

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a major change. I'm assuming you tested this and it works, right? It will make troubleshooting celery tasks more difficult for developers (no pdb for them). But on the other hand it will get us closer to production. If you think that this will be an overall improvement for developer experience, then please merge.

@kdmccormick
Copy link
Collaborator Author

It will make troubleshooting celery tasks more difficult for developers (no pdb for them).

@regisb Let me check this out. I think devs should be able to set breakpoints in Celery tasks and then interact with them using tutor dev start [lms|cms]-worker, but I haven't confirmed that yet. As I mentioned in Slack, I've been having trouble with breakpoints in general recently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants