WIP : Jimmyd/impact retour emploi by JimmyDore · Pull Request #384 · StartupsPoleEmploi/labonneboite

JimmyDore · 2019-07-05T08:34:22Z

Ajout des scripts réalisés par Joris, Refacto et industrialisation de ces scripts,
afin de calculer l'impact sur le retour à l'emploi de LBB.

celine-m-s

De manière générale :

c'est vraiment dommage qu'il n'y ait aucune docstring, au moins en haut au niveau du module... Un nouvel arrivant sur le projet aura bien du mal à comprendre ce que fait chaque script.
les fonctions sont bien trop grandes à mon goût et ont trop de responsabilités. Les fractionner en petites fonctions qui font moins de choses aiderait à comprendre plus rapidement chaque script (en lisant la fonction principale on devrait comprendre tout de suite chaque étape sans entrer dans le détail).
il n'y a aucun test !!! => 😱 Avant d'envisager un refacto il faut surtout faire des tests unitaires à mon humble avis !
les requêtes en DB en écriture n'ont pas de rollback en cas de souci. J'imagine que tu as dû tester les cas d'erreur mais ça me semble assez dangereux en l'état, non ?
l'historique git contient pas mal de wip, ça ne me semble pas très lisible.

Et sinon, bravo pour le boulot !! 👏

celine-m-s · 2019-07-15T07:53:37Z

    return con, cur

+def create_sqlalchemy_engine():
+    connexion_url = ('mysql://'+DATABASE['USER']+':%s@'+\


Je trouve que ce serait plus lisible d'enlever les "+" et de passer par format. Exemple : f"mysql://{DATABASE['USER']}:%s@".

TIL nouvelle syntaxe, je connaissais seulement '{} {}'.format('one', 'two')

celine-m-s · 2019-07-15T07:57:24Z

@@ -0,0 +1,147 @@
+from datetime import date


Une docstring générale ne ferait pas de mal pour expliquer ce que fait le script et comment l'utiliser, toujours dans l'optique de la MCO.

celine-m-s · 2019-07-15T07:58:12Z

+import pandas as pd
+from labonneboite.importer import util as import_util
+from labonneboite.importer import settings as importer_settings
+from labonneboite.importer.jobs.common import logger


Ce serait bien de différencier les librairies natives des externes et de celles du projet par un saut de ligne.

celine-m-s · 2019-07-15T08:00:50Z

+def clean_csv_act_dpae_file(existing_sql_table=True):
+
+    dpae_folder_path = importer_settings.INPUT_SOURCE_FOLDER + '/'
+    csv_path = dpae_folder_path+'act_dpae.csv'


A mon humble avis, tu peux supprimer la ligne précédente et écrire csv_path = f"{importer_settings.INPUT_SOURCE_FOLDER}/act_dpae.csv", ça enlève une variable et c'est plus lisible.

celine-m-s · 2019-07-15T08:08:11Z

+                    inplace=True)
+
+
+    def get_type_contrat(row):


Est-ce que ce ne serait pas plus logique que les fonctions soient réunies au début du script, avant la fonction principale appelée par run_main ? Là ça mélange tout.
D'ailleurs j'ai un peu de mal à saisir pourquoi plusieurs fonctions sont dans "clean_csv_act_dpae_file" alors qu'elles pourraient être définies précédemment et être ainsi indépendantes. Ca aiderait grandement à les tester en tests unitaires.

celine-m-s · 2019-07-15T08:45:27Z

+    #We select the last DPAE date that has been used in the last joined dpae
+    engine = import_util.create_sqlalchemy_engine()
+
+    query = "select date_embauche from act_dpae_clean order by date_embauche DESC LIMIT 1 "


Ce serait plus lisible si les commandes SQL étaient toutes en majuscules.

celine-m-s · 2019-07-15T08:48:58Z

+        chunksize = 10 ** 5
+    else:
+        chunksize = 10 ** 6
+    i = 0


Devrait être juste avant la ligne 77 car est utilisé uniquement dans le cadre de cette boucle for.

celine-m-s · 2019-07-15T08:49:52Z

+from labonneboite.scripts.impact_retour_emploi.scripts_charts import grand_public as gd
+from labonneboite.importer import util as import_util
+from labonneboite.importer.jobs.common import logger
+from labonneboite.scripts.impact_retour_emploi.settings_path_charts import root_path, clean_path, gd_pub_path, images_path


Même remarque que précédemment sur les imports.

celine-m-s · 2019-07-15T08:50:38Z

+
+ALPHABET = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'
+
+# FIXME : Refacto all files about the creation of charts and pasting on sheets (<3 Joris)


celine-m-s · 2019-07-15T08:57:42Z

@@ -0,0 +1,6 @@
+from labonneboite.importer import settings as importer_settings


Ces settings ne devraient-ils pas être dans scripts_charts ? Ainsi il n'y aurait pas à l'appeler "settings_path_charts" mais juste "settings.py".
Our bien dans les settings de l'importeur (s'il y en a) de manière générale. Ce serait d'ailleurs mieux car ça centralise toutes les configurations de ce module, non ?

dejafait

Quelques points en plus de la review très complète de @celine-m-s

dejafait · 2019-07-15T10:12:59Z

    return con, cur

