Update IRecognizer and LUIS to support strongly typed results #477

chrimc62 · 2018-04-20T00:48:00Z

…xtensions. They are intended to support the new schema model for recognizers.

Revamp the LUIS transform to the new schema to include all built-in types.
Add Recognize and classes to support generation of strongly typed LUIS results.
LuisResult is now added as an extended property on RecognizerResult.
Tests of the corresponding functionality.

…xtensions. They are intended to support the new schema model for recognizers. Revamp the LUIS transform to the new schema to include all built-in types. Add Recognize and classes to support generation of strongly typed LUIS results. LuisResult is now added as an extended property on RecognizerResult. Tests of the corresponding functionality.

emadelwany · 2018-04-20T02:17:25Z

I love the removal of ILuisRecognizer, it was really bothering me! Thanks :-)

emadelwany · 2018-04-20T01:44:50Z

libraries/Microsoft.Bot.Builder.Ai.LUIS/Generator/NumberWithUnit.cs

+        /// <summary>
+        /// Normalized recognized unit.
+        /// </summary>
+        [JsonProperty("unit")]


Should this be "units"? Or should the property be "Unit"?

Changed to be number and units.

emadelwany · 2018-04-20T01:45:52Z

libraries/Microsoft.Bot.Builder.Ai.LUIS/LuisRecognizer.cs

-        /// </summary>
-        /// <param name="luisModel">The LUIS model to use to recognize text.</param>
-        /// <param name="luisRecognizerOptions">The LUIS recognizer options to use.</param>
-        /// <param name="options">The LUIS request options to use.</param>
        public LuisRecognizer(ILuisModel luisModel, ILuisRecognizerOptions luisRecognizerOptions = null, ILuisOptions options = null)


Any specific reason for removing the docs on the ctor()?

Added back and also did inheritdoc on the strongly typed recognize and made the CallAndRecognize private.

emadelwany · 2018-04-20T01:46:57Z

libraries/Microsoft.Bot.Builder.Ai.LUIS/LuisRecognizer.cs

        public async Task<RecognizerResult> Recognize(string utterance, CancellationToken ct)
        {
-            var result = await CallAndRecognize(utterance, ct).ConfigureAwait(false);
-            return result.recognizerResult;
+            return (await CallAndRecognize(utterance, ct).ConfigureAwait(false));
        }


return CallAndRecognize(); without awaiting, also now that CallAndRecognize() returns one thing (awesome) I think RecognizeInternal is a better name (the old name).

emadelwany · 2018-04-20T01:51:51Z

libraries/Microsoft.Bot.Builder.Ai.LUIS/LuisRecognizer.cs

+        public async Task<T> Recognize<T>(string utterance, CancellationToken ct)
+        {
+            return (T) Activator.CreateInstance(typeof(T), (await CallAndRecognize(utterance, ct).ConfigureAwait(false)));
+        }


This is an interesting way to implement this, it means thought that any type T must have a constructor that accepts a RecognizereResults object, right? I don't think that's an ideal design, I don't see why developers should tie their schema to ours?

If you go with this implementation, it'd be good to make it self documenting using Types, we should have a constraint on T that restricts it to an abstract class that has a ctor(RecognizeResults). But I think we'd be putting too much work on people who are implementing these converters instead of creating them for them.

This was the least evil alternative Tom and I came up with. You either couple on serialization or a constructor on the generic version. The strongly typed Recognize makes it discoverable. The question is how to get that constructed. Did you have a different idea on how to do so?

I think what we are really strongly typing is Intents and Entities properties (the JObjects).
The method could be written as such:

public interface ITypedRecognizerResult<TIntents, TEntiies> { string Text {set; get;} TIntents Intents; TEntiies Entities; }

