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 celery configuration not being applied sometimes #223

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

raffomania
Copy link
Contributor

The module dispatching the celery tasks did not import our celery worker module, which resulted in the celery configuration not being applied in some cases, e.g. when using uvicorn's hot reload functionality.

By referencing the celery tasks directly, we make sure that the celery config used for defining the tasks is also used for dispatching them.

See https://docs.celeryq.dev/en/stable/userguide/application.html#breaking-the-chain for more information.

fixes #219

@raffomania raffomania self-assigned this Nov 16, 2023
The module dispatching the celery tasks did not import
our celery worker module, which resulted in the celery configuration not
being applied in some cases, e.g. when using uvicorn's hot reload
functionality.

By referencing the celery tasks directly, we make sure that the celery
config used for defining the tasks is also used for dispatching them.

See https://docs.celeryq.dev/en/stable/userguide/application.html#breaking-the-chain
for more information.
Copy link
Member

@floschne floschne left a comment

Choose a reason for hiding this comment

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

Uhh nice! I didn't think about that this is now possible. The historical reason why we had to use Signature and a string that points to the function was, that we could not import the methods in the backend_api worker process because it would load the ML models thus requiring many dependencies. Now we moved all ML stuff to the ray worker, which make your changes possible.

The only question I have here is, why you import the tasks in the *_async() functions? Shouldn't it be possible to import them at root level? Because if you import them in the functions, the celery related code gets imported at runtime in the backend_api worker. But then again I'm wondering if this is working?

@raffomania
Copy link
Contributor Author

raffomania commented Nov 17, 2023

I put the import statements into the functions because they would otherwise have caused an import cyle:

background_jobs/__init__.py -> background_jobs/tasks.py -> backround_jobs/preprocess.py -> background_jobs/preprocessing_service.py -> background_jobs/__init__.py

I've tested uploading and processing documents with this change and it worked fine.

if you import them in the functions, the celery related code gets imported at runtime in the backend_api worker

I'm not sure which celery-related code you mean. Is the backend_api worker the container running the celery worker? How would you expect this to break?

@floschne
Copy link
Member

I meant the @celery.task decorated methods. With this, the decorated methods get registered with celery and are available in the background_jobs celery worker. Then, to decouple, we used a Signature with a string pointing to the respective methods to call them and the backend_api process, we did not import the files containing the @celery.task methods. But yeah, we chose this strategy mostly because we were loading the ML models in the mentioned methods and files.
IDK, if it works like this, I like it because it's a bit cleaner :)

@raffomania
Copy link
Contributor Author

raffomania commented Nov 20, 2023

To keep the decoupling, we could also just import celery_worker to make sure the configuration is applied, but then we'd have an unused import. Also, as you mentioned, the decoupling is not really necessary anymore.

I've tested this and it works fine, so if you're happy with it I'd say we can merge it :)

@floschne floschne merged commit 3e4eedb into mwp_v1 Nov 21, 2023
4 checks passed
@floschne
Copy link
Member

Alright! Then, let's meeeerge ;)

@floschne floschne deleted the fix-celery-configuration branch February 8, 2024 12:32
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.

Celery configuration is not loaded when using uvicorn with auto-reload
2 participants