Closed drub0y closed 5 years ago
Perhaps @stevenic can comment further.
In many ways what you have here with the Dialog stack is a cross language (but very simple) reflection system. Specifically, the Dialog stack is really just naming pieces of implementation code. Whether you take these names as the class name or a user provided string doesn't change the architecture. But have these explicit strings does make the design language independent. A nice pattern in C# is to use the nameof(MyClass) operator for the Dialog name as you add it to the DialogSet.
Regarding all the construction at runtime, the pattern that works more effectively is to use ComponentDialogs. For example, write a RootDialog class derived from ComponentDialog and then inside that class build your dialog. The point is each ComponentDialog brings its own DialogSet.
Having done that you should be able to create a singleton and then dependency inject that, if that's the model you like to use.
Of course, in your IBot or Controller instance you will need to instantiate a minimal stub DialogSet and do the boilerplate Continue/Begin dance but that isn't much runtime from a construction point of view because you only have a single item in the top level DialogSet. I'm sure this could be optimized further but it's much less of a problem when you factor your code like this.
There is certainly a real problem here in that this boilerplate code is repeated everywhere but the solution that has been discussed there is a "Run" method that can encapsulate all this. However, the fisrt step towards this was simply to make everything consistent and then iterate from there.
In many ways what you have here with the Dialog stack is a cross language (but very simple) reflection system. Specifically, the Dialog stack is really just naming pieces of implementation code. Whether you take these names as the class name or a user provided string doesn't change the architecture. But have these explicit strings does make the design language independent. A nice pattern in C# is to use the nameof(MyClass) operator for the Dialog name as you add it to the DialogSet.
This seems slightly off topic. Yes, the indirection of using ids is great... I shouldn't (necessarily) know the implementation of the "airport picker" dialog. Though, technically, there's a small catch there too because there is a minimal contract around the result type of a dialog (e.g. wanting an airport code as a string
vs an Airport
instance), but I digress.
Regarding all the construction at runtime, the pattern that works more effectively is to use ComponentDialogs. For example, write a RootDialog class derived from ComponentDialog and then inside that class build your dialog. The point is each ComponentDialog brings its own DialogSet.
Sure, I'm aware of ComponentDialog
(frankly I love the concept), but I don't think it addresses the details I'm raising in this issue. In fact, it introduces its own set of unique issues. More on this below.
Having done that you should be able to create a singleton and then dependency inject that, if that's the model you like to use.
The issue is partly about having transient lifetimes for Dialog
instances, just like controllers in ASP.NET. I know I can have singletons, but that causes the realization of an entire dependency graph which, by virtue of the root service (be it DialogSet
or ComponentDialog
) being a singleton, themselves become singletons. Singletons are not the answer to this issue.
Of course, in your IBot or Controller instance you will need to instantiate a minimal stub DialogSet and do the boilerplate Continue/Begin dance but that isn't much runtime from a construction point of view because you only have a single item in the top level DialogSet. I'm sure this could be optimized further but it's much less of a problem when you factor your code like this.
First, I don't want to "instantiate" anything. My dependencies should be passed in via the constructor just like an ASP.NET controller. All of the samples seem to be "instantiating" DialogSet
s in their constructors and filling them out by instantiating specific Dialog
s and adding them with Add[Dialog]
. This should be considered an anti-pattern. This will be a performance nightmare for any non-trivial dialog tree.
Let's walk through this idea that you'd instantiate a "minimal stub" DialogSet
that may contain a single ComponentDialog
subclass (e.g. the RootDialog
you mentioned).
Here's what the bot might look like:
public class MyBot : IBot
{
private readonly DialogSet _myDialogs;
public MyBot(IStatePropertyAccessor<DialogState> dialogStatePropertyAccessor)
{
_myDialogs = new DialogSet(dialogStatePropertyAccessor)
.Add(new RootDialog("root"));
}
// OnTurn impl elided for brevity
}
Looks "minimal", seems lightweight... 2 objects being new'd up, no biggy. Except that's not exactly the case. When you instantiate RootDialog
, which is a ComponentDialog
, its constructor is going to do something like this:
public class RootDialog : ComponentDialog
{
public RootDialog(string dialogId) : base(dialogId)
{
AddDialog(new WaterfallDialog("mainMenu", CreateMainMenuWaterfall()));
AddDialog(new AirportPickerDialog("airport"));
// probably a bunch more here
}
// CreateMainMenuWaterfall elided for brevity, but obviously would create the steps
}
Hmm, ok so now we've got 2 more Dialog
instantiations here, as well as that whole waterfall creation, but... wait, what's that AirportPickerDialog
? Oh that's another ComponentDialog
subclass that I brought in from a NuGet package and happens to have 10 more dialogs inside of it and the pattern repeats itself until the dialog tree is fully realized.
If my breakdown of the scenario above is accurate, there is no scenario where instantiating a DialogSet
does not result in the entire dialog tree and all of its dependencies being fully realized.
Thus far, this has only addressed Dialog
s themselves. What if my AirportPickerDialog
contains a sub-dialog that needs an IAirportListingService
dependency? Is the proposal that someone who writes a ComponentDialog
would need their constructor to take all dependencies for all sub-dialogs so that I can plumb them through? In this case I'd have to change my RootDialog
to take the IAirportListingService
and pass it to the new AirportPickerDialog(...)
which would then have to pass it to the subdialog that needed it. Imagine if the AirportPickerDialog
itself used another ComponentDialog
that needed another N services? Extrapolate this out even just a little bit and I don't see how anyone could argue that this won't become completely unmanageable very quickly.
As mentioned, I have already taken the steps towards providing a solution to the problem in this working branch and have had great success, including keeping 100% backwards compatibility with the existing API. But, before we go over the details of that solution, let’s get agreement on the problem.
I was suggesting creating a singleton RootDialog object and injecting that in the constructor of the IBot instance? So that's slightly different than the code you outlined.
Also something to keep in mind is that there is a requirement to have exactly the same core model (and that would include dialogs) in every implementation language.
I believe the issue of not having DI in place does not only concern Dialogs, but also the access to service clients.
Imagine I have a service client class that sends a webhook to Teams. I don't want to instantiate a new object of it every time. It would be much more convenient and clean if I could register that once and then use it afterwards.
Besides, Singleton-style objects have three risks:
Of course a programmer is still responsible for writing good code without leaks, but having no proper dependency injection as a fallback is a lack.
This is another reason why having proper dependency injection with proper lifetime scoping is a best practice and a request for bot framework.
References:
Creating a root component dialog effectively addresses this issue, for example this pattern is used effectively in sample 42.
The other point that sample illustrates is that the dialog stack can effectively be contained and encapsulated in a single and relatively simple function call. Basically along the lines of:
(newState, activities) Process(oldState, activity)
This clean functional approach is a big step towards making the whole framework more friendly to DI in particular and more modular in general. The current plan is to work towards this incrementally.
Agreed this is not specifically an issue about dialogs.
The current plan is to work towards this incrementally.
@johnataylor Could you be a bit more concrete on the steps that are taken towards this?
People are asking for it in https://github.com/Microsoft/AI/issues/579, too.
@johnataylor @cleemullins is there any update on this?
In my opinion this issue is closed too early as it is not solved and a clear path towards a solution is not given.
IMHO, I think that the main problem is that the dialogs are stuck with the current implementation of DialogSet
and would be better if Dialogs expect not a DialogSet
but just an interface with single method (#1409):
Task<Dialog> FindDialogAsync(string id)
and leave us to decide how to implement it (current DialogSet
could be just one possible implementation, but not the only one).
Is your feature request related to a problem? Please describe. The design of the Dialogs API is such that the developer constructs a
DialogSet
by adding instances ofDialog
subclasses to it when theDialogSet
is created which means the developer has to create thoseDialog
instances themselves. While these instances could be resolve from a DI container, thus causing their dependencies to be resolved from the container as well automatically, none of the samples do this. Additionally this essentially leads to realizing an entire, potentially deeply nested dependency graph duringDialogSet
creation time. Any non-trivial conversation structure is going to contain several dialogs and those dialogs each may require several dependencies which themselves may require dependencies..NET developers are more dependent on DI these days than ever with the advent of .NET Core and, more specifically, ASP.NET intrinsically being built around DI patterns. They expect to be able to use these patterns in other modern .NET frameworks... especially ones that integration with ASP.NET.
Describe the solution you'd like The Dialogs API should be reworked to describe the types of
Dialog
s rather than instances and then use a factory abstraction to resolveDialog
instances in a just-in-time, transient manner when they are needed. A basic implementation might just creates instances via a default constructor, but then a richer implementation could be created to resolve the instances from a DI container (e.g.IServiceProvider
) which would enable transient instances whose entire dependency chain would be automatically resolved for a given turn.Additional context I intend to submit a full DCR around this and have begun work on prototyping the implementation in the
drmarsh/dcr/better-dialogs-integration
branch with great success including full backwards compatibility with the existing static APIs (e.g.DialogSet
).