-
Notifications
You must be signed in to change notification settings - Fork 487
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
Change AdaptiveBotDialog to fix 5 issues and use DialogManager internally #5389
Conversation
…se DialogManager * No perf regressions * No duplicated code * Root dialogs collection still there * Naming aligned.
libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/AdaptiveDialogManager.cs
Outdated
Show resolved
Hide resolved
/// <param name="turnContext">turn context.</param> | ||
/// <param name="cancellationToken">cancellationToken.</param> | ||
/// <returns>task.</returns> | ||
Task IBot.OnTurnAsync(ITurnContext turnContext, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant and can be removed, DialogManager is an IBot and it implicitly implements OnTurnAsync below, there is no need for an explicit interface implementation here, and IBot will be able to access the OnTurnAsync method defined below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the signature is different <gah>
IBot defines OnTurjn as returning Task
Existing method returns Task<DialogManagerResult>()
/// <summary> | ||
/// An <see cref="IBot"/> implementation that manages the execution of an <see cref="AdaptiveDialog"/>. | ||
/// </summary> | ||
public class AdaptiveDialogManager : DialogManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a bit fan of the AdaptiveDialogBot name but I think it better describes that this is a bot that works with adaptive dialogs. AdaptiveDialogManager does not describe itselft as a Bot unless you read the implementation.
libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/AdaptiveDialogManager.cs
Outdated
Show resolved
Hide resolved
@@ -116,13 +116,24 @@ public Dialog RootDialog | |||
/// </value> | |||
public int? ExpireAfter { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this property? It doesn't seem to be used anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows you to say that conversations expire after an hour. ConversationState will be cleared if no interactions for an hour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Check for expired conversation
if (ExpireAfter.HasValue && (DateTime.UtcNow - lastAccess) >= TimeSpan.FromMilliseconds((double)ExpireAfter))
{
// Clear conversation state
await ConversationState.ClearStateAsync(context, cancellationToken).ConfigureAwait(false);
}
lastAccess = DateTime.UtcNow;
await lastAccessProperty.SetAsync(context, lastAccess, cancellationToken).ConfigureAwait(false);
OK. So Consolidation == good. The fact that RunAsync() was in there and had forked logic was bad, and I wish that hadn't happened, because looking at the code it appears that dialogContext.DialogManager may or may not be there, which is also bad, as we don't know who is depending on it being there.
So I have 5 issues as current coded in main that need to be addressed
1. We broke DialogContext.DialogManager property
We switched to using dialog.RunAsync() which does not set dialogContext.DialogManager and so it will always be null, because we don't use the dialog manager class any more. That worries me. That is a change in behavior
2. We have introduced a performance regression
by awaiting in sequence conversation/userstate SaveChanges() calls. This is a change of behavarior and a perf regression. This is easily addressed by using a BotStateSet.SaveAllChanges() as it will save them in parallel as designed.
3. We have introduced a performance regression
by loading the root dialog on every turn. That essentially loads all resources, on each turn.
4. We have improperly registered LanguageGenerators
The simplification of adding the LanguageGenerator was lost in translation. The dialogManager.UseLanguageGeneraion() adds the resource to the turnState as (), the new code does not.
5. LanguagePolicy should be dI
We abstracted away the language Policy which is something that can be created and defined via dependency injection.
My PR fixes these 5 issues so that we can do the refactoring that John wants.
#1, #2, #4 are fixed by using the dialogManager class as a private encapsulated instance.
#3 is easily fixed by loading the root dialog in the constructor, not on each turn.
#5 is just adding a new argument to the ctor.
As you see 3 of these bugs would not have happened at all if we just kept the existing dialog Manager, and using the DialogManager is internal implementation detail that can be removed in a future release as John desires.
To me that is a safe course to take given we are up against the wire. It preserves John's intent to continue to cleanup and refactor, and it leverages the code that we have already shipped and debugged.
These bugs are easy to apply to the John's code too:
#2 is easily fixed by using BotStateSet.SaveAllChanges()
#3 is easily fixed by loading the resource the ctor
#4 is easily fixed by property calling TurnState.Add()
#5 is easily fixed by adding the argument to the Ctor.
So it really comes down to given that we are not exposing that we are using DialogManager , we have a PR which uses it, addresses all of these issues, given the time pressure, what is the compelling reason which says that we MUST remove it right now today in R13?