public Task<TResult> Recognize<TResult, TIntents,TEntities>(string utterance) where TResult: ITypedRecognizerResult<TIntents, TEntities>, new() { var rr = await CallAndRecognize(); var typedRR = new TResult { Text = rr.Text, Intents = rr.Intents as TIntents, Entities = rr.Entities as TEntities }; return Task.FromResult(typedRR); }

The beauty of this implementation is that it's not LUIS specific and can be shared across all recognizers, that's why i think it should actually be part of RecognizerResults class and not every specific Recognizer:

public class RecognizerResults { public TResult As<TResult, TIntents, TEntities>() where TResult: ITypedRecognizeResult<TIntents, TEntities>, new() { return new TResult { Text = this.Text, Intents = this.Intents as TIntents, Entities = this.Entities as TEntities }; } }

And it can then be used this way:

var rr = luisRecognize.Recognize(); var typedRr = rr.As<LUIS.Contoso_App>();

The only constraint that it adds on the schema writer is that their schema must extend ITypedRecognizerResult. If my Schema is:

class MyStrongType { MyIntents Intents; MyEntities Entities; string text; }

All I have to do is:

If I can modify my schema file:

class MyStrongType : ITypedRecognizedResult<MyIntents, MyEntities>{ ... }

If I don't have access to to the type source code or it's already compiled:

class MyStrongTypeRecognizedResult: MyStrongType, ITTypedRecognizedResult<MyIntents, MyEntities> { ... }

There's a lot of complexity here introduced because part of RecognizeResult is Typed (text, alteredText etc) and part isn't (intents, entities), my proposal will be MUCH simpler if RecognizerResult is just a JObject that happens to have a text/alteredText property, then we don't need any of these interfaces and the implementation of As() would be simply:

return rr as T;

My #1 preference would be the latter (i.e. just use JObject all the way for untyped, especially that we now have a way to Type it)

My #2 preference is the former.

And I'm ok with my current implementation too as long as we're aware of its limitation...

OK, I changed this to an interface with type constraints.

Emad's solution here is very interesting and seems broadly applicable.

emadelwany · 2018-04-20T02:19:48Z

libraries/Microsoft.Bot.Builder.Ai.LUIS/LuisRecognizer.cs

+            {
+                return null;
+            }
+            return long.TryParse((string)value, out var longVal) ?


I thought you wanted to get rid of this and always return double? I'm ok with keeping it

I map to double in the strong conversion--fine to leave it either here in JSON.

emadelwany · 2018-04-20T02:28:41Z

libraries/Microsoft.Bot.Builder.Core.Extensions/RecognizerResult.cs

+    /// <summary>
+    /// Recognizer return value.
+    /// </summary>
+    public class RecognizerResult


I added JsonProperties to that class with camelCasing that seem to have been lost.

1) Added IRecognizerConvert interface for strong types. 2) Renamed CallAndRecognize 3) Added some missing JSonProperty and xmldocs.

cleemullins · 2018-04-20T16:47:54Z

Microsoft.Bot.Builder.sln

-		{183B3324-4CFF-477A-8EAE-73953D3E383D}.Debug - NuGet Packages|Any CPU.ActiveCfg = Debug - NuGet Packages|Any CPU
-		{183B3324-4CFF-477A-8EAE-73953D3E383D}.Debug - NuGet Packages|Any CPU.Build.0 = Debug - NuGet Packages|Any CPU
+		{183B3324-4CFF-477A-8EAE-73953D3E383D}.Debug - NuGet Packages|Any CPU.ActiveCfg = Debug|Any CPU
+		{183B3324-4CFF-477A-8EAE-73953D3E383D}.Debug - NuGet Packages|Any CPU.Build.0 = Debug|Any CPU


We need to make sure the Release configurations are setup as well. Should be trivial...

cleemullins · 2018-04-20T16:49:41Z

libraries/Microsoft.Bot.Builder.Ai.LUIS/Generator/DateTimeExpression.cs