+def create_sqlalchemy_engine():
+    connexion_url = ('mysql://'+DATABASE['USER']+':%s@'+\


TIL nouvelle syntaxe, je connaissais seulement '{} {}'.format('one', 'two')

dejafait · 2019-07-15T10:19:23Z

+    cell_A5.alignment = openpyxl.styles.Alignment(horizontal="center")
+    DPAE_for_gd_pub = [cell_A4.value, nbre_DPAE]  # for grand_public
+
+    # Add number of IDPE unique


IDPE =?

on utilise PEID partout ailleurs dans le code et avec les autres startups, je conseille de s'aligner

s/idpe/peid/

dejafait · 2019-07-15T10:21:47Z

+    return [title]+stats
+
+
+def Graph(ordre, columns_date, df, title, name, time_type='week'):


méthode hyper longue à casser en petits bouts

dejafait · 2019-07-15T10:22:18Z

+        x = []
+
+
+def Stacked_Bar(ordre, columns_x, df, titles, name, columns_legend):


dejafait · 2019-07-15T10:23:42Z

+04 	Réunion
+05 	Saint Pierre et Miquelon
+06 	Mayotte
+'''


à quoi sert cette string? Pourquoi n'est-elle pas stockée dans une variable?

dejafait · 2019-07-15T10:24:18Z

+                 "La plus grande valeur est : "]
+
+
+def build_grand_public_sheet(nbre_DPAE, nbre_IDPE, nbre_IDPE_sign, all_stats, impact_xlsx):


à casser en petits bouts

dejafait · 2019-07-15T10:25:06Z

+openpyxl
+pygal
+pygal_maps_fr
+matplotlib


dejafait · 2019-07-15T10:27:06Z

+matplotlib
+imgkit
+xlsxwriter
+cairosvg


c'est chaud d'importer autant de libs (comme matplotlib) en production alors qu'elles sont utiles seulement pour l'IRE

on paye le prix de la non séparation de l'importer vs le frontend/api

j'ai pas de solution mais je veux bien l'avis de @celine-m-s

à noter: Michel pour son projet ROME NAF avait séparé ses requirements dans son dossier spécifique ROME_NAF à la racine du repo, c'était pas mal

C'est vrai que ce n'est pas très efficace...
Peut-être qu'on peut créer un requirements.in spécifique à l'importer et installer ces dépendances dans chaque commande make qui les nécessite ?
Ou bien, si c'est Ansible qui lance les commandes make et installe l'environnement, on peut les installer à ce moment-là ?

dejafait · 2020-01-02T13:51:22Z

Elle a 8 mois cette branche, temps de tourner la page et en fait une nouvelle non? @JimmyDore besoin de review?

JimmyDore force-pushed the jimmyd/impact_retour_emploi branch from 8618084 to 8d24c28 Compare July 9, 2019 12:27

celine-m-s reviewed Jul 15, 2019

View reviewed changes

dejafait suggested changes Jul 15, 2019

View reviewed changes

JimmyDore force-pushed the jimmyd/impact_retour_emploi branch from 45e1449 to 8ac38f2 Compare July 16, 2019 09:31

JimmyDore force-pushed the jimmyd/impact_retour_emploi branch from 8ac38f2 to 741db50 Compare July 31, 2019 15:29

JimmyDore force-pushed the jimmyd/impact_retour_emploi branch from 741db50 to f4e39e0 Compare September 23, 2019 14:51

dejafait assigned JimmyDore Jan 2, 2020

JimmyDore added 22 commits January 3, 2020 12:01

WIP Add Joris work about impact retour emploi

841a83d

Update setup.py to create executables in virtualenv for ire scripts

c51259f

wip

960aebf

Industrialize daily copy script

389034d

Fix scripts launcher

a5f6d0f

Add logs informations

5589f81

Add Exception for the daily parser script

7923a64

Clean and prepare jobs join & clean activity_logs-dpae for Jenkins

f6f36b2

Remove debug mode

04902f2

Add log about size of DPAE file

d4e8756

wip make report

c333095

Fix (approximately) issues with path

22aaf5f

Fix last problem with path

adfbcb1

Add settings file with different paths

b3693ce

Fix import module charts

17e4c6f

Add useful libs to install in DockerFile

0292e93

Add xvfb to run imgkit from Docker image

a503443

Add comments on main script to make charts and excel report

bf21e56

Update name of DPAE file to be used

5d439e3

Add function to parse activity logs for PSE study

6ff55df

Update the way to check if a file needs to be used or not

571b82f

Add option to join data on SIREN (or SIRET as before)

5299955

JimmyDore added 8 commits January 3, 2020 12:01

Remove debug mode

98a068b

Fix import

cd71c44

Fix check existence of csv generated file

d2e40a3

Fix SIREN issue int/str

a68b5ed

Fix types of columns siren/siret

5ab18af

Fix pandas bug

e9c9653

Try with SIRET to compare data

ccf6a21

Fix path to dpae file

fc35a7a

dejafait force-pushed the jimmyd/impact_retour_emploi branch from 4421e1d to fc35a7a Compare January 3, 2020 10:01

Fix siren bug

43d3b82

JimmyDore added the obsolete label Apr 3, 2020


		ALPHABET = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'

		# FIXME : Refacto all files about the creation of charts and pasting on sheets (<3 Joris)

		@@ -0,0 +1,6 @@
		from labonneboite.importer import settings as importer_settings

		return [title]+stats


		def Graph(ordre, columns_date, df, title, name, time_type='week'):

		x = []


		def Stacked_Bar(ordre, columns_x, df, titles, name, columns_legend):

		"La plus grande valeur est : "]


		def build_grand_public_sheet(nbre_DPAE, nbre_IDPE, nbre_IDPE_sign, all_stats, impact_xlsx):

Conversation

JimmyDore commented Jul 5, 2019

Uh oh!

celine-m-s left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dejafait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dejafait Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dejafait commented Jan 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

celine-m-s left a comment •

edited

Loading

dejafait Jul 15, 2019 •

edited

Loading