LuisRecognizer can't handle null or empty text

[v-kydela]

Version

4.2

Describe the bug

When an activity with no text is sent to LUIS an ArgumentNullException is thrown. This can be seen in the RecognizeInternalAsync method in LuisRecognizer.cs.

To Reproduce

Steps to reproduce the behavior:

1. Run a bot that calls LuisRecognizer.RecognizeAsync
2. Send a message with no text to the bot
3. See error

Expected behavior

Originally I had thought that a message with no text should just produce a "None" intent, but then I tested with the API and discovered that an empty query produces a LUIS result with no intent at all. The query property is null, the intents and entities properties are empty arrays, and there is no topScoringIntent property. I began to question whether we can even assume a LUIS app has a None intent, though I've tested in the LUIS portal and found that the None intent can't be deleted. I think we're facing a bit of a design dilemma when it comes to how to fix this. It seems to me that there are a few options.

1. Assume the app has a None intent and have LuisRecognizer create a fake result with a None intent when it receives an activity with no text.
2. Send an empty query to LUIS and handle the LUIS result with no intents. This may change the design of the SDK in ways that are hard to predict, because it's currently assumed that RecognizeAsync will always return at least one intent. We'd need to make sure none of the LUIS code in the SDK would break if a LuisResult has no intents, and bots would need to make sure to account for LUIS results with no intents.
3. Leave the SDK unmodified and expect bots to make sure they don't send empty utterances to LUIS. This may be the easiest option as far as the SDK is concerned and it wouldn't put any more responsibility on bots than option 2. But like option 2, it would require modification of the samples as the bots I've seen do not generally check if an activity's text is null before sending it to LUIS.

Screenshots

Additional context

Continued in comments to keep this post concise...

[bug]

[v-kydela]

This issue is an extension of #1211 which I left intentionally open-ended since I didn't know if I was missing anything. Since then I've done a search in the SDK and I've discovered that QnAMaker.cs also follows the pattern of not allowing empty text. I haven't tested it but based on the source code I think it's safe to assume the same behavior can be observed in QnAMaker bots. If that bug does exist, I expect that it would be easy to fix since QnAMaker always has to account for questions that don't have corresponding answers anyway.

---

I know of two ways to send textless messages to a bot, though this is of course dependent on channel capabilities. You can send an attachment (like an image) or you can click on an Adaptive Card's submit button. One could make the case that every bot should know not to send those kinds of messages to LUIS etc. and that if a message does end up going to LUIS and LUIS returns a None intent, the user won't know what's going on and the bot developer might have to open an issue anyway. All the same, I'm currently thinking that having LUIS return a None intent for textless messages may be the best and most user-friendly option since there's not necessarily any need to throw an exception in that case.

[garypretty]

@v-kydela I agree that returning the None intent makes the most sense here. At least for now. My understanding is that all LUIS models do indeed need to contain a None intent, so this seems like a straightforward fix from my perspective. At least it doesn't produce any breaking / unexpected behaviour which means we have the option of changing the way things work in the future, but fixing the immediate issue now.

[cleemullins]

@cahann I'm not sure what the correct behavior should be. We can obviously "Fake" the "None" intent locally, but I'll leave that decision in your hands.

[EricDahlvang]

In BotBuilder dotnet V3, we return an empty intent if the text property is empty: https://github.com/Microsoft/BotBuilder-V3/blob/master/CSharp/Library/Microsoft.Bot.Builder/Dialogs/LuisDialog.cs#L264

[cahann]

we don't return an intent cause there is no prediction score, anything that we will return will be fake

[moabba]

I think the current behavior makes sense. The user didn't send a query, then how would it have an intent? It's like a dividing by zero scenario. @chrimc62 What do you think about this?

[v-kydela]

@chrimc62 - Have you had a look at this yet?

[CoHealer]

@moabba, by "I think the current behavior makes sense. The user didn't send a query, then how would it have an intent? It's like a dividing by zero scenario." are you referring to the v3 behavior or v4? Because v4 throws an error which results in a potential support issue.

[v-kydela]

If it's like a dividing by zero scenario, how come the LUIS endpoint doesn't return an error code when an empty query is sent?

Supposing we do still want to throw an exception when the turn context's activity has no text, we should still modify it a bit. We're throwing an ArgumentNullException even though the argument isn't null. It's not even a property of an argument that's null. We're throwing an exception based on a property of a property of an argument. I think this is sort of unusual and unexpected, and it's likely to confuse developers who are using the framework. If we're going to throw an exception, it should surely contain more information about why the exception is being thrown, since "utterance" isn't even an argument.

[CoHealer]

There doesn't seem to be any recommendation on this. We'd like to move forward with implementing the same behavior as is in V3. @v-kydela, please put a PR together and include the discussion participants as reviewers. Thanks.

[v-kydela]

In my pull request, the solution I'm going with follows after the behavior of V3. It's sort of a combination of options 1 and 2. Instead of faking a None intent, I'm faking an empty intent. This makes sense to me because if it was None then it would be hard to tell the difference between a None intent that came from the LUIS endpoint and a None intent that didn't.

At first I tried just sending the empty query to the LUIS endpoint and that almost worked but it encountered an exception in the LuisDelegatingHandler after the call. Then I thought it's better to not make an endpoint call anyway since that has a cost associated with it.

[v-kydela]

Surprisingly, I just noticed that the Node SDK doesn't have this problem. So this PR will bring the .NET SDK closer to the Node SDK.