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

496 extend comment syntax in khiops dictionaries #503

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

marcboulle
Copy link
Collaborator

@marcboulle marcboulle commented Dec 20, 2024

Seuls les 6 derniers commits sont a prendre en compte.
Le commit principal contient une spécification détaillée de la fonctionnalité, après ré-actualisation due à certains choix d'implémentation: Extend comment syntax in Khiops dictionaries

@marcboulle marcboulle requested a review from popescu-v December 20, 2024 10:58
@marcboulle marcboulle linked an issue Dec 20, 2024 that may be closed by this pull request
@marcboulle marcboulle force-pushed the 496-extend-comment-syntax-in-khiops-dictionaries branch from 5f99393 to fe67982 Compare December 20, 2024 14:03
@@ -2067,28 +2109,90 @@ boolean KWClass::CheckNameWithMessage(const ALString& sValue, int nEntity, ALStr

boolean KWClass::CheckLabelWithMessage(const ALString& sValue, int nEntity, ALString& sMessage)
{
const int nMaxLabelLength = 100000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we group these constants (also below) in a header file like KWClass.h (AFAIU we can do this in C++, as they default to static linkage, so there is no name clash with other translation units)?

@marcboulle marcboulle requested a review from popescu-v December 20, 2024 15:24
@@ -68,9 +68,29 @@ class KWClass : public Object
KWMetaData* GetMetaData();

// Libelle
// Premiere ligne prefixee par '//' precedent la declaration du dictionnaire
Copy link
Collaborator

@popescu-v popescu-v Dec 20, 2024

Choose a reason for hiding this comment

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

s/precedent/precedant/ (present participle here?)

const ALString& GetLabel() const;
void SetLabel(const ALString& sValue);

// Commentaires
// Ensemble des lignes prefixees par '//', entre le libelle et le debut de la declaration du dictionnaire
// Par tolerance du parser, on accepte egalement tout commentaire situee apres la partie obligatoire
Copy link
Collaborator

Choose a reason for hiding this comment

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

situee (delete second "e").

if (GetLabel() != "")
fJSON->WriteKeyString("label", GetLabel());

// Commentaires
if (GetComments()->GetSize() > 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder whether it wouldn't be more consistent to keep the contents of the if block in all cases, that is, have comments array even when there is no comment (i.e. GetComments()->GetSize() == 0 - in this case the array would just be empty, because the for loop would not have what to iterate on).

Contraintes sur les cle dans le cas de la creation de table
- une table creee peut etre associee a un dictionnaire ayant ou non des champs cles
  - par exemple, si l'on construit une Table a partir d'un champs Json, on n'a pas besoin de cle
- par contre, il est interdit de construire une table externe, geree par un dictionnaire Root
  - en effet, dans ce cas, Root est un indicateur de table externe explicite, ce qui implique un fichier de donnees dedie

En definitive, un schema multi-table cree ne peut etre Root, mais n'a sinon aucune contrainte sur les cles.
Le schema "natif" issu des variables relations non calculees (Table ou Entity) peut alors impliquer des dictionnaires
ayant ou non des cles, a tous les niveau de la hierarchie.
En fait, un schema multi-table est specifie selon sa structure hierarchique, native ou calculee.
Un meme schema en flocon (ex: Customer-Services-Usages) peut etre ainsi alimente de plusieurs facon differentes:
- depuis trois fichiers, si le schema est natif et exploite des dictionnaires avec des cles coherentes
- depuis un seule table, avec un champs json permet de reconstruire le schema a la volee par une regle de construction de table,
  impliquant des dictionnaires ayant ou non des cles, coherentes enre elles ou non

Dans la version V10 ou V11 de Khiops, un fichier de dictionnaire qui compile est necessairement utilisable pour specifier
un mapping multi-tables, avec la liste des fichier necessaire a l'alimentattion des donnees

Dans la version avec creation de table, les controles se font en deux temps
- au moment du chargement du dictionnaire, on verifie tout ce qui est possible
  - pour un dictionnaire Root, on peut verifier que le systeme de cle de son schema est coherent
  - pour les schemas non Root, on ne peut pas verifier a priori la coherence des cle, car ces dictinnaires peuvent etre utilises
    en sortie de regles de creation de table
  - on memorise en indicateur interne les dictionnaire qui sont `storable`, ceux qui sont completement coherent vis a vis des cles
- au moment du choix d'un dictionnaire d'analyse
  - on alimente la liste des fichiers selon le schema natif du dictionnaire principal, qu'il soit coherent ou non
    - cela permet ainsi de visualiser la structure native du schema
    - mais on ne genere pas en temps reel des messages d'erreur complexes, a chaque changement de choix de dictionnaire d'analyse
 - au moment du declenchement d'une action (apprentissage, analyse)
   - on genere des messages d'erreur, les plus didactiques possibles

KWClass
- IsKeyBasedStorable
  - Capacite a etre stocke sur un systeme de fichiers multi-tables a l'aide de cles
  - mise a jour par la methode IndexClass
- CheckKeyBasedStorability: verification de la capacite a etre stocke, avec emission de message d'erreur
- Check
  - appel des CheckNativeComposition avec verification des cles uniquement pour les classes Root
- CheckNativeComposition(bCheckKeys, bVerboseCheckKeys)
  - test de non recursion dans la composition
  - test de validite des cles a la demande
- InternalCheckNativeComposition: methode technique pour la gestion de la methode CheckNativeComposition
  - rappatriement de tous les controles sur la composition d'un dictionnaire et la coherence de ses cles,
    en provenance de KWClass::Check et KWAttribute::Check

KWMTDatabase
- CheckPartially
  - on verifie en prealable que le dictionnaire est stockable, avec si necessaire des messages d'erreur didactiques

Tests systematiques dans LearningTest\TestKhiops\z_TableCreationRules

ost << '{';
if (firstAttribute != NULL)
// Commentaires prededents le debut du bloc
Copy link
Collaborator

@popescu-v popescu-v Dec 20, 2024

Choose a reason for hiding this comment

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

s/prededents/precedant/ (present participle).

Pour des raisons de lisibilite du log, une ligne blanche est ajoute apres chaque action (apprentissage, deploiement..)
Cela permet de separer les compte-rendus des actions et de les lire par blocs.

On ajoute cette fois la ligne blanche apres le declenchement de l'action, que l'action soit executee ou non
en raison d'erreurs de specification utilisateur.
Cela permet d'isoler les messages d'erreur en blocs, pour empecher qu'ils soit "colles" a l'action suivante.

Mise a jour des jeux de test CI/CD impactes (lignes blanches additionnelles dans le log)
Tests complets sur LearningTest, avec mise a jour des jeux de test impactes
Avec la possibilité d'avoir des dictionnaire avec ou sans cle, on peut avoir des dictionnaires stockable ou non.

Au moment du choix d'un dictionnaire d'analyse, dans la liste d'aide, on met en tete les dictionnaires storable,
puis une ligne blanche, puis les dictionnaires non storable.
On les garde tous dans la liste d'aide, pour des raisons didactiques, et pour ne pas "inquieter" les utilisateurs
en cas d'absence de dictionnaires.

Note: il aurait ete preferable de mettre ces nomde dictionnaire en gris dans les liste d'aide, mais le rapport
cout/benefice de cette option est tres defavorable (necessite de modifier significativement le moteur de la GUI dans Norm)

Impacts principaux
- KWClassManagementActionView
  - mise en place d'une liste d'aide rafraichie dynamiquement, dediee aux noms dictionnaire, selon la specification
  - RefreshHelpLists: mise a jour de la liste, appelee dans EventRefresh
- KWLearningProblemView::KWLearningProblemView: parametrage de la liste d'aide avec le service de KWClassManagementActionView
- CCLearningProblemView::CCLearningProblemView: parametrage de la liste d'aide avec le service de KWClassManagementActionView
- KWDatabaseTransferView::Open: variante similaire, mais avec liste d'aide locale (pas de rafaichssement necessaire)
- KWDataTableKeyExtractorView::Open: commentaire expliquant pourquoi on garde cette fois tous les dictionnaires
- KWDataTableSorterView::Open: commentaire expliquant pourquoi on garde cette fois tous les dictionnaires

Impacts la bibliotheque java norm.jar
- src/Norm/NormGUI/src/normGUI
  - GUIObject::getParametersAsArray: correction pour prendre en compte les items vides dans une liste de parametres
  - GUIStringElementHelpedComboBox: correction pour autoriser les doublons (deux fois la valeur vide, par exemple)
Cf. methodes KWMTDatabase::DisplayMultiTableMappingWarnings
  // Affichage de warning dedies au mapping multi-table
  // Ces warnings ne sont pas affiches lors du Check, pour eviter d'entrainer une
  // nuisance pour l'utilisateur par des affichage repetes
  // C'est a l'applicatif de decider quand appeler explicitement cette methode

Utilise uniquement dans:
- KWLearningProblem::ComputeStats: pour l'apprentissage avec Khiops
- CCLearningProblem::BuildCoclustering: pour l'apprentissage avec Khiops coclustering

Nouveau test specifique sur LearningTest\TestKhiops\MultiTables\GraphSchemaFromSubTable

Tests complets sur LearningTest
Contraintes sur les cle dans le cas de la creation de table
- une table creee peut etre associee a un dictionnaire ayant ou non des champs cles
  - par exemple, si l'on construit une Table a partir d'un champs Json, on n'a pas besoin de cle
- par contre, il est interdit de construire une table externe, geree par un dictionnaire Root
  - en effet, dans ce cas, Root est un indicateur de table externe explicite, ce qui implique un fichier de donnees dedie

En definitive, un schema multi-table cree ne peut etre Root, mais n'a sinon aucune contrainte sur les cles.
Le schema "natif" issu des variables relations non calculees (Table ou Entity) peut alors impliquer des dictionnaires
ayant ou non des cles, a tous les niveau de la hierarchie.
En fait, un schema multi-table est specifie selon sa structure hierarchique, native ou calculee.
Un meme schema en flocon (ex: Customer-Services-Usages) peut etre ainsi alimente de plusieurs facon differentes:
- depuis trois fichiers, si le schema est natif et exploite des dictionnaires avec des cles coherentes
- depuis un seule table, avec un champs json permet de reconstruire le schema a la volee par une regle de construction de table,
  impliquant des dictionnaires ayant ou non des cles, coherentes enre elles ou non

Dans la version V10 ou V11 de Khiops, un fichier de dictionnaire qui compile est necessairement utilisable pour specifier
un mapping multi-tables, avec la liste des fichier necessaire a l'alimentattion des donnees

Dans la version avec creation de table, les controles se font en deux temps
- au moment du chargement du dictionnaire, on verifie tout ce qui est possible
  - pour un dictionnaire Root, on peut verifier que le systeme de cle de son schema est coherent
  - pour les schemas non Root, on ne peut pas verifier a priori la coherence des cle, car ces dictinnaires peuvent etre utilises
    en sortie de regles de creation de table
  - on memorise en indicateur interne les dictionnaire qui sont `storable`, ceux qui sont completement coherent vis a vis des cles
- au moment du choix d'un dictionnaire d'analyse
  - on alimente la liste des fichiers selon le schema natif du dictionnaire principal, qu'il soit coherent ou non
    - cela permet ainsi de visualiser la structure native du schema
    - mais on ne genere pas en temps reel des messages d'erreur complexes, a chaque changement de choix de dictionnaire d'analyse
 - au moment du declenchement d'une action (apprentissage, analyse)
   - on genere des messages d'erreur, les plus didactiques possibles

KWClass
- IsKeyBasedStorable
  - Capacite a etre stocke sur un systeme de fichiers multi-tables a l'aide de cles
  - mise a jour par la methode IndexClass
- CheckKeyBasedStorability: verification de la capacite a etre stocke, avec emission de message d'erreur
- Check
  - appel des CheckNativeComposition avec verification des cles uniquement pour les classes Root
- CheckNativeComposition(bCheckKeys, bVerboseCheckKeys)
  - test de non recursion dans la composition
  - test de validite des cles a la demande
- InternalCheckNativeComposition: methode technique pour la gestion de la methode CheckNativeComposition
  - rappatriement de tous les controles sur la composition d'un dictionnaire et la coherence de ses cles,
    en provenance de KWClass::Check et KWAttribute::Check

KWMTDatabase
- CheckPartially
  - on verifie en prealable que le dictionnaire est stockable, avec si necessaire des messages d'erreur didactiques

Tests systematiques dans LearningTest\TestKhiops\z_TableCreationRules
Ajout de commentaires et de commentaires internes dans les dictionnaires

Impacts dans KWClass
- Set|GetLabel
  // Libelle
  // Premiere ligne prefixee par '//' precedent la declaration du dictionnaire
  // dans le fichier dictionnaire
- Set|GetComments
  // Commentaires
  // Ensemble des lignes prefixees par '//', entre le libelle et le debut de la declaration du dictionnaire
  // Par tolerance du parser, on accepte egalement tout commentaire situee apres la partie obligatoire
  // de la declaration du dictionnaire (("Root") "Dictionary" <Name>)
  // - avant la declaration de cle
  // - avant la declaration de meta-donnees
  // - avant le tout debut du bloc '{'
  // Il s'agit uniquement d'une tolerance: tous les commentaires presents avant, au milieu, ou apres la
  // declaration seront concatenes et re-ecrit apres le libelle, avant le debut de la declaration du dictionnaire
- Set|GetInternalComments
  // Commentaires internes
  // Ensemble des lignes prefixees par '//', precedents la fin du bloc '}'
  // de declaration des variables dans le fichier dictionnaire
- Impacts sur l'implementation
  - CopyFrom
  - Check
  - GetUsedMemory
  - Write
  - WriteJSONFields
Ajout de commentaires dans les variables de dictionnaires

Impacts dans KWAttribute:
- Set|GetComments
  // Commentaires
  // Lignes prefixees par '//' precedent la declaration de l'attribut dans le fichier dictionnaire
- impacts sur l'implementation
  - Clone
  - Check
  - GetUsedMemory
  - Write
  - WriteJSONFields
Ajout de commentaires et de commentaires internes dans les blocs de variables

Impacts dans KWAttributeBlock
- Set|GetComments
  // Commentaires
  // Ensemble des lignes prefixees par '//' precedent le debut du bloc d'attributs '{' dans le fichier dictionnaire
- Set|GetInternalComments
  // Commentaires internes
  // Ensemble des lignes prefixees par '//' precedent la fin du bloc d'attributs '}' dans le fichier dictionnaire
- Impacts implementation
  - KWClass::CopyFrom
  - Check
  - GetUsedMemory
  - Write
    - re-ecriture de facon similaire a celle de WriteJSONFields, pour etre reutilisee simplement par KWClass::Write
  - WriteJSONFields
== Contexte ==

Contexte: gestion des commentaires dans la version actuelle (V10)
- les commentaires sont prefixes par '//'
- on peut mettre des commentaires a peu pres partout
- pour chaque groupe de lignes de commentaire, on ne conserve que la premiere ligne de commentaire, les autres sont ignorees
- on garde les commentaires suivants, attaches aux entites du langage
  - dictionnaire: commentaire precedent le debut de declaration du dictionnaire
  - variable: commentaire de fin de declaration d'une variable
- tous les autres commentaire sont toleres, mais simplement ignores
  - n'etant rattache a aucune entite du langage, ils ne sont pas geres
  - quand on re-ecrit un dictionnaire, ils ont disparus

Besoins d'extension:
- pour une ligne de declaration de variable trop longue, on prefere parfois mettre un commentaire avant la declaration d'une variable
- besoins potentiels de commentaires multi-lignes
- pour un usage classique d'edition d'un fichier kdic, besoin de commenter tout ou partie d'un dictionnaire,
  sans perdre les specifications apres lecture/ecriture d'un fichier dictionnaire
- pour un usage de type APIU via pykhiops core dictionary, besoin de garder chaque comemntaire associe a ue entite du langage

== Specification ==

Specification
- a chaque entite d'un langage, on associe:
  - un libelle court
    - `label` actuel dans pykhiops, pour les entites Dictionary, Variable et VariableBlock
    - dans un fichier kdic, en fin de declaration, sur la meme ligne, pour les variables et les bloc de variables
    - dans la GUI: champ Label dans la liste des caracteristique d'un dictionnaire, obtenus via "Inspect dictionary"
  - un commentaire multi-lignes
    - `comments` a ajouter dans pykhiops, pour les entites ayant deja un label
      - commentaire multi-lignes
    - dans un fichier kdic, liste des commentaire lignes precedent une declaration d'entite
      - les commentaires parses sont trimes (comme les labels)
      - on peut ainsi avoir plusieurs lignes devant une variable, ou devant un dictionnaire
      - mais les lignes blanches restent ignorees
      - quand on re-ecrit un dictionnaire, on ecrit les commentaires lignes
        -  en tout debut de ligne pour les commentaires associes au Dictionary
        - au niveau de la declaration de type pour les commentaire de Variable ou VariableBlock
    - dans la GUI, on ne voit pas les commentaires
- dictionnaire
  - label: le premier commentaires ligne present avant la declaration du dictionnaire, ayant un role de titre avant les comemntaires
  - comments: tous les commentaires lignes (sauf le premier) presents avant la declaration du dictionnaire
    - tolerance du parser, pour accepter des commentaires n'importe ou entre le debut de la declaration et le debut du bloc '{'
      - avant en debut de la declaration
      - avant les elements de declaration facultatifs: cle et meta-donnees
      - avant le debug du bloc '{'
    - au moment de l'ecriture, ils sont tous ecrits avant la declaration du dictionnaire
  - internal comments: les commentaires suivant la derniere variable, avant la fin du bloc '}'
- variable
  - comments: tous les commentaires lignes presents avant la declaration de la variables
  - label: le commentaire de fin de ligne, en fin de declaration de la variable
- block de variable
  - comments: tous les commentaires lignes presents avant le debut du bloc de variable '{'
  - comments: tous les commentaires lignes presents avant la fin du bloc de variable '}'
  - label: le commentaire de fin de ligne, en fin de declaration du bloc de variables
- tout commentaire autorise par le parser sera conserve, en etant rattache au mieux a une entite du langage
- les commentaires sont interdit
  - en fin de fichier, car non rattachable a une entite du langage
  - au milieu d'une declaration de variable, ou d'une regle de derivation
  - au milieu de la partie obligatoire de la declaration de dictionnaire (("Root") "Dictionary" <Name>)

Avantages
- extension significative avec des commentaires multi-lignes
- faible cout de développement et maintenance
- on peut faire une documentation plus pedagogique, compatible avec le parser de kdic
- dictionnaires via API pykhiops:
  - ils sont tous correctement associes a une entite du langge
- dictionnaire via edition de fichier kdic
  - on garde l'exhaustivite des commentaires des fichier kdic, auparavant ignores pour partie
  - on peut commenter plusieurs variables, le temps de la mise au point
    - ces variables commentees ne seront pas perdues
    - effet de bord: leur commentaire sera associe a la variable suivante (pas grave)

== Implementation ==

Impacts principaux dans KWCLex.lex
- ajout d'une regle pour parser les commentaires
- ils sont prefixes par '//' comme les libelle, mais sont seuls sur leur ligne

Impacts principaux dans KWCYac.yac
- token existant coments renomme en label
- ajout d'un token comments de type StringVector*
- regle kwclassFile: on interdit les commentaires en fin de fichier
- regle kwclass: prise en compte des commentaires internes de fin de bloc
- regle kwclassHeader:
  - on accepte les commentaire a presque tous les endoit possible
    - au debut
    - avant les cles
    - avant les meta data
    - avant le debut de bloc '{'
  - le premier commentaire precedent le debut de declaration sert de libelle
  - ils sont tous concatenes pour etre conserve
  - les commentaires apres le debut de bloc '}' servent de InternalComments
- regle kwclassBegin: prise en compte des commentaire internes en fin des blocs d'attributs
- regle label: 0 ou 1 token LABEL
- regle comments: 0 a n tokens COMMENT
- regle labelOrComments: libelle ou commentaires
- regle kwclassBegin, cas d'un bloc d'attribut: prise en compte des comemntaire de debut de bloc
- kwattributeDeclaration: prise en compte des commentaires prededant la declaration de l'attribut

Refactoring
- Pour toutes les regles acceptant potentiellement aucun token (reperees par le commentaire /* NULL */),
  on fait desormais systematiquement passer cette option en premier au lieu de en dernier.
  Cela reduit les risques de conflit de regles, notamment pour les commentaires optionnels,
  ou le parser ne fonctionne pas sinon.

== Tests ==

Nouveaux jeux de test dans dans LearningTest\TestKhiops\Advanced
- CommentedDictionary
- CommentedMTDictionary
- CommentedSparseDictionary

Tests complets sur LearningTest
Passage systematique des commentaires de type '/* comment */' a '// comment' dans les parser de langage,
pour se conformer aux "coding guideline" du rojet (cf. https://github.com/KhiopsML/khiops/wiki/Coding-Guidelines#comments)

Impacts:
- JSONLex.lex et JSONYac.yac
- KWCLex.lex et KWCYac.yac
- il reste quelques rares lignes de style '/* comment */' dans les sections des fichiers .lex,
  la ou l'outil flex n'accepte que ce type de commentaires
@marcboulle marcboulle force-pushed the 496-extend-comment-syntax-in-khiops-dictionaries branch from 6a66ca7 to b97c43e Compare December 20, 2024 16:43
Dans les fichiers de grammaire, les regles prennent en entree des tokenq, nommes $1, $2, $3...
Pour ameliorer la lisibilite et mianteanilite du cdoe de grammaire, on renomme systematiquement
ces tokens avec des variables du bons type, et des noms parlant, en entree des portions de code
dediee aux actions de reduction des regles de grammaire.
Sauf dans les cas triviaux avec des actions en une ou deux lignes.

Impacts dans JSONYac.yac et KwcYac.yac
@marcboulle marcboulle force-pushed the 496-extend-comment-syntax-in-khiops-dictionaries branch from b97c43e to a76d4ba Compare December 20, 2024 16:49
@popescu-v
Copy link
Collaborator

s/les regles prennent en entree des tokenq, nommes $1, $2, $3... Pour ameliorer la lisibilite et mianteanilite du cdoe de grammaire,/les regles prennent en entree des tokens, nommes $1, $2, $3... Pour ameliorer la lisibilite et maintenabilite du code de grammaire,/ in the commit message of commit a76d4ba.

Copy link
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

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

Several pending typos, mostly, plus some optional suggestions.

if (GetLabel() != "")
fJSON->WriteKeyString("label", GetLabel());

// Commentaire
if (GetComments()->GetSize() > 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would discard the test and write the comments array anyway (even if empty).

@@ -842,6 +902,15 @@ void KWAttributeBlock::WriteJSONFields(JSONFile* fJSON)
parentClass->GetNextAttribute(attribute);
}
fJSON->EndArray();

// Commentaires internes
if (GetInternalComments()->GetSize() > 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idem, I would write the internalComments array anyway, even if empty. If the if is kept, then the BeginKeyArray and EndArray could be added outside the if block?

/* La completion des informations de type (CompleteTypeInfo) est centralisee */
/* au niveau du domaine en fin de parsing */

/* Commentaires internes */
if ((yyvsp[-2].svValue) != NULL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the inner brackets around yyvsp[-2].svValue?

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.

Extend comment syntax in Khiops dictionaries
2 participants