-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
Réecriture évaluation règle #1191
The head ref may contain hidden characters: "refacto-evaluation-r\u00E8gle"
Conversation
d84bdc9
to
d5832b1
Compare
bca8c22
to
29deac7
Compare
4f3a75a
to
682878d
Compare
682878d
to
c6df4d3
Compare
7c0b08c
to
fb1d0d8
Compare
f50b671
to
04fd3f1
Compare
- Introduction de nouveaux mécanismes - Réecriture de l'evaluation et du parsing des règles. - Les règles peuvent apparaître dans les formules de calcul - Introduction d'un AST en bonne et due forme - Réecriture de buildRuleDependancies. - Ajout d'une passe pour la désambiguation des références - Réecriture de rendNonApplicable et de remplace - Réimplémentation de parentDependancy Voir #1191
a11d82b
to
86a0046
Compare
🚀 La branche est déployée ! https://deploy-preview-1191--syso.netlify.app Voir aussi : version anglaise | site publicodes |
86a0046
to
6b1915b
Compare
Pourquoi a-t-on besoin de garder la référence vers la raw node? |
]) | ||
} | ||
|
||
function buildRuleDependancies(rule: RuleNode): Array<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo ici: buildRuleDependencies
* Tarjan method, which is **not necessarily the smallest cycle**. However, the | ||
* smallest cycle would be the most legibe one… | ||
*/ | ||
export function cyclicDependencies<Names extends string>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tu peux enlever <Names>
ici
this.parsedRules = parsePublicodes( | ||
rules as Record<string, Rule> | ||
) as ParsedRules<Name> | ||
this.replacements = getReplacements(this.parsedRules) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(comme pour la plupart de mes commentaires ceci n'est peut-être pas lié à la PR en question mais il est difficile pour moi de faire la diff)
Je vois que les remplace
finissent mangés à deux sauces:
- d'une part sous forme de
ReplacementNode
dans l'AST - d'autre part dans la liste
Engine.replacements
.
En fait je trouve ça assez asymétrique d'une certaine façon. Une autre manière de le voir est que le top-level Engine
a besoin de connaître ReplacementNode
alors qu'il est agnostique des autres types de node.
A voir si on peut y faire qqch, mais ce serait mieux que tu m'expliques (de vive voix sans doute) pourquoi la structure actuelle est ce qu'elle est, pour que je puisse reformuler ma remarque plus précisément.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En fait je trouve ça assez asymétrique d'une certaine façon. Une autre manière de le voir est que le top-level Engine a besoin de connaître ReplacementNode alors qu'il est agnostique des autres types de node.
Oui, c'est tout à fait une limite. La raison est que Engine a besoin des remplacements pour pouvoir remplacer les valeurs dans la situation ou dans une évaluation :
const rules = `
a: 15
x:
remplace: a
valeur: 10
`
new Engine(rules).evaluate('a') === 10 // true
En fait, ce qu'il manque pour avoir une API unifiée, et plus compacte, c'est la possibilité pour Publicode d'être interprété à la volée, un peu comme n'importe quel langage interprété. Par exemple si je tape :
> const a = 5
dans la console js, et que j'évalue a
quelques lignes plus tard, le contexte est sauvegardé.
Il faudrait la même chose pour le moteur. Lorsqu'on lui passe des définitions de règles, il retourne rien mais le sauvegarde dans un contexte d'execution. Lorsqu'on lui passe des expressions, ils les évalue dans le contexte donné (avec les remplacements?)
Cela permettrait également de gérer le cas d'extension de base de règle. On pourrait "à la volée" ajouter des nouvelles règles sans qu'il ne soit nécessaire de reparser toutes celles existantes.
Mais c'est une modification pour plus tard, en attendant d'y voir un peu plus clair.
parsedSituation: ParsedSituation<Names> = {} | ||
export { Rule } | ||
|
||
export type evaluationFunction<Kind extends NodeKind = NodeKind> = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1ere lettre Majuscule pour un type, non?
const missingVariables = mergeAllMissing( | ||
[explanation.situationValeur, explanation.valeur].filter( | ||
Boolean | ||
) as Array<EvaluatedRule> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je trouve étonnant/déroutant de trouver des type XxxRule
dans les mécanismes, alors que ceux-ci devraient opérer sur l'AST, non?
Je trouverais ça plus clair d'avoir des types Rule
(ou bien RawRule
, changement plus simple à faire) qui n'interviennent que dans le parsing, et ensuite des XxxNode
(ou bien on pourrait garder Rule
aussi là-dedans si on utilise RawRule
) pour les nodes de l'AST, qui interviennent pour l'évaluation.
Cf. ma remarque sur rule.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oui, c'est le mauvais type ici. Je voulais typer par EvaluatedNode, ce qui a beaucoup plus de sens 👍
} | ||
} | ||
|
||
export default function Arrondi(recurse, v) { | ||
export default function Arrondi(v, context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je suis gêné par les fonctions qui débutent par une majuscule quand ce ne sont pas des composants React. N'y a-t-il pas une convention pour ça?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je suis d'accord avec cette remarque
} as SituationNode | ||
} | ||
|
||
parseSituation.nom = 'nom dans la situation' as const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pas fan, mais pas grave. J'aurais exporté une var "globale" (une var de module quoi).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oui je suis d'accord, c'est pas l'idéal. Dans le futur, les modules de définition de mécanisme auront une signature unifiée, ce qui permettra de se débarrasser de ça #1015
} | ||
export default function parseSituation(v, context) { | ||
const explanation = { | ||
situationKey: v[parseSituation.nom], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Si je comprends cette ligne et ce que tu dis dans ton commentaire explicatif, il faut s'attendre à ce que le code publicodes contienne des nom dans la situation
. Mais je n'en vois pas? Dans ce cas je ne comprends pas à quoi cela sert?
En tout cas ce qui serait bien c'est d'ajouter un test unitaire pour expliquer comment ça marche. Pour l'instant je n'ai pas compris.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caveat (plutôt que de mettre un edit, ça explique le problème intellectuel que cela pose).
En fait je viens de voir dans rule.tsx
que tu modifies la raw rule pour en faire un objet intermédiaire qui est passé à parse
(fn qui n'est pas typée correctement). Autrement dit c'est difficile à lire.
Je vois deux problèmes/questions:
- Est-ce que
nom de la situation
a vocation à être utilisé dans du code publicodes? Si oui ton approche de créer un mécanisme est juste. Si non, alors quelle est la raison qui t'a poussée à en faire un? Dans un cas comme dans l'autre, tu sembles avoir choisi "nom de la situation" en francais au lieu d'un nom de variable allGluedTogether en anglais, donc ça serait bien d'avoir un test unitaire d'exemple explicatif. - Je trouve qu'on s'y perd dans le data flow. En gros j'aurais bien aimé avoir un truc du genre Publicodes =parse=> AST =evaluate=> EvaluatedAST. Mais là c'est plus complexe et c'est sans nul doute légitime. Mais je pense qu'on peut faire mieux en rendant tout cela explicite au plus haut niveau (au niveau "controller" de l'engine, au niveau de l'appel à
parse
quoi), avec un truc du genre Publicodes =parse1=> PublicodesWithSituation =parse2=> AST… A mon avis ce genre de formalisation n'ajoute pas de code au total et donne des abstractions plus solides. J'aime bien la notion de wholemeal programming.
Parlons d'ailleurs ensemble de si on pourrait essayer d'avoir un typage qui ressemble à cela dans index.ts
pour que les choses soient bien claires:
type PreParser = Publicodes => PublicodesExpanded
type Parser = PublicodesExpanded => AST
type Eval = AST => EvaluatedAST
// avec sans doute
type Publicodes = Array<Rule>
type PublicodesExpanded = Array<RuleWithSituation>
type AST = Array<RuleNode>
type EvaluatedAST = Array<EvaluatedRule>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il y a plusieurs remarques ici :
1/ nom dans la situation
Ce mécanisme est la brique de base qui opère pour la logique des missingVariables. Il permet de dire : à cette endroit de la chaîne d'évaluation, on vérifie si la valeur nommée existe dans la situation. Si oui, alors on la retourne. Si non, on évalue le reste.
Si la valeur évaluée est nulle et qu'elle ne retourne pas de missings variables, alors on est sur une "feuille" de l'arbre. On ajoute le nom dans la situation dans les missings variables.
Cette logique est, dans l'état actuel du moteur, indissociée de la définition d'une règle. Lorsque l'on définit une règle, on s'attends à pouvoir trouver dans la situation un valeur indexée par le dottedName, et à utiliser celle si au besoin.
Oui mais voilà, on ne peut pas ajouter cette logique directement dans le parseRule car elle s'intègre plus loin dans la chaîne d'execution. On veut que la situation (ou les missings) soient remontées, et que les mécanismes suivants (si ils existent) s'applique au dessus : unité
, arrondi
, par défaut
, applicable
, non applicable si
, plancher
, plafond
.
D'où le fait que ce soit un mécanisme à part entière.
Je vais faire un test unitaire là dessus.
2/ Typage
Je suis d'accord pour le data flow. J'aimerai avoir des types bien plus précis pour l'AST, en fonction des étapes de parsing qu'il a subit. Mais pour ne pas trop me perdre dans le typage, je me suis limité à une définition générique qui est raffinée au cas par cas. Au fur et à mesure, on pourra affiner le typage de l'AST en le paramétrisant avec l'étape. Quand au fait que l'AST commence au niveau de la règle et pas au niveau de parsedRules, c'est parce que je ne voulais pas que l'introduction des listes soit forcément associée au noeud racine. En attendant d'y voir plus clair, j'ai préféré m'abstenir.
return reduceAST<string[]>( | ||
(acc, node, fn) => { | ||
switch (node.nodeKind) { | ||
case 'replacement': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh beh! C'est bcp mieux :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En revanche je suis très fortement pour éviter les strings qui se balladent et systématiquement remplacer par des variables constantes. Les strings c'est risqué pour l'avenir, car la dépendance n'est pas explicite et un changement peut ne pas être répercuté. En plus, je ne vois pas d'où vient cette string "replacement"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je ne suis pas d'accord dans ce cas précis. Nous sommes en typescript, aussi, ce n'est pas "une string qui se ballade" mais bien une valeur parmis les valeurs possibles de nodeKind. Si une typo se glisse, TS le signalera directement. Ecrire une constante n'a plus d'utilité dans ce cas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Ceci dit je pense que mon soucis est toujours présent, car le typage qui est remonté pour la var node
est any
, ce qui explique que je n'y retrouvais pas.
Est-ce que tu as la même chose chez toi? J'utilise le tsserver local de mon côté.
) | ||
} | ||
|
||
function buildDependenciesGraph(rulesDeps: RulesDependencies): graphlib.Graph { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai comparé les graphes entre le common ancerstor de master (a746e51) et cette branche et j'ai des différences. Pas trouvé pourquoi de manière évidente, tu as une idée? Le graphe de dépendances est plus fourni dans ton cas.
Je me note de regarder plus tard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Et pourtant j'ai du enlever les dépendances liées au remplacement qui faisait vraiment trop de boucle assez dure à comprendre. D'ailleurs pour les boucles, c'est vraiment dommage que ce ne soit pas systématiquement les plus petites qui soit retournée par l'algorithme. Il n'y a pas moyen de faire autrement ?
Le graphe de dépendances est plus fourni dans ton cas.
Non, pas d'idée claire là dessus...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'ailleurs pour les boucles, c'est vraiment dommage que ce ne soit pas systématiquement les plus petites qui soit retournée par l'algorithme. Il n'y a pas moyen de faire autrement ?
L'algo super-naïf suivant ne fonctionne pas ?
function findCycle(dottedName, stack = []) {
if (stack.includes(dottedName)) {
throw `Cycle détecté : ${print(stack)}`
}
parsedRules[dottedName].dependencies.forEach(dependencyName=> {
findCycle(dependencyName, [...stack, dependencyName])
})
}
Object.keys(rules).forEach(findCycle)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mquandalle On pourrait en effet lancer un tel algo sur les parties du graphe qui sont déjà identifiées comme bouclées par le premier algo plus efficace fourni par la lib (hypothèse à vérifier en fait: l'algo naif serait trop lent pour être lancé sur tout le graphe).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Et si on mémoïse cet algo naïf, tu penses qu'on est encore trop lent ?
Edit: hum en fait mémoïser ne sert probablement à rien ici
Vu avec @mquandalle, cette PR est désormais trop grosse pour suivre. Je vais la merger dans next, et faire de plus petite PR pour les modifications. |
6b1915b
to
02de456
Compare
- Introduction de nouveaux mécanismes - Réecriture de l'evaluation et du parsing des règles. - Les règles peuvent apparaître dans les formules de calcul - Introduction d'un AST en bonne et due forme - Réecriture de buildRuleDependancies. - Ajout d'une passe pour la désambiguation des références - Réecriture de rendNonApplicable et de remplace - Réimplémentation de parentDependancy Voir #1191
- Introduction de nouveaux mécanismes - Réecriture de l'evaluation et du parsing des règles. - Les règles peuvent apparaître dans les formules de calcul - Introduction d'un AST en bonne et due forme - Réecriture de buildRuleDependancies. - Ajout d'une passe pour la désambiguation des références - Réecriture de rendNonApplicable et de remplace - Réimplémentation de parentDependancy Voir #1191
Tiens la fermeture automatique des issues associées n'a pas fonctionné — on aurait peut-être dû laisser cette PR ouverte, ou bien définir |
Cette PR répond aux issues suivantes : #1153 #1139 #1031
Elle est une étape nécessaire à #1070
fix #1244, fix #1246, fix #887, fix #988, close #1153, close #1139, close #1031
Voici les gros changements effectués :
Introduction de nouveaux mécanismes
5 nouveaux mécanisme chainés font leur apparition :
Réecriture de l'evaluation et du parsing des règles.
Le parsing et l'évaluation des règles (hors parentDependancy) tiens maintenant en quelques lignes, en réutilisant les mécanisme de base :
https://github.com/betagouv/mon-entreprise/pull/1191/files#diff-dbc098c72918ac2ea376ce1072aaab5901568fd0c951fcf262bd77e353865741R68-R80
Cela signifie que le corps d'une règle accepte tout objet publicode bien formé. On peut donc écrire :
Pour des raisons de compatibilités, il est toujours possible d'utiliser le mot-clé
formule
, celui-ci sera traduit envaleur
avant d'être parsé.Les règles ne sont plus limitées aux racines.
Il est possible de définir des nouvelles règles à l'intérieur d'une règle. Pour cela, c'est il suffit d'ajouter une propriété
nom
à un objet publicodes.Dans cet exemple, deux nouvelles règles seront crée :
cotisations retraite . salarié
etcotisations retraite . employeur
. Elle sont des règles en soi, c'est à dire qu'elle seront parsée et évaluée de la même manière que les règles top-level. Cette fonctionalité permet de réecrire le mécanisme composante comme une simple transformation statique : https://github.com/betagouv/mon-entreprise/pull/1191/files#diff-043db8bc2e41434e87c5b13a8f5f6e865c417592a868190efe66a9dd94e0e757L'écriture ci dessous étant transformée en celle ci-dessus :
Le mot clé
[ref]
est également transformé en utilisant cette fonctionalité.Introduction d'un AST en bonne et due forme
publicodes/source/AST/types.ts
Cette PR introduit le typage systématique des noeuds de l'AST. Chaque mécanisme à son propre noeud, avec un
nodeKind
spécifique. Il existe également un type de noeud ReplacementNode et un type RuleNode pour les règles.On a également définit des fonctions permettant de travailler avec cet AST. Une ou de récupérer une valeur calculée
updateAST
pour le modifier en gardant sa structure d'arbre etreduceAST
pour récuperer une valeur calculée.cf https://github.com/betagouv/mon-entreprise/pull/1191/files#diff-43a46b95c72d582ad6897062b07075592461d40c9a3ebd242733a6192314e10e
Réecriture de buildRuleDependancies.
Pour récupérer les dépendances d'une règles, on utilise maintenant la fonction
reduceAST
et tiens en 10 ligneshttps://github.com/betagouv/mon-entreprise/pull/1191/files
Ajout d'une passe pour la désambiguation des références
En utilisant la fonction
updateAST
, on update des noeud de type référence avec le dottedName référencé. Si il n'existe pas, on lève une erreur. On doit le faire dans une passe séparée pour prendre en compte les règles crées à l'intérieur d'autres règles.Réecriture de
rendNonApplicable
et deremplace
Ces deux mécanisme prennent la forme d'une substitution de références dans la lignée de ce qui a été discuté ici. Le détail d'implémentation est situé dans le fichier https://github.com/betagouv/mon-entreprise/pull/1191/files#diff-043db8bc2e41434e87c5b13a8f5f6e865c417592a868190efe66a9dd94e0e757
Réimplémentation de parentDependancy
Chaque règles a maintenant une dépendance à son parent, peu importe son type (plus de findParentDependancy hacky. Si ce dernier est non applicable, la règle est non applicable. On detecte les boucles au runtime et on les ignore si il y en a.
Réimplémentation du mécanisme synchronisation
Avec l'ajout d'un type "objet" et le renommage de l'attribut "API" en "data"
Breakings notoires 🚨
variable [filtre]
n'est plus supportée. Pour convertir l'unité d'une variable, on passera par le mécanisme chaîné unité, et les composantes sont maintenant gérée comme le reste des règles.a
est manquante pour le calcul deb
(puisque parentDependancy). Pour éviter cela, il faut remplacer para: oui
%.€/mois
. Pour avoir une valeur simplifiée il faut maintenant appelerformatValue
ou utiliser le mécanismeunité
.rend non applicable
permet de désactiver un espace de nom.RuleNode
. Les autres sont accessible via la propriété rawNode. Ce point est sans doute le breaking le plus important, et j'aimerai en discuter avec vous. L'idée est de garder un AST clean, avec les champs du domaine utilisateur relégués derrière une propriété pour éviter au maximum les interference avec les champs calculés par le moteur. L'autre possibilité serait de mettre les champs moteurs derrière une clé__internal
.TODO
La vue "voir ma situation"
Les vues des mécanisme applicable, non applicable, situation, unité et par défaut
Tests mon-entreprise
Dependancies d'une règle
Suppression de la conversion d'unité inline
Suppression des filter (rempacé par l'utilisation de définition dans composante)
Ajout d'un mécanisme définition
Réecriture du mécanisme composante
Récriture de parseRule
Suppression de evaluateRule
Séparation entre parsedRules et parsedDefinitions. Toutes les règles ont une définition, mais toutes les définitions ne sont pas forcément des règles (et donc n'ont pas forcément une page associée dans la doc)
Suppression de virtualRule
...
Réimplémenter remplace
Réimplémenter rend non applicable
Réimplémenter parentDependancies
Prochaine étape
Il y en a plusieurs :