(PC-34345)[API] setup celery tasks for mails #15838

lgerard-pass · 2025-01-13T16:33:13Z

But de la pull request

Utilisation de celery pour lancer les tâches asynchrones liées au mail (sous feature flag).

Voici la configuration utilisée (qui pourrait être changée)

acks_late et task_reject_on_worker_lost mis par défaut pour éviter de perdre la tâche si le worker est tué (⚠️ cela peut faire que la tâche se joue 2 fois).

retry sur un type d'erreur en particulier, sur les autres erreurs la tâche est acquitée.

Vérifications

J'ai écrit les tests nécessaires
J'ai fait la revue fonctionnelle de mon ticket

github-actions · 2025-01-13T16:35:39Z

Visit the preview URL for this PR (updated for commit 0460368):

https://pc-pro-testing--pr15838-bsr-setup-celery-tas-5hnhk4qu.web.app

_{(expires Sat, 01 Feb 2025 16:43:17 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 032d233ee67e1c50d6af12e29c936c7076770eb1}

jcicurel-pass

My 2 cents sur cette nice PR 👌 , je ne connais pas très bien le fonctionnement de l'asynchrone actuel

api/pyproject.toml

jcicurel-pass · 2025-01-30T16:05:53Z

api/src/pcapi/celery_tasks/celery.py

+            with app.app_context():
+                return self.run(*args, **kwargs)


Pour l'instant c'est seulement les mails donc pas d'update en DB mais ce serait p-e pas mal d'avoir atomic à terme (ça risque de coincer au moment où on voudra passer une tâche sur celery, ou alors on peut se dire que ce sera l'occasion de la rendre atomic-compatible)

Oui c'est vrai que du coup j'ai pas traité cette problématique pour le moment. Je vais voir pour le rajouter.

api/src/pcapi/core/mails/transactional/send_transactional_email.py

jcicurel-pass · 2025-01-30T16:07:50Z

api/src/pcapi/flask_app.py

+        task_reject_on_worker_lost=True,
+        # Pickle seems the best pick since we don't support
+        # anything other than python https://docs.celeryq.dev/en/latest/userguide/calling.html#serializers
+        task_serializer="pickle",


Je ne sais plus vraiment pour quelles raisons mais.. il me semble avoir entendu des arguments pour le JSON en format... 🤔 ça peut se logger notamment ?

La serialisation JSON ne fonctionnait pas, surement car certain champs n'était pas serialisables. Du coup je suis aller chercher l'info ici :

https://docs.celeryq.dev/en/latest/userguide/calling.html#serializers

En lisant les avantages et inconvénients, pickle me paraissait pertinent puisqu'on est que dans du python. Ceci dit je peux tenter de le faire fonctionner avec JSON si besoin.

En fait ouais le JSON te contraint un peu sur les champs que tu envoies

@xordoquy nous avait mis en garde sur ce sujet, en recommandant plutôt une sérialisation en JSON.

La sérialisation en pickle permet de transmettre des objets Python complexes, mais elle entraînait apparemment des bugs très étranges. Par exemple une connexion à la base de données ou des état internes d'objets pouvaient être envoyés aux workers Celery.

ok je vais regarder si j'arrive à le faire fonctionner avec du JSON

C'est fait je viens de pousser une version qui fonctionne avec le serialiser JSON, j'ai rajouté les try/except au cas ou le format ne soit pas le bon

pro/src/apiClient/v1/services/DefaultService.ts

api/pyproject.toml

api/src/pcapi/core/mails/transactional/send_transactional_email.py

jcicurel-pass · 2025-02-06T15:42:53Z

api/src/pcapi/core/external/sendinblue.py

    else:
-        update_contact_attributes_task(contact_request)
+        if FeatureToggle.WIP_ASYNCHRONOUS_CELERY_TASKS.is_active():
+            update_contact_attributes_task_celery(contact_request.dict())


Il y a un inconvénient au JSON que je n'avais pas en tête, on perd ici le typing au passage (d'ailleurs la signature de update_contact_attributes_task_celery ne doit plus être bonne pour le type de payload)

Il faudrait des TypedDict sinon mais.. ça ferait doublon avec le modèle Pydantic dans ce genre de cas..

Après y a p-e des cas où on aura juste plus besoin du modèle Pydantic et le TypedDict suffira

Bah là effectivement je suis reparti tu modèle Pydantic pour coller le plus possible à l'existant.
Donc effectivement on perd le typing lors du check statique mais on vérifie quand même au runtime. Je voyais pas comment faire mieux sans faire de doublons.

J'ai pas en tête beaucoup de cas de nos tâches async

Mais en fait si on a des schemas Pydantic définis seulement pour les passer à des tâches, en faisant le switch vers Celery / JSON ça me parait pas déconnant de switch en même temps les schemas vers des TypedDict

Par contre si on a des cas de schemas Pydantic utilisés par des tâches et par autre chose.. là c'est plus embêtant

Mais bon du coup tant que c'est un WIP sous FF on peut sans doute laisser comme ça (vu qu'on va pas virer le Pydantic maintenant de toute façon)

