Pilotage : Chiffres clés dynamiques dans le tableau de bord - #4589
Conversation
rsebille
added
1-recette-jetable
|
🥁 La recette jetable est prête ! 👉 Je veux tester cette PR ! |
134df89 to
2c65d14
Compare
2c65d14 to
141745d
Compare
hellodeloo
left a comment
hellodeloo
left a comment
There was a problem hiding this comment.
Je t'ai pushé un petit commit d'ajustement ui 😘
|
@rsebille si t'es chaud, y'a le même besoin sur pilotage qui attend que j'ai le temps et que j'en sois capable ;) |
a942fb7 to
4695868
Compare
rsebille
left a comment
rsebille
left a comment
There was a problem hiding this comment.
PI, la PR passe en stand by pour le moment car il y a un problème métier avec les deux derniers indicateurs donc j’attends la décision sur le sujet : soit suppression, soit remplacement
9cd5095 to
83990c8
Compare
rsebille
removed
the
1-recette-jetable
eb9d130 to
7a5a314
Compare
155bb71 to
15f688c
Compare
cc614a2 to
eb3326a
Compare
|
Je réouvre la PR maintenant que les chiffres affichés sont OK et validés |
92ed9da to
d6f9603
Compare
| "region": [17676], | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Tous ces entiers/ids en dur 🙈
| *filters, | ||
| ] | ||
|
|
||
| def fetch_card_results(self, card, filters=None, group_by=None, *, single_value=False): |
There was a problem hiding this comment.
J'ai l'impression qu'on appelle uniquement cette fonction avec single_value=True , a-t-on besoin de single_value=False ? 👀
There was a problem hiding this comment.
Dans notre utilisation actuel non car on ne va chercher que des chiffres agrégés, mais autant le gérer dés à présent plutôt que devoir se replonger dans l'API metabase à un autre moment, non ?
| raise RuntimeError(f"{field_id=} was not found in columns metadata") | ||
|
|
||
| @staticmethod | ||
| def transform_metabase_results(results, group_by=None, *, single_value=False): |
There was a problem hiding this comment.
Peut-être qu'un commentaire détaillant les structures possibles de results aiderait à la compréhension 😬
d6f9603 to
53062f5
Compare
| assert Client.transform_metabase_results(data_results, group_by=["Col 2"], single_value=True) == { | ||
| 1: 1.0, | ||
| 2: 2.0, | ||
| } |
There was a problem hiding this comment.
Je trouve la combinaison des group_by & single_value assez peu intuitive: ce n'est pas clair pourquoi on récupère la valeur de Col 3 (et non celle de Col 1 par exemple, ou ce qu'il se passe si on récupérait encore plus de colonne.
Peut-être que pour clarifier, il faudrait enlever la partie single_value de transform_metabase_results et faire la transformation spécifique à single_value dans fetch_card_results directement ?
Après le retour de l'API est assez peu clair et https://www.metabase.com/docs/latest/api/card ne fournit pas la réponse 😬
There was a problem hiding this comment.
Je trouve la combinaison des
group_by&single_valueassez peu intuitive: ce n'est pas clair pourquoi on récupère la valeur deCol 3(et non celle deCol 1par exemple, ou ce qu'il se passe si on récupérait encore plus de colonne.
Oui mais en principe si tu donnes single_value=True alors il n'est sensé y avoir qu'1 seul élément autre que la clé donc ça me choque pas que dans le cas où tu en donnes plus ça tombe dans un cas indéfini et laissé au choix de l'implémentation. C'est ce qui ce passe ici, on utilise .popitem() donc on prend la dernière valeur récupérée, et ça colle aussi au fait que l'API retourne toujours les champs group_by premier donc que la valeur est a la fin.
Peut-être que pour clarifier, il faudrait enlever la partie
single_valuedetransform_metabase_resultset faire la transformation spécifique àsingle_valuedansfetch_card_resultsdirectement ? Après le retour de l'API est assez peu clair et https://www.metabase.com/docs/latest/api/card ne fournit pas la réponse 😬
Oui, la doc de l'API c'est juste une liste d'endpoint donc c'est de la pure ingénierie inverse d'où le fait que tout ne soit pas super clair et que ça tâtonne :).
fetch_card_results est déjà assez poilu donc je sais pas si ça serais mieux, ou peut-être dans une fonction à part, mais pas sûr 🤔.
There was a problem hiding this comment.
Disons que sans le cas particulier du group_by/single_value , transform_metabase_results devient très simple et sort une liste de dictionnaire clef valeur.
Alors qu'actuellement la partie du group_by est assez indigeste et semble se baser sur plein de non-dits sur le format a priori qu'on reçoit et qui (j'imagine) dépendent de l'endpoint utilisé d'où IMHO l'intérêt de déplacer cette partie à coté de l'appel au endpoint.
Ainsi dans fetch_card_results on aurait pour la carte seule:
data = self._client.post(f"/card/{card}/query").raise_for_status().json()
[single_metadata_result] = self.transform_metabase_results(data["data"])
return single_metadata_result["la bonne colonne - dont le nom est ptet récupérable dans data ?"]
et pour le dataset:
data = self._client.post("/dataset", json=card_query).raise_for_status().json()
metadata_results = self.transform_metabase_results(data["data"])
# insérer ici du code intelligible ^.^
53062f5 to
ea1b073
Compare
rsebille
removed
the
1-recette-jetable
ea1b073 to
56922e9
Compare
|
Gros changement au niveau du client API, je passe par l'export en JSON pour les deux endpoints donc plus besoin de recoller les noms à la main, ce qui par extension m'a fait supprimer |
1dff31c to
79e923d
Compare
xavfernandez
left a comment
xavfernandez
left a comment
There was a problem hiding this comment.
C'est effectivement plus digeste 👍
| return {"database": database, "type": "query", "query": query, "parameters": []} | ||
|
|
||
| def fetch_dataset_results(self, dataset_query): | ||
| # /!\ MB (hardcoded) limit to the first 2000 rows when "viewing", "download" has a 1_000_000 rows limits |
There was a problem hiding this comment.
Je ne vois pas de lien entre ce commentaire et le code ?
There was a problem hiding this comment.
Oui c'est pas clair je vais compléter, en fait il y a un endpoint /dataset qui permet d'explorer les résultats d'une requête mais ça limite à 2_000 lignes car c'est pour faire joujou dans l'interface, alors que /dataset/<format> est pour exporter les données et est donc limité à 1_000_000 lignes cette fois.
| if limit: | ||
| query["limit"] = limit |
There was a problem hiding this comment.
On est d'accord que ce n'est jamais utilisé dans le code (tout comme select & table) ?
There was a problem hiding this comment.
Oui c'est inutilisé, mais vu que je suis tombé dessus et que ça complexifiais pas trop le code je me suis que c'était pas plus mal d'inclure tout ceux que j'avais trouvé pour "documenter".
Et select ou table vont me servir coté pilotage très très bientôt :).
79e923d to
cde6ebd
Compare
| # - `/dataset` limit to 2_000 rows as it is used to preview query results | ||
| # - `/dataset/{export-format}` limit to 1_000_000 rows as it is used to download queries results | ||
| data = self._client.post("/dataset/json", data={"query": json.dumps(query)}).raise_for_status().json() | ||
| if type(data) is not list: # `/dataset/json` return a list of rows if successful otherwise it's a dict |
There was a problem hiding this comment.
Oui... Metabase sort beaucoup de chose en cas d'erreur donc par précaution j'ai préféré ne pas faire totalement passe-plat.
cde6ebd to
d451e02
Compare
4 participants
🤔 Pourquoi ?
[Chiffres clés dynamiques TB privés] - Institutions : intégrer les chiffres clés dynamiques et à jour sur la page statistique privée des emplois
🍰 Comment ?
Récupération des données via l'API Metabase, elles sont ensuite stockées dans le cache (un redis avec une expiration très longue)
🏝️ Comment tester
[Optionnel] Pour avoir des données :
METABASE_API_KEY./manage.py metabase_data fetch --wet-run💻 Captures d'écran