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

Enregistrement des dataframes entre les tâches en table dans une DB spécifique (vs xcom) #1139

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kolok
Copy link
Contributor

@kolok kolok commented Dec 16, 2024

Description succincte du problème résolu

⚠️ Tâche très embroyonnaire, pas du tout prète pour ne review, même partielle

Arrêter de passer par la table XCOM pour transférer les dataframe entre les tâches:

  • les xcom ne s'occupe que des métadonnées
  • les dataframes sont enregistrés en base de données :
    • création d'une base de données spécifiques pour ne pas interférer avec les données de django lors de calculs en DB
    • les tables sont nommées en utilisant les ID de dag, run et tache
    • les tables sont nettoyée régulièrement

A discuter :

  • Utilisation du même serveur de base de données que la base de données Django
  • Nommage de table
  • politique de nettoyage

Tache à suivre :

  • Transférer les calculs sur la base de données quand c'est possible

Type de changement :

  • Bug fix
  • Nouvelle fonctionnalité
  • Mise à jour de données / DAG
  • Les changements nécessitent une mise à jour de documentation
  • Refactoring de code (explication à retrouver dans la description)

Auto-review

Les trucs à faire avant de demander une review :

  • J'ai bien relu mon code
  • La CI passe bien
  • En cas d'ajout de variable d'environnement, j'ai bien mis à jour le .env.template
  • J'ai ajouté des tests qui couvrent le nouveau code

Comment tester

En local / staging :

@kolok kolok requested a review from a team as a code owner December 16, 2024 07:22
@kolok kolok requested review from fabienheureux and maxcorbeau and removed request for a team December 16, 2024 07:22
@kolok kolok marked this pull request as draft December 16, 2024 07:22
@kolok kolok force-pushed the create_final_actor_reorg branch 2 times, most recently from d2f9675 to bf0f14a Compare December 19, 2024 11:53
Base automatically changed from create_final_actor_reorg to main December 19, 2024 11:57
@maxcorbeau
Copy link
Contributor

Contexte: problème soulevé lors de mon audit d'arrivé via Etat de fonctionnement des DAGs au 2024-11-04

image

Et en comparant la taille DB avant/après avec la requête suivante:

SELECT
	DATE(timestamp) AS date,
	COUNT(*) AS nombre_entrees,
	ROUND(SUM(pg_column_size(value) / 1024.0 / 1024.0)) AS taille_mb
FROM xcom
GROUP BY 1
ORDER BY 1 DESC

On voit la taille de la table xcom qui explose (178MB de plus le 2024-11-04 avec seulement 18 DAGs lancées):

image

Copy link
Contributor

@maxcorbeau maxcorbeau left a comment

Choose a reason for hiding this comment

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

La direction me semble bonne:

  • On créer un Engine/Variable DB séparés: après libre au gestionnaire d'infra de décider si cela doit être sur une DB logiquement/physiquement séparée, le code donne la flexibilité
  • On créer des fonctions d'écriture/lecture pour sauvegarder/récupérer les données entres les tâches

Et donc pour la suite je m'attends à ce que:

  • Les appels XCOM (push, pull) ne contiennent que des pointeurs vers les données, pointeurs fournis (xcom_push) par les tâches en amonts après l'insertion des données, et utilisés par les tâche en aval (xcom_pull) pour aller récupérer les données

J'ai laissé des commentaires sur le nommage car je le trouve en conflit avec la logique, ou alors j'ai pas compris la logique 😄

self.django_conn_id = django_conn_id
self.data_conn_id = data_conn_id
self.django_engine = self._create_engine(self.django_conn_id)
self.data_engine = self._create_engine(self.data_conn_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi pas un nommage plus hiérarchique avec:

  • conn_id_django
  • conn_id_data
  • engine_django
  • engine_data

table_name = _table_name(dag_id, dag_run_id, task_id, dataset_name)
df.to_sql(table_name, self.data_engine, if_exists="replace", index=False)

def read_data_xcom(
Copy link
Contributor

@maxcorbeau maxcorbeau Jan 22, 2025

Choose a reason for hiding this comment

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

Je suis confu par le nommage des fonctions: j'ai l'impression que le but est justement de NE PLUS échanger de données par le XCOM (simplement les pointeurs vers ces données) et mais le nom des fonctions indique clairement l'inverse: write_data_xcom et read_data_xcom.

def _table_name(dag_id: str, dag_run_id: str, task_id: str, dataset_name: str):
# dag_run_id remove str before __
dag_run_id = dag_run_id.split("__")[1]
timestamp = datetime.strptime(dag_run_id, "%Y-%m-%dT%H:%M:%S.%f%z")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
timestamp = datetime.strptime(dag_run_id, "%Y-%m-%dT%H:%M:%S.%f%z")
timestamp = datetime.fromisoformat(dag_run_id)

ça fonctionne ça ?

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.

3 participants