Fix bug that whether an Orchestrator object can extract entities or n…

[hcyang]

…ot should not depend on a member variable ScoreEntities.

Fixes #5750

Description

Specific Changes

Testing

[coveralls]

Pull Request Test Coverage Report for Build 255773

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 8 unchanged lines in 4 files lost coverage.
• Overall coverage increased (+0.04%) to 76.202%

---

Files with Coverage Reduction	New Missed Lines	%
/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Recognizers/EntityRecognizers/TextEntityRecognizer.cs	1	85.71%
/libraries/Microsoft.Bot.Builder.Dialogs/Prompts/DatetimePrompt.cs	1	77.5%
/libraries/Microsoft.Bot.Builder/Adapters/TestFlow.cs	2	78.32%
/libraries/Microsoft.Bot.Builder.Dialogs/Memory/DialogStateManager.cs	4	78.76%

Totals
Change from base Build 255298:	0.04%
Covered Lines:	22952
Relevant Lines:	30120

---

💛 - Coveralls

[tsuwandy]

@hcyang, @daveta and I reviewed the changes again and not sure why we need the extra layer, OrchestratorDictionaryEntry?

[hcyang]

@hcyang, @daveta and I reviewed the changes again and not sure why we need the extra layer, OrchestratorDictionaryEntry?

@tsuwandy, the removed ScoreEntities flag is renamed IsEntityReady and placed in an OrchestratorDictionaryEntry and associated with a static Orchestrator object. If there is no need for the ScoreEntities flag, then we can remove that IsEntityReady flag and remove the whole OrchestratorDictionaryEntry abstraction as well.

[daveta]

Seems like the current code and this fix are equivalent (?). The difference is the new code, you have available the Orchestrator more readily available in the Recognizer instance (but I don't see it being used).

[carlosscastro]

Hello! Some of the general comments I added are me just catching up and asking questions about this codebase and not issues with this PR in particular. I did find 2 things that we should fix before merging as I was learning by reading it, which I wanted to call out:

1- We cannot remove public properties that shipped
2- We should minimize visibility of classes wherever possible. I'm not sure that OrchestratorDictionaryEntry needs to be protected, since as such we'd need to maintain backward compat until V5 at least

Happy to help with these, and we can chat about the other comments offline and not block this PR on them.

The easiest way to fix the 2 most blocking issues is 1) do not remove the public property, instead mark it obsolete with a reasonable explanation of what to do moving forward for developers and 2) make the new entry class private instead of protected

[carlosscastro]

Since we are visiting this codebase and adding some restructuring, it would be nice, if possible, to add some comments and explain what is the json format, what we are trying to extract, and other decisions that are not obvious in the code.

[tsuwandy]

@carlosscastro, it's modeled after https://github.com/microsoft/botbuilder-dotnet/blob/402bc02b4cbbd2f4ec359134640e99211367e4a5/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Recognizers/EntityRecognizers/EntityRecognizer.cs

[carlosscastro]

Hung-chih and I chatted. The Entry class is going away, and we're discussing the backward compat issue. The rest of the comments from pre-existing code can be addressed / analyzed outside of the scope of this PR.

[hcyang]

Hung-chih and I chatted. The Entry class is going away, and we're discussing the backward compat issue. The rest of the comments from pre-existing code can be addressed / analyzed outside of the scope of this PR.

@carlosscastro, after consulting with @daveta, we decided to bring the DictionaryEntry class back, but make it a private class per your suggeston.

[carlosscastro]

What's the latency of this method on moderate sized models?

[hcyang]

The latency for that is just seconds. Loading the models are quick even though they can be up to 1GB. The actual time consuming part is building the resolver consuming some big snapshot file. Perhaps that's the reason the InitializeModel() function is called once inside RecognizeAsync(), which is async.

[carlosscastro]

what is the latency of the entire initializeModel?

[carlosscastro]

Asking because 1) this will get run on the first turn and not when the app service starts. This means that the first turn of the first conversation will be really slow, i.e. as long as this method takes.

[carlosscastro]

Ideally we'd want to transfer this time to application load somehow in the future. We don't need to resolve this now, but it is importnat. I'll create an issue for this.

[carlosscastro]

Thanks for the iteration @hcyang !!!