+
+        public DateTimeSpec(string type, IEnumerable<string> expressions)
+        {
+            Type = type;


Argument Null checking. (Public method of a public class)

if (string.isNullOrWhitespace(type)) throw new ArgumentNullException(nameof(type)) if (expressions == null) throw new ArgumentNullException(nameof(expressions))

cleemullins · 2018-04-20T16:51:00Z

libraries/Microsoft.Bot.Builder.Ai.LUIS/Generator/DateTimeExpression.cs

+        /// Type of expression.
+        /// </summary>
+        [JsonProperty("type")]
+        public readonly string Type;


I would prefer to avoid the word type. Using Language Keywords (and Type is C# has a zillion meanings) is often problematic in the long term. It's bad enough we do this on Activity.Type, I would prefer to avoid making the problem worse.

I don't have a better word, so leaving for now.

cleemullins · 2018-04-20T16:51:13Z

libraries/Microsoft.Bot.Builder.Ai.LUIS/Generator/DateTimeExpression.cs

+    public class DateTimeSpec
+    {
+        /// <summary>
+        /// Type of expression.


As code comments go, this one could use a bit of work. :)

Added reference to GitHub and some comments on type.

cleemullins · 2018-04-20T17:55:02Z

libraries/Microsoft.Bot.Builder.Ai.LUIS/Generator/InstanceData.cs

+    /// Strongly typed information corresponding to LUIS $instance value.
+    /// </summary>
+    /// <remarks>
+    /// This is a partial class in order to support adding custom meta-data.


metadata is one word. I think.

cleemullins · 2018-04-20T18:30:52Z

libraries/Microsoft.Bot.Builder.Ai.LUIS/LuisRecognizerMiddleware.cs

@@ -36,7 +38,7 @@ public class LuisRecognizerMiddleware : IMiddleware
        /// </summary>
        public const string Obfuscated = "****";


The pattern of using this "Obfuscated" is really odd and just sets off all kind of weird red-flags.

We removed it from the QnA Maker components

Added bug for Emad.

@cleemullins it's just a string constant to indicate that the key property includes sensitive data, could be null or empty if you prefer, what's the "pattern" and red flags just so I would understand? We aren't really "obfuscating" anything or editing any existing data structures. This "****" string goes to a brand new data structure.

I could introduce a new data structure that's identical to the existing one and not include the key property, but I was trying to keep the diagnostic info mapped as much as possible to existing data structures to 1. enforce the schema and 2. avoid introducing a new data structure that the developer will have to map back to the existing schema. We need all the data in the existing data structure so there's no point in introducing a new one.

cleemullins · 2018-04-20T18:32:12Z

libraries/Microsoft.Bot.Builder.Core.Extensions/IRecognizer.cs

+    /// <summary>
+    /// Interface for Recognizers.
+    /// </summary>
+    public interface IRecognizer


This is a nice basic definition for a recognizer.

cleemullins · 2018-04-20T18:33:10Z

libraries/Microsoft.Bot.Builder.Core.Extensions/Microsoft.Bot.Builder.Core.Extensions.csproj

@@ -58,6 +54,7 @@
  <ItemGroup>
    <PackageReference Include="Microsoft.CSharp" Version="4.4.1" />
    <PackageReference Include="Newtonsoft.Json" Version="10.0.3" />
+    <PackageReference Include="System.Dynamic.Runtime" Version="4.3.0" />


I would STRONGLY like to avoid pulling in system.dynamic to the Extensions package.

How can we avoid doing this?

Hmmm. I removed it and it seems to work fine--even though there are some dynamics in here.

cleemullins · 2018-04-20T18:34:16Z

libraries/Microsoft.Bot.Builder.Core.Extensions/RecognizerResult.cs

+        /// <summary>
+        /// Object with the intent as key and the confidence as value.
+        /// </summary>
+        public JObject Intents { get; set; }


I'm not nearly as comfortable with the Recognizer Result here. Returning JObjects from our core SDK seems like a big abstraction leak. Let's sync on this before we commit.

cleemullins · 2018-04-20T18:35:18Z

libraries/Microsoft.Bot.Builder.Core.Extensions/RecognizerResult.cs

+        /// Shallow copy constructor.
+        /// </summary>
+        /// <param name="other">Result to copy results from.</param>
+        public RecognizerResult(RecognizerResult other)


Is this useful? Seems it might introduce more bugs than it would prevent.

I would have a real copy constructor (that round-trips through the JSON to clone) or nothing. This approach seems like it's going to end badly.

Changed EndIndex to be +1 to better match language conventions. Beefed up DateTimeExpression robustness and docs. Moved AlteredText back to null. Minor renaming.

emadelwany

Thanks Chris!

…crosoft#477) * [QnA Maker] IsTest and Ranker type support for QnAMaker.GetAnswer * Formatting fix * Formatting fix

chrimc62 requested a review from emadelwany April 20, 2018 00:48

emadelwany suggested changes Apr 20, 2018

View reviewed changes

Address feedback.

3ab827e

1) Added IRecognizerConvert interface for strong types. 2) Renamed CallAndRecognize 3) Added some missing JSonProperty and xmldocs.

