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

Proposition pour decoupler les sources des traitements finaux #264

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

hlecuyer
Copy link
Contributor

No description provided.

@hlecuyer hlecuyer changed the title wip Proposition pour decoupler les sources des traitements finaux Jul 29, 2024
@hlecuyer hlecuyer requested a review from vperron July 30, 2024 09:45
@vmttn
Copy link
Contributor

vmttn commented Aug 5, 2024

@vperron les data tests n'ayant lieu qu'après l'exec des models, si il y a une erreur sur l'étape intermediate de dora par exemple, les données invalides contenues dans la table intermediate alimenteront quand même les models en aval. D'où l'idée du nouveau layer qui fait buffer...

autre solution qui ne nécessite pas de modifier la structure des models (et donc plus proche de ce que tu imaginais @hlecuyer en passant via une transaction) : changer de schéma temporairement ?

dans dbt_project.yml :

...

models:
  data_inclusion:

    ...

    intermediate:
      +schema: "{{ 'intermediate_tmp' if var('tmp', false) else 'intermediate' }}"
      +materialized: table

...
  1. build une 1ère fois dans un schéma "temporaire" via une tâche airflow dédiée avec :

dbt build -s models/intermediate/sources/france_travail/ --vars 'tmp: true'

  1. build une seconde fois dans le bon schéma via une autre tâche airflow dépendant de la 1ère

dbt build -s models/intermediate/sources/france_travail/

Si erreur, les données ne se propageront pas

@vperron
Copy link
Contributor

vperron commented Aug 5, 2024

Je suis d'accord @vmttn pour les modèles intermediate, on avait "pas de solution".
Mais mon point était de dire que si on blinde les data tests de staging pour bien valider les données amont qui viennent des sources et peuvent casser hors de notre contrôle, a priori on se protège déjà assez bien et les intermediate vont jouer leur rôle de buffer de données J-1 (si on met en place le ALL_DONE)

Ta suggestion est cool cependant car elle permettra de protéger AUSSI le layer intermediate et ne prend pas bcp de code !

@@ -106,6 +110,7 @@ def get_before_geocoding_tasks():
"path:models/intermediate/int__union_structures.sql",
]
),
trigger_rule=TriggerRule.ALL_DONE,
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, yes, mais du coup ça pose la question de:

  • découpler les before_geocoding_tasks en "tâches sources alternatives" (odspep, monenfant, en faire deux TaskGroup comme les autres ?) et les pousser dans les staging tasks ?
  • conserver les int__union__xxx derrière le trigger ALL_DONE

Je me suis posé la question de savoir si ça fonctionnerait si par exemple pour une source donnée, les tests de staging fail + une intermediate est donc skipped, on a bien une erreur (et non un skip) pour le TaskGroup. Donc le ALL_DONE est bien ce qu'il nous faut.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@hlecuyer
Copy link
Contributor Author

Je vais tester la solution de @vmttn.

@hlecuyer hlecuyer force-pushed the feat/core/decoralate-intermediate-states branch 2 times, most recently from 2dbc94d to 39a9000 Compare August 21, 2024 15:27
@hlecuyer hlecuyer marked this pull request as ready for review August 22, 2024 13:32
@hlecuyer hlecuyer requested a review from vmttn as a code owner August 22, 2024 13:32
@hlecuyer hlecuyer requested a review from vperron August 22, 2024 13:32
@hlecuyer hlecuyer merged commit 78a7747 into main Aug 28, 2024
8 of 9 checks passed
@hlecuyer hlecuyer deleted the feat/core/decoralate-intermediate-states branch August 28, 2024 09:34
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