Fix to update LU parser to support phrase list flags - enabledForAllM…

…odels, disabled. (#366) * fix * Fix enabledForAllModels * Fix
microsoft · Nov 20, 2019 · fba06a0 · fba06a0
1 parent 104f81a
commit fba06a0
Show file tree

Hide file tree

Showing 12 changed files with 221 additions and 22 deletions.
diff --git a/packages/lu/docs/lu-file-format.md b/packages/lu/docs/lu-file-format.md
@@ -358,6 +358,25 @@ By default synonyms are set to be **not interchangeable** (matches with the port
     - you are
 ```
 
+Phrase lists can be marked as `disabled` using this notation: 
+
+```markdown
+@ phraselist abc disabled
+
+> also same as this
+@ phraselist question(interchangeable) = 
+    - are you
+    - you are
+
+@ question disabled
+```
+
+Phrase lists by default are enabled for all models. However when you explicitly start assigning phrase list as a feature (descriptor) to other models, then the specific phrase lists is not enabled for all models. To explicitly make a phrase list always available to all models, you can do so via:
+
+```markdown
+@ phraselist abc enabledForAllModels
+```
+
 ## Model as feature
 
 Here's how you add a feature to a ml entity or an intent - with `usesFeature`.

diff --git a/packages/lu/src/parser/index.js b/packages/lu/src/parser/index.js
@@ -8,9 +8,9 @@ const modules = {
         validateLUISBlob: require('./luis/luisValidator').validateLUIS
     },
     refresh: {
-        constructMdFromLUIS: require('./luis/luConverter').luisToLuContent,
-        constructMdFromQnA: require('./qna/qnamaker/qnaConverter').qnaToLuContent,
-        constructMdFromQnAAlteration: require('./qna/alterations/qnaConverter').qnaAlterationsToLuContent
+        constructMdFromLUIS: require('./luis/luConverter'),
+        constructMdFromQnA: require('./qna/qnamaker/qnaConverter'),
+        constructMdFromQnAAlteration: require('./qna/alterations/qnaConverter')
     },
     translate: {
         parseAndTranslate: require('./lufile/translate-helpers').parseAndTranslate,

diff --git a/packages/lu/src/parser/lu/luMerger.js b/packages/lu/src/parser/lu/luMerger.js
@@ -136,7 +136,8 @@ const haveLUISContent = function(blob) {
     (blob[LUISObjNameEnum.UTTERANCE].length > 0) ||
     (blob.prebuiltEntities.length > 0) ||
     (blob[LUISObjNameEnum.REGEX].length > 0) ||
-    (blob.model_features.length > 0) ||
+    (blob.model_features && blob.model_features.length > 0) ||
+    (blob.phraselists && blob.phraselists.length > 0) ||
     (blob.composites.length > 0));
 }
 

diff --git a/packages/lu/src/parser/lufile/parseFileContents.js b/packages/lu/src/parser/lufile/parseFileContents.js
@@ -409,7 +409,7 @@ const parseFeatureSections = function(parsedContent, featuresToProcess) {
                             addFeatures(intentExists, feature, featureTypeEnum.featureToModel, section.ParseTree.newEntityLine(), featureProperties.phraseListFeature);
                             // set enabledForAllModels on this phrase list
                             let plEnity = parsedContent.LUISJsonStructure.model_features.find(item => item.name == feature);
-                            plEnity.enabledForAllModels = false;
+                            if (plEnity.enabledForAllModels === undefined) plEnity.enabledForAllModels = false;
                         } else {
                             // de-dupe and add model to intent.
                             validateFeatureAssignment(section.Type, section.Name, entityExists.type, feature, section.ParseTree.newEntityLine());
@@ -456,7 +456,7 @@ const parseFeatureSections = function(parsedContent, featuresToProcess) {
                             addFeatures(srcEntity, feature, featureTypeEnum.featureToModel, section.ParseTree.newEntityLine(), featureProperties.phraseListFeature);
                             // set enabledForAllModels on this phrase list
                             let plEnity = parsedContent.LUISJsonStructure.model_features.find(item => item.name == feature);
-                            plEnity.enabledForAllModels = false;
+                            if (plEnity.enabledForAllModels === undefined) plEnity.enabledForAllModels = false;
                         } else {
                             // de-dupe and add model to intent.
                             validateFeatureAssignment(entityType, section.Name, featureExists.type, feature, section.ParseTree.newEntityLine());
@@ -1207,14 +1207,25 @@ const RemoveDuplicatePatternAnyEntity = function(parsedContent, pEntityName, ent
  * @param {String []} valuesList Array of individual lines to be processed and added to phrase list.
  */
 const handlePhraseList = function(parsedContent, entityName, entityType, entityRoles, valuesList, currentLine) {
+    let isPLEnabledForAllModels = undefined;
+    let isPLEnabled = undefined;
     if (entityRoles.length !== 0) {
-        let errorMsg = `Phrase list entity ${entityName} has invalid role definition with roles = ${entityRoles.join(', ')}. Roles are not supported for Phrase Lists`;
-        let error = BuildDiagnostic({
-            message: errorMsg,
-            context: currentLine
+        // Phrase lists cannot have roles; however we will allow inline definition of enabledForAllModels as well as disabled as a property on phrase list.
+        entityRoles.forEach(item => {
+            if (item.toLowerCase() === 'disabled') {
+                isPLEnabled = false;
+            } else if (item.toLowerCase() === 'enabledforallmodels') {
+                isPLEnabledForAllModels = true;
+            } else {
+                let errorMsg = `Phrase list entity ${entityName} has invalid role definition with roles = ${entityRoles.join(', ')}. Roles are not supported for Phrase Lists`;
+                let error = BuildDiagnostic({
+                    message: errorMsg,
+                    context: currentLine
+                })
+
+                throw (new exception(retCode.errorCode.INVALID_INPUT, error.toString()));
+            }
         })
-
-        throw (new exception(retCode.errorCode.INVALID_INPUT, error.toString()));
     }
     // check if this phraselist entity is already labelled in an utterance and or added as a simple entity. if so, throw an error.
     try {
@@ -1255,8 +1266,11 @@ const handlePhraseList = function(parsedContent, entityName, entityType, entityR
             if (!wordsSplit.includes(plValueItem)) pLEntityExists.words += (pLEntityExists.words !== '' ? ',' : '') + plValueItem;
         })
     } else {
-        parsedContent.LUISJsonStructure.model_features.push(new helperClass.modelObj(entityName, intc, pLValues.join(','), true));
+        pLEntityExists = new helperClass.modelObj(entityName, intc, pLValues.join(','), true);
+        parsedContent.LUISJsonStructure.model_features.push(pLEntityExists);
     }
+    if (isPLEnabled !== undefined) pLEntityExists.activated = isPLEnabled;
+    if (isPLEnabledForAllModels !== undefined) pLEntityExists.enabledForAllModels = isPLEnabledForAllModels;
 }
 
 /**

diff --git a/packages/lu/src/parser/luis/luConverter.js b/packages/lu/src/parser/luis/luConverter.js
@@ -221,7 +221,13 @@ const handlePhraseLists = function(collection) {
     }
     fileContent = '> # Phrase list definitions' + NEWLINE + NEWLINE;
     collection.forEach(function(entity) {
+        let flags = '';
         fileContent += `@ phraselist ${entity.name}${(entity.mode ? `(interchangeable)` : ``)}`;
+        if (entity.activated !== undefined && !entity.activated) flags += `disabled`;
+        if (entity.enabledForAllModels !== undefined && entity.enabledForAllModels) {
+            flags += (flags !== '') ? `, enabledForAllModels` : `enabledForAllModels`;
+        }
+        if (flags !== '') fileContent += ` ${flags}`;
         if (entity.words && entity.words !== '') {
             fileContent += ` = ${NEWLINE}\t- ${entity.words}`;
         }

diff --git a/packages/lu/src/parser/utils/helpers.js b/packages/lu/src/parser/utils/helpers.js
@@ -165,12 +165,16 @@ const helpers = {
         let v5DefFound = false;
         v5DefFound = (finalLUISJSON.entities || []).find(i => i.children || i.features) ||
                     (finalLUISJSON.intents || []).find(i => i.features) || 
-                    (finalLUISJSON.composites || []).find(i => i.features);
+                    (finalLUISJSON.composites || []).find(i => i.features) || 
+                    (finalLUISJSON.luis_schema_version === '6.0.0');
         if (v5DefFound) {
             finalLUISJSON.luis_schema_version = "6.0.0";
             if (finalLUISJSON.model_features) {
                 finalLUISJSON.phraselists = [];
-                finalLUISJSON.model_features.forEach(item => finalLUISJSON.phraselists.push(Object.assign({}, item)));
+                finalLUISJSON.model_features.forEach(item => {
+                    if (item.enabledForAllModels === undefined) item.enabledForAllModels = true
+                    finalLUISJSON.phraselists.push(Object.assign({}, item))
+                });
                 delete finalLUISJSON.model_features;
             }
             (finalLUISJSON.composites || []).forEach(composite => {

diff --git a/packages/lu/test/commands/luis/convert.test.ts b/packages/lu/test/commands/luis/convert.test.ts
@@ -186,7 +186,7 @@ describe('luis:convert', () => {
     .stderr()
     .command(['luis:convert', '--in', `${path.join(__dirname, './../../fixtures/testcases/invalid-entity-definition.lu')}`])
     .it('luis:convert writes out an error when invalid entity definition is found', async (ctx) => {
-      expect(ctx.stderr).to.contain("[ERROR] line 1:14 - line 1:16: syntax error: missing ':' at '\\r\\n'")
+      expect(ctx.stderr).to.contain("syntax error: missing ':'")
     })
 
     test
@@ -424,6 +424,13 @@ describe('luis:convert version 5 upgrade test', () => {
   .it('luis:convert successfully converts LUIS JSON model with no phrase lists (output must have phraselists if any v6 concepts are present in the .lu file)', async () => {
     expect(await compareLuFiles('./../../../results/root38.json', './../../fixtures/verified/v6WithoutPhraseLists.json')).to.be.true
   })
+
+  test
+  .stdout()
+  .command(['luis:convert', '--in', `${path.join(__dirname, './../../fixtures/testcases/plWithFlags.lu')}`, '--out', './results/root39.json'])
+  .it('luis:convert successfully converts LUIS JSON model with no phrase lists (output must have phraselists if any v6 concepts are present in the .lu file)', async () => {
+    expect(await compareLuFiles('./../../../results/root39.json', './../../fixtures/verified/plWithFlags.json')).to.be.true
+  })
 })
 
 describe('luis:convert negative tests', () => {

diff --git a/packages/lu/test/fixtures/testcases/plWithFlags.lu b/packages/lu/test/fixtures/testcases/plWithFlags.lu
@@ -0,0 +1,14 @@
+> phrase list as feature to intent (also applicable to entities)
+@ intent getUserProfileIntent usesFeature city
+
+# getUserProfileIntent
+- test
+
+@ phraselist city
+@ city =
+    - Seattle
+    - SEATAC
+    - SEA
+
+@ city enabledForAllModels
+@ city disabled
diff --git a/packages/lu/test/fixtures/verified/plWithFlags.json b/packages/lu/test/fixtures/verified/plWithFlags.json
@@ -0,0 +1,42 @@
+{
+  "intents": [
+    {
+      "name": "getUserProfileIntent",
+      "features": [
+        {
+          "featureName": "city",
+          "featureType": "phraselist"
+        }
+      ]
+    }
+  ],
+  "entities": [],
+  "composites": [],
+  "closedLists": [],
+  "regex_entities": [],
+  "regex_features": [],
+  "utterances": [
+    {
+      "text": "test",
+      "intent": "getUserProfileIntent",
+      "entities": []
+    }
+  ],
+  "patterns": [],
+  "patternAnyEntities": [],
+  "prebuiltEntities": [],
+  "luis_schema_version": "6.0.0",
+  "phraselists": [
+    {
+      "name": "city",
+      "words": "Seattle,SEATAC,SEA",
+      "mode": false,
+      "activated": false,
+      "enabledForAllModels": true
+    }
+  ],
+  "versionId": "0.1",
+  "name": "",
+  "desc": "",
+  "culture": "en-us"
+}
diff --git a/packages/lu/test/fixtures/verified/v5Upgrade.json b/packages/lu/test/fixtures/verified/v5Upgrade.json
@@ -44,7 +44,8 @@
       "name": "abc",
       "words": "one,two,three",
       "mode": false,
-      "activated": true
+      "activated": true,
+      "enabledForAllModels": true
     }
   ],
   "versionId": "0.1",

diff --git a/...rseFileContents.@EntityDefinition.test.js → ...eFileContents.NewEntityDefinition.test.js b/...rseFileContents.@EntityDefinition.test.js → ...eFileContents.NewEntityDefinition.test.js
@@ -2,7 +2,7 @@
  * Copyright (c) Microsoft Corporation. All rights reserved.
  * Licensed under the MIT License.
  */
-const parseFile = require('./../../../src/parser/lufile/parseFileContents');
+const parseFile = require('../../../src/parser/lufile/parseFileContents');
 var chai = require('chai');
 var assert = chai.assert;
 describe('V2 Entity definitions using @ notation', function () {
@@ -460,6 +460,97 @@ describe('V2 Entity definitions using @ notation', function () {
     });
 
     describe('Phrase lists are handled correctly', function(done){
+        it('Phrase lists can be marked as disabled inline', function(done){
+            let luFile = `
+                @ phraselist city disabled
+                @ city =
+                    - Seattle
+                    - SEATAC
+                    - SEA
+            `;
+
+            parseFile.parseFile(luFile)
+                .then(res => {
+                    assert.equal(res.LUISJsonStructure.model_features.length, 1);
+                    assert.equal(res.LUISJsonStructure.model_features[0].name, 'city');
+                    assert.equal(res.LUISJsonStructure.model_features[0].activated, false);
+                    done();
+                })
+                .catch(err => done(err))
+        });
+
+        it('Phrase lists can be marked as disabled in a separate line', function(done){
+            let luFile = `
+                @ phraselist city
+                @ city =
+                    - Seattle
+                    - SEATAC
+                    - SEA
+                @ city disabled
+            `;
+
+            parseFile.parseFile(luFile)
+                .then(res => {
+                    assert.equal(res.LUISJsonStructure.model_features.length, 1);
+                    assert.equal(res.LUISJsonStructure.model_features[0].name, 'city');
+                    assert.equal(res.LUISJsonStructure.model_features[0].activated, false);
+                    done();
+                })
+                .catch(err => done(err))
+        });
+
+        it('Phrase lists can be marked as enabled for all models in a separate line', function(done){
+            let luFile = `
+                > phrase list as feature to intent (also applicable to entities)
+                @ intent getUserProfileIntent usesFeature city
+                
+                # getUserProfileIntent
+                - test
+                
+                @ phraselist city
+                @ city =
+                    - Seattle
+                    - SEATAC
+                    - SEA
+                
+                @ city enabledForAllModels
+                @ city disabled
+            `;
+
+            parseFile.parseFile(luFile)
+                .then(res => {
+                    assert.equal(res.LUISJsonStructure.phraselists.length, 1);
+                    assert.equal(res.LUISJsonStructure.phraselists[0].name, 'city');
+                    assert.equal(res.LUISJsonStructure.phraselists[0].activated, false);
+                    done();
+                })
+                .catch(err => done(err))
+        });
+
+        it('Phrase lists can be marked as enabled for all models inline', function(done){
+            let luFile = `
+                > phrase list as feature to intent (also applicable to entities)
+                @ intent getUserProfileIntent usesFeature city
+                
+                # getUserProfileIntent
+                - test
+                
+                @ phraselist city disabled, enabledForAllModels = 
+                    - Seattle
+                    - SEATAC
+                    - SEA
+            `;
+
+            parseFile.parseFile(luFile)
+                .then(res => {
+                    assert.equal(res.LUISJsonStructure.phraselists.length, 1);
+                    assert.equal(res.LUISJsonStructure.phraselists[0].name, 'city');
+                    assert.equal(res.LUISJsonStructure.phraselists[0].activated, false);
+                    done();
+                })
+                .catch(err => done(err))
+        });
+
         it('Basic phrase list definition is handled correctly', function(done) {
             let luFile = `
                 @phraselist xyz
@@ -469,7 +560,7 @@ describe('V2 Entity definitions using @ notation', function () {
                 .then(res => {
                     assert.equal(res.LUISJsonStructure.model_features.length, 1);
                     assert.equal(res.LUISJsonStructure.model_features[0].name, 'xyz');
-                    done();assert.equal(res.LUISJsonStructure.model_features[0].name, 'xyz');
+                    done();
                 })
                 .catch(err => done(err))
         });

diff --git a/packages/lu/test/parser/lufile/parseFileContents.modelAsFeature.test.js b/packages/lu/test/parser/lufile/parseFileContents.modelAsFeature.test.js
@@ -219,7 +219,7 @@ describe('Model as feature definitions', function () {
                         assert.equal(res.LUISJsonStructure.phraselists[1].mode, true);
                         assert.equal(res.LUISJsonStructure.phraselists[1].words, 'portland,PDX');
                         assert.equal(res.LUISJsonStructure.phraselists[1].activated, true);
-                        assert.equal(res.LUISJsonStructure.phraselists[1].enabledForAllModels, undefined);
+                        assert.equal(res.LUISJsonStructure.phraselists[1].enabledForAllModels, true);
                         done();
                     })
                     .catch(err => done(err))
@@ -298,7 +298,7 @@ describe('Model as feature definitions', function () {
                         assert.equal(res.LUISJsonStructure.phraselists[1].mode, true);
                         assert.equal(res.LUISJsonStructure.phraselists[1].words, 'portland,PDX');
                         assert.equal(res.LUISJsonStructure.phraselists[1].activated, true);
-                        assert.equal(res.LUISJsonStructure.phraselists[1].enabledForAllModels, undefined);
+                        assert.equal(res.LUISJsonStructure.phraselists[1].enabledForAllModels, true);
                         done();
                     })
                     .catch(err => done(err))
@@ -463,7 +463,7 @@ describe('Model as feature definitions', function () {
                         assert.equal(res.LUISJsonStructure.phraselists[1].mode, true);
                         assert.equal(res.LUISJsonStructure.phraselists[1].words, 'portland,PDX');
                         assert.equal(res.LUISJsonStructure.phraselists[1].activated, true);
-                        assert.equal(res.LUISJsonStructure.phraselists[1].enabledForAllModels, undefined);
+                        assert.equal(res.LUISJsonStructure.phraselists[1].enabledForAllModels, true);
                         done();
                     })
                     .catch(err => done(err))