chrimc62 requested a review from cleemullins April 20, 2018 18:22

cleemullins requested changes Apr 20, 2018

View reviewed changes

Address Chris Mullins comments.

8e8eca1

Changed EndIndex to be +1 to better match language conventions. Beefed up DateTimeExpression robustness and docs. Moved AlteredText back to null. Minor renaming.

cleemullins mentioned this pull request Apr 20, 2018

Change some scopes, per issue #472 #484

Merged

cleemullins approved these changes Apr 20, 2018

View reviewed changes

emadelwany approved these changes Apr 21, 2018

View reviewed changes

Merge branch 'master' into chrimc

b228ba9

chrimc62 merged commit 7d89262 into master Apr 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update IRecognizer and LUIS to support strongly typed results #477

Update IRecognizer and LUIS to support strongly typed results #477

chrimc62 commented Apr 20, 2018

emadelwany commented Apr 20, 2018

emadelwany Apr 20, 2018

chrimc62 Apr 20, 2018

emadelwany Apr 20, 2018

chrimc62 Apr 20, 2018

emadelwany Apr 20, 2018

emadelwany Apr 20, 2018

chrimc62 Apr 20, 2018

emadelwany Apr 20, 2018 •

edited

Loading

chrimc62 Apr 20, 2018

cleemullins Apr 20, 2018

emadelwany Apr 20, 2018

chrimc62 Apr 20, 2018

emadelwany Apr 20, 2018

chrimc62 Apr 20, 2018

cleemullins Apr 20, 2018

cleemullins Apr 20, 2018

chrimc62 Apr 20, 2018

cleemullins Apr 20, 2018

chrimc62 Apr 20, 2018

cleemullins Apr 20, 2018

chrimc62 Apr 20, 2018

cleemullins Apr 20, 2018

chrimc62 Apr 20, 2018

cleemullins Apr 20, 2018

chrimc62 Apr 20, 2018

emadelwany Apr 21, 2018

cleemullins Apr 20, 2018

cleemullins Apr 20, 2018

chrimc62 Apr 20, 2018

cleemullins Apr 20, 2018

cleemullins Apr 20, 2018

emadelwany left a comment

		@@ -36,7 +38,7 @@ public class LuisRecognizerMiddleware : IMiddleware
		/// </summary>
		public const string Obfuscated = "****";

Update IRecognizer and LUIS to support strongly typed results #477

Update IRecognizer and LUIS to support strongly typed results #477

Conversation

chrimc62 commented Apr 20, 2018

emadelwany commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emadelwany Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emadelwany left a comment

Choose a reason for hiding this comment

emadelwany Apr 20, 2018 •

edited

Loading