-
Couldn't load subscription status.
- Fork 622
.NET: [BREAKING] Unify ExecutorIsh and ExecutorRegistration, unify/simplify APIs #1637
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
base: main
Are you sure you want to change the base?
Conversation
8d015db to
b2978c4
Compare
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.
Pull Request Overview
This PR removes the ExecutorIsh type and unifies its functionality with ExecutorRegistration, now implemented as a record-based type hierarchy. The change simplifies the API by normalizing extension methods to .AsExecutor() and .RegisterExecutor(), while obsoleting older methods like .ConfigureFactory() and .ConfigureSubworkflow().
Key Changes:
- Replaced
ExecutorIshwith an abstractExecutorRegistrationrecord class hierarchy - Added concrete registration types:
ExecutorInstanceRegistration,SubworkflowRegistration,RequestPortRegistration,AIAgentRegistration,PlaceholderRegistration, andConfiguredExecutorRegistration - Updated all extension methods to use
.AsExecutor()and.RegisterExecutor()naming conventions
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| ExecutorIsh.cs | Deleted file - entire ExecutorIsh type removed |
| ExecutorRegistration.cs | Converted from internal sealed class to public abstract record class with type hierarchy |
| ExecutorRegistrationExtensions.cs | New file containing unified extension methods for creating registrations |
| ExecutorInstanceRegistration.cs | New registration type for executor instances |
| SubworkflowRegistration.cs | New registration type for workflows used as executors |
| RequestPortRegistration.cs | New registration type for request ports |
| AIAgentRegistration.cs | New registration type for AI agents |
| PlaceholderRegistration.cs | New registration type for unbound executor references |
| ConfiguredExecutorRegistration.cs | New registration type for configured executors |
| WorkflowBuilder.cs | Updated to use ExecutorRegistration instead of ExecutorIsh |
| Workflow.cs | Updated property names and reset logic for new registration model |
| Sample files | Updated to use new .AsExecutor() and .RegisterExecutor() methods |
dotnet/src/Microsoft.Agents.AI.Workflows/ExecutorRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Microsoft.Agents.AI.Workflows/ExecutorRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Microsoft.Agents.AI.Workflows/ExecutorRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Microsoft.Agents.AI.Workflows/ExecutorRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Microsoft.Agents.AI.Workflows/ExecutorRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Microsoft.Agents.AI.Workflows/ExecutorRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Microsoft.Agents.AI.Workflows/ExecutorRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Microsoft.Agents.AI.Workflows/ExecutorRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Microsoft.Agents.AI.Workflows/ExecutorRegistrationExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Microsoft.Agents.AI.Workflows/ExecutorRegistration.cs
Outdated
Show resolved
Hide resolved
b2978c4 to
8549eae
Compare
8549eae to
240deb3
Compare
|
Good riddance |
|
For thew new public registration types, is there sample debt being incurred here? If so, can some issues be created to track new samples that might be desired? |
|
Are there any patterns here that should be channeled back into the python platform? @ekzhu? |
|
Do we have tests that cover the issues being closed? |
240deb3 to
7cab9ad
Compare
Not really: It's the same thing as with
Yes, the test was updated to cover the scenario that discovered this. |
* Switch to more modern Record type-tree for Sum Types * Unify APIs for getting ExecutorRegistration * Fix an issue where workflows consisting entirely of cross-run shareable executors which are not instance-resettable do not properly clear state when running non-concurrently.
7cab9ad to
4a13466
Compare
| Assert.Fail("Not all ExecutorIsh types were tested."); | ||
| } | ||
| // It would be great if there was a way to test exhaustion for record-based Sum Types | ||
| // TODO: Unit tests don't have to be AOT-compatible, so... |
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.
Looks like there's a proposal for "closed hierarchies" https://github.com/dotnet/csharplang/blob/7603e3bee2ea3edb53e6a3b27efbaf24949b609f/proposals/closed-hierarchies.md
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.
Nice, that could help with certainty of exhaustiveness!
Would still probably need reflection to count them, which is more what I was getting at in the TODO.
if its not expected / intended to be used directly, then why |
| Func<string, string> uppercaseFunc = s => s.ToUpperInvariant(); | ||
| var uppercase = uppercaseFunc.AsExecutor("UppercaseExecutor"); |
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.
If this were instead just a static method like Executor.Create, it could instead be:
var uppercase = Executor.Create<string, string>(s => s.ToUpperInvariant(), "UppercaseExecutor");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.
Is there a reason to prefer Executor.Create<TIn, TOut> over Executor<TIn, TOut>.Create?
Also, one can just do new FunctionExecutor<TIn, TOut>(s => s.ToUpperInvariant(), "..."), or even better, get rid of var, put the type on the left, and use typeless new(...). Maybe we should remove the .AsExecutor() overloads for functions and instead put the overloads on the FunctionExecutor ctors?
(I was genuinely disappointed when I realized that it cannot be applied directly to a method, or a lambda in parens, so less valuable as an API than I had thought it would be. Feels like a very low-hanging fruit to be able to bind that way, especially in the extensions-everything world - but then, this is the "implicit cast" thing again, and I know your stance on that :))
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.
Is there a reason to prefer
Executor.Create<TIn, TOut>overExecutor<TIn, TOut>.Create?
The main benefit of the former is you get generic type inference if the compiler can discern it, e.g. with the former you can write:
Executor.Create((string s) => s.ToUpperInvariant(), "UppercaseExecutor");but not with the latter. The former also lets you avoid having the extra type with two generic type parameters if we don't already have it.
|
|
||
| // Step 2: Configure the sub-workflow as an executor for use in the parent workflow | ||
| ExecutorIsh subWorkflowExecutor = subWorkflow.ConfigureSubWorkflow("TextProcessingSubWorkflow"); | ||
| ExecutorRegistration subWorkflowExecutor = subWorkflow.AsExecutor("TextProcessingSubWorkflow"); |
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.
Does AsExecutor return an "Executor" or an "ExecutorRegistration"?
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.
ToRegistration would be more accurate, but my thinking was that AsExecutor is more obviously relevant when browsing IntelliSense. Will think more on the name today, to see if I can come up with something better.
| } | ||
|
|
||
| private static ExecutorIsh GetExecutorIsh(IModeledAction action) => | ||
| private static ExecutorRegistration GetExecutorIsh(IModeledAction action) => |
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.
"Ish"
| // provenance tracking exposed in the workflow context passed to a handler. | ||
| ExecutorIsh[] agentExecutors = (from agent in agents select (ExecutorIsh)new AgentRunStreamingExecutor(agent, includeInputInOutput: false)).ToArray(); | ||
| ExecutorIsh[] accumulators = [.. from agent in agentExecutors select (ExecutorIsh)new CollectChatMessagesExecutor($"Batcher/{agent.Id}")]; | ||
| ExecutorRegistration[] agentExecutors = (from agent in agents select (ExecutorRegistration)new AgentRunStreamingExecutor(agent, includeInputInOutput: false)).ToArray(); |
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.
"Executors"... these are now "Registrations"?
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.
Until we get real anonymous union types we need to model the type explicitly. We need a way to allow passing information about future Executors without encouraging passing Executor instances to WorkflowBuilder, but also without disallowing it.
We are trying to avoid having to do .RegisterExecutor() on the builder object, as it feels more heavy-weight for the developer, and we already have much more ceremony than Python due to limitations stemming from AOT, among other things.
At some point we have to make compromises somewhere due to the limits of the expressivity of the language.
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 am with @stephentoub how about WorkflowExecutor or a similar name? thus adding parity to the naming in Python?
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 do people think about WorkflowNode or ExecutorUnion, or BoundExecutor (especially since I just realized we need to add object-ownership semantics to executor instances too if we want to be really careful)?
TODO: Need to add ownership semantics to Executor to avoid cross-workflow sharing. (In a separate PR)
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.
Had a bit of an epiphany overnight. New object name: ExecutorBinding. It is more than a factory, and carries some state information with it, but it is not yet "registered" so Registration is not a good name for it - as pointed out above.
| namespace Microsoft.Agents.AI.Workflows; | ||
|
|
||
| // TODO: Unwrap the Configured object, just like for WorkflowRegistration | ||
| internal record ConfiguredExecutorRegistration(Configured<Executor> ConfiguredExecutor, Type ExecutorType) |
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.
Is "Configured" still needed? I guess I don't understand its purpose separate from these registrations (and its name is fairly ambiguous).
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.
A way to smuggle options objects and certain other important information and avoid having to constantly break the interface for developers, but that did not last long as a conviction, due to the arrival of RunId, which is not available when the configuration objects are created.
This is probably the next place to "fix" as we review the APIs.
| /// </remarks> | ||
| /// <param name="executor">The executor instance.</param> | ||
| /// <returns>An <see cref="ExecutorRegistration"/> instance wrapping the specified <see cref="Executor"/>.</returns> | ||
| public static ExecutorInstanceRegistration RegisterExecutor(this Executor executor) |
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.
ExecutorInstanceRegistration instead of ExecutorRegistration?
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.
At one point I got a CodeAnalysis performance complaint about something similar where I was returning a more specific type, and I flipped all of them. Doesn't seem to be triggering now, so must have had some other reason to fire and I did not understand it right.
| /// <typeparam name="TExecutor">The type of the resulting executor</typeparam> | ||
| /// <param name="factoryAsync">The factory method.</param> | ||
| /// <returns>An <see cref="ExecutorRegistration"/> instance that resolves to the result of the factory call when messages get sent to it.</returns> | ||
| public static ExecutorRegistration RegisterExecutor<TExecutor>(this Func<string, string, ValueTask<TExecutor>> factoryAsync) |
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.
"Register" makes it sound like something is being stored somewhere, some existing object is being mutated to hold this new record, etc., like on CancellationToken. But this is only creating the ExecutorRegistration instance, right? It's not actually registering it with something?
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, but I could not come up with a clearly better name for it. The thinking around "registration" was in the sense of being the "registration" one submits when trying to get registered for something, not the established "registration" after the fact.
I need a name for an object that represents a future executor (or a placeholder for one - so factory is not really a good term, as it only deals with one part), with certain info about its underlying type that get erased by the time it is in that object, and the ability to create and "reset" it (if shared).
Maybe "ExecutorProvider"? Though it goes a fair bit beyond "Providing" an Executor, so it does not really fit either.
Aside: The Registration bit would be internal to the Workflow/Builder, but previous name for such a thing was ...Ish, and that was pretty polarizing.
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 believe that the process mostly is doing is assigning ownership of the workflow instance so it cannot be used while "taken" or "assigned", so I am all in with finding a suitable name that clearly shows what is happening. And IMHO it is not exactly "registration" its more an assignation, or giving ownership of this workflow executor.
But I am all a newbie here so can be completely wrong...
| /// <typeparam name="TExecutor">The type of the resulting executor</typeparam> | ||
| /// <param name="factoryAsync">The factory method.</param> | ||
| /// <returns>An <see cref="ExecutorRegistration"/> instance that resolves to the result of the factory call when messages get sent to it.</returns> | ||
| [Obsolete("Use RegisterExecutor() instead.")] |
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.
Is this to give folks time to migrate off of it? Consider also adding EditorBrowsableState.Never
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.
Yes: I intend to remove it when we get to the "RC-like" phase or before. And good call on Browsable.
| /// Defines an implicit conversion from an AIAgent to an ExecutorRegistrationIsh instance. | ||
| /// </summary> | ||
| /// <param name="agent"></param> | ||
| public static implicit operator ExecutorRegistration(AIAgent agent) => agent.AsExecutor(); |
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.
You know my opinion on these, but I'll stop commenting on it.
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 think we're going to have to agree to disagree on this point, yeah :).
I promise to remove it is as soon as I can replace it anonymous type unions that work across languages, though, and not add more except in specifically cases like these.
I need a type to use on a public API that has the property of being a type-union of the types I actually want people to use. That's why |
Concurrent run support was recently added to workflows, but Orchestrations did not fully update to support it. A few executors were missing Cross-Run Shareable annotations, and the ConcurrentEnd executor needed to be factory-instantiated. This also ports the fix for #1613 from #1637, to avoid waiting on that PR.
Concurrent run support was recently added to workflows, but Orchestrations did not fully update to support it. A few executors were missing Cross-Run Shareable annotations, and the ConcurrentEnd executor needed to be factory-instantiated. This also ports the fix for #1613 from #1637, to avoid waiting on that PR.
Concurrent run support was recently added to workflows, but Orchestrations did not fully update to support it. A few executors were missing Cross-Run Shareable annotations, and the ConcurrentEnd executor needed to be factory-instantiated. This also ports the fix for #1613 from #1637, to avoid waiting on that PR.
Concurrent run support was recently added to workflows, but Orchestrations did not fully update to support it. A few executors were missing Cross-Run Shareable annotations, and the ConcurrentEnd executor needed to be factory-instantiated. This also ports the fix for #1613 from #1637, to avoid waiting on that PR.
Concurrent run support was recently added to workflows, but Orchestrations did not fully update to support it. A few executors were missing Cross-Run Shareable annotations, and the ConcurrentEnd executor needed to be factory-instantiated. This also ports the fix for microsoft#1613 from microsoft#1637, to avoid waiting on that PR.
Motivation and Context
ExecutorIsh, a type built to work around the lack of full support for anonymous discriminated unions in .NET, leads to confusion, and makes it difficult to find how to integrateExecutor-like types into a workflow. It also duplicates some functionality withExecutorRegistration, and now that something broke in F#'s ability to feed the auto-cast types toWorkflowBuilderthe value of the split between the two is lowered.Description
ExecutorIshandExecutorRegistration, relying on the record type hierarchy for sum-type-like behaviourExecutorRegistrations:.AsExecutor()and.RegisterExecutor()Breaking:
ExecutorIshtype, in favour ofExecutorRegistration.ConfigureFactory()and.ConfigureSubworkflow()extension methods in favour of.RegisterExecutor()and.AsExecutor(), respectivelyRelevant Issues:
Contribution Checklist