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

Améliorations de l'importer de déclarations historiques #1478

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

pletelli
Copy link
Collaborator

@pletelli pletelli commented Jan 17, 2025

L'importer avait quelques soucis :

  • erreurs non catchées
  • performances bof

Cette PR améliore un peu les perf mais la création des Déclarations historiques met ~ 1h (j'estime ~3h pour la création de toutes les déclarations historiques mêmes pour les entreprises non connues).

Il est possible d'accélérer ce temps en utilisant bulk_create mais cela ne permet pas l'overwrite des champs modification_date et creation_date

TODO : creuser si une autre solution est possible.

+    bulk = []
     # Parcourir tous les compléments alimentaires dont l'entreprise déclarante a été matchée
-    for ica_complement_alimentaire in IcaComplementAlimentaire.objects.filter(
-        etab_id__in=Company.objects.values_list("siccrf_id", flat=True)
+    for index, ica_complement_alimentaire in enumerate(
+        IcaComplementAlimentaire.objects.filter(etab_id__in=Company.objects.values_list("siccrf_id", flat=True))
     ):
         # retrouve la déclaration la plus à jour correspondant à ce complément alimentaire
         all_ica_declarations = IcaDeclaration.objects.filter(cplalim_id=ica_complement_alimentaire.cplalim_ident)
@@ -262,13 +261,14 @@ def create_declaration_from_teleicare_history():
                     if latest_ica_declaration.dcl_date_fin_commercialisation
                     else DECLARATION_STATUS_MAPPING[latest_ica_version_declaration.stattdcl_ident],
                 )
-
-                try:
-                    with suppress_autotime(declaration, ["creation_date", "modification_date"]):
-                        declaration.save()
-                        nb_created_declarations += 1
-                except IntegrityError:
-                    # cette Déclaration a déjà été créée
-                    pass
-
+                bulk.append(declaration)
+                if index % 500 == 0:
+                    with suppress_autotime(Declaration, ["creation_date", "modification_date"]):
+                        Declaration.objects.bulk_create(bulk, ignore_conflicts=True)
+                        logger.info(f"Bulk saving {len(bulk)} déclaration")
+                        nb_created_declarations += len(bulk)
+                        bulk = []
+    Declaration.objects.bulk_create(bulk, ignore_conflicts=True)
+    logger.info(f"Bulk saving {len(bulk)} déclaration")
+    nb_created_declarations += len(bulk)

@pletelli pletelli force-pushed the declaration-import-improvements branch from ff6f247 to 89310ab Compare January 17, 2025 15:05
@pletelli pletelli requested review from hfroot and alemangui January 20, 2025 10:32
@hfroot
Copy link
Collaborator

hfroot commented Jan 21, 2025

possibilité pour modifier les dates et utiliser bulk_create: https://stackoverflow.com/questions/70085655/django-how-to-over-ride-created-date
Difference entre auto_now et default: https://stackoverflow.com/questions/59074688/difference-between-auto-now-add-and-timezone-now-as-default-value

ça nécessite alors un changement du modèle, faut réflechir si il y a d'autre consequences de faire ça

il y a combien de déclarations ? je crois que c'est aussi assez normale que ça prend du temps. C'est quoi les soucis d'un temps long ?

D'ailleurs, chez ma cantine ils ont fait un changement avec redis côté clever cloud qui a aidé à réduire le RAM utilisé. P-e on peut faire la même chose avec complalim avant l'import pour ne pas mettre de la pression sur le RAM

logger.error(
f"Ce IcaComplementAlimentaire cplalim_ident={ica_complement_alimentaire.cplalim_ident} n'a pas une unique déclaration la plus récente"
)
continue
# retrouve la version de déclaration la plus à jour correspondant à cette déclaration
latest_ica_version_declaration = IcaVersionDeclaration.objects.filter(
dcl_id=latest_ica_declaration.dcl_ident,
Copy link
Collaborator

Choose a reason for hiding this comment

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

qu'est-ce que ce passe si on avait MultipleObjectsReturned ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alors la Déclaration n'est pas importée, en tout cas pas pour l'instant.
Il y a environ 1000 déclarations pour lesquelles c'est le cas (qui ne sont pas importées), il faut que je prenne plus de temps pour creuser comment les importer dans ce cas.
C'est plutôt un TODO

company=company, # resp étiquetage, resp commercialisation
company=Company.objects.get(
siccrf_id=ica_complement_alimentaire.etab_id
), # resp étiquetage, resp commercialisation
Copy link
Collaborator

Choose a reason for hiding this comment

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

l'erreur originelle n'est plus un pb ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ce n'est plus possible de rencontrer l'erreur Company.DoesNotExist parce que ligne 191 on filtre sur les déclarations dont la company existe forcément

    for ica_complement_alimentaire in IcaComplementAlimentaire.objects.filter(
        etab_id__in=Company.objects.values_list("siccrf_id", flat=True)
    ):

@alemangui
Copy link
Collaborator

alemangui commented Jan 23, 2025

@hfroot je ne changerais pas le auto_now en default pour l'instant, en tout cas pas pour cette fonctionnalité car c'est qqchose qu'on fera seulement une fois. S'il y avait un import régulier des objets avec des timestamps gérés ailleurs je serai plus de l'avis de passer à default, mais téléicare sera 💀 bientôt. Je trouve donc plus intéressant de garder le côté immuable de auto_add

@pletelli
Copy link
Collaborator Author

possibilité pour modifier les dates et utiliser bulk_create: https://stackoverflow.com/questions/70085655/django-how-to-over-ride-created-date Difference entre auto_now et default: https://stackoverflow.com/questions/59074688/difference-between-auto-now-add-and-timezone-now-as-default-value

ça nécessite alors un changement du modèle, faut réflechir si il y a d'autre consequences de faire ça

il y a combien de déclarations ? je crois que c'est aussi assez normale que ça prend du temps. C'est quoi les soucis d'un temps long ?

D'ailleurs, chez ma cantine ils ont fait un changement avec redis côté clever cloud qui a aidé à réduire le RAM utilisé. P-e on peut faire la même chose avec complalim avant l'import pour ne pas mettre de la pression sur le RAM

Yes j'avais vu cette possibilité de mettre un default plutôt qu'auto_now mais en effet, je préférais ne pas changer le modèle pour ça.
Je merge alors :) Merci pour vos reviews à tous les 2.

@pletelli pletelli merged commit 4307626 into staging Jan 23, 2025
5 checks passed
@pletelli pletelli deleted the declaration-import-improvements branch January 23, 2025 09:25
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