api/src/pcapi/flask_app.py

lgerard-pass force-pushed the BSR-setup-celery-tasks branch 3 times, most recently from c4144f0 to 90f2de2 Compare January 16, 2025 16:19

lgerard-pass force-pushed the BSR-setup-celery-tasks branch 3 times, most recently from 6e90927 to c0899e4 Compare January 30, 2025 13:20

lgerard-pass force-pushed the BSR-setup-celery-tasks branch 4 times, most recently from b7bed9c to 727b26c Compare January 30, 2025 14:23

lgerard-pass self-assigned this Jan 30, 2025

lgerard-pass marked this pull request as ready for review January 30, 2025 14:40

lgerard-pass force-pushed the BSR-setup-celery-tasks branch from 727b26c to 47c3c0a Compare January 30, 2025 14:50

lgerard-pass had a problem deploying to pullrequest January 30, 2025 16:05 — with GitHub Actions Error

jcicurel-pass reviewed Jan 30, 2025

View reviewed changes

lgerard-pass changed the title ~~Bsr setup celery tasks~~ (PC-34345)[API] setup celery tasks for mails Jan 30, 2025

lgerard-pass had a problem deploying to pullrequest January 30, 2025 16:51 — with GitHub Actions Failure

jcicurel-pass reviewed Jan 30, 2025

View reviewed changes

api/pyproject.toml Outdated Show resolved Hide resolved

api/pyproject.toml Outdated Show resolved Hide resolved

api/src/pcapi/core/mails/transactional/send_transactional_email.py Show resolved Hide resolved

lgerard-pass force-pushed the BSR-setup-celery-tasks branch 3 times, most recently from 91eb8ef to 5e32e0d Compare February 6, 2025 15:14

lgerard-pass requested a review from mgeoffray-pass February 6, 2025 15:30

jcicurel-pass reviewed Feb 6, 2025

View reviewed changes

api/src/pcapi/flask_app.py Show resolved Hide resolved

lgerard-pass force-pushed the BSR-setup-celery-tasks branch from 5e32e0d to fe17999 Compare February 6, 2025 16:39

lgerard-pass added 4 commits February 7, 2025 12:34

(BSR)[API] feat: use celery for sending mails

fb4b7ab

(BSR)[API] feat: gate celery usage behind FF

a8806f6

(BSR)[API] chore: local setup for celery

1913349

(BSR)[API] chore: sort dependencies

a42545b

(PC-34345)[API] feat: use json serialization for celery tasks

39de3e6

lgerard-pass force-pushed the BSR-setup-celery-tasks branch from fe17999 to 39de3e6 Compare February 7, 2025 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PC-34345)[API] setup celery tasks for mails #15838

(PC-34345)[API] setup celery tasks for mails #15838

lgerard-pass commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

jcicurel-pass left a comment

jcicurel-pass Jan 30, 2025 •

edited

Loading

lgerard-pass Jan 30, 2025

jcicurel-pass Jan 30, 2025

lgerard-pass Jan 30, 2025

jcicurel-pass Jan 30, 2025

mgeoffray-pass Feb 5, 2025

lgerard-pass Feb 6, 2025

lgerard-pass Feb 6, 2025

jcicurel-pass Feb 6, 2025

jcicurel-pass Feb 6, 2025

lgerard-pass Feb 6, 2025

jcicurel-pass Feb 6, 2025

(PC-34345)[API] setup celery tasks for mails #15838

Are you sure you want to change the base?

(PC-34345)[API] setup celery tasks for mails #15838

Conversation

lgerard-pass commented Jan 13, 2025 • edited Loading

But de la pull request

Vérifications

github-actions bot commented Jan 13, 2025 • edited Loading

jcicurel-pass left a comment

Choose a reason for hiding this comment

jcicurel-pass Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgerard-pass commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

jcicurel-pass Jan 30, 2025 •

edited

Loading