.Net Processes - Map Step Feature #9339

crickman · 2024-10-20T23:39:26Z

Motivation and Context

Fixes: #9193

Description

Map each value from a set to a map-operation and present the results as a set for potential reduction.

Includes:

ProcessMapBuilder (Core)
KernelProcessMap / KernelProcessMapState (Abstractions)
LocalMap (LocalRuntime)
MapActor (DaprRuntime)
Serialization
Unit Tests (exhaustive)
Integration Tests
Sample

Features:

Executes map operations in parallel
Handles when output type has been transformed from input type
Accepts either step or subprocess for map operation
Proxies events for edges defined directly on the map-step
Allows additional (non proxied) edges for the map-operation
Participates in state serialization

Follow-up:

Documentation - .Net Processes - Document Map Step #9616

Contribution Checklist

The code builds clean without any errors or warnings
The PR follows the SK Contribution Guidelines and the pre-submission formatting script raises no violations
All unit tests pass, and I have added new tests where possible
I didn't break anyone 😄

…antic-kernel into processes-map-step

esttenorio · 2024-11-08T19:14:07Z

dotnet/src/Experimental/Process.Abstractions/KernelProcessMap.cs

+    public const string MapEventId = "StartMap";
+
+    /// <summary>
+    /// The map operation.


[nit] perhaps this description should be more detailed - mention that is the step/subprocess that will be applied to the array received as input

Now that we have our constants file, I've moved this definition (and updated comment).

esttenorio · 2024-11-08T19:19:30Z

dotnet/src/Experimental/Process.Core/ProcessStepBuilder.cs

@@ -66,6 +66,8 @@ public ProcessStepEdgeBuilder OnFunctionError(string functionName)

    #endregion

+    internal const char EventIdSeparator = '.';


[nit] remove since it seems unused (?)

Using now, as you identified...also moved to our constants file.

esttenorio · 2024-11-08T19:22:28Z

dotnet/src/Experimental/Process.LocalRuntime/LocalKernelProcessFactory.cs

-        Verify.NotNullOrWhiteSpace(process.State?.Name);
-        Verify.NotNull(kernel);
-        Verify.NotNull(initialEvent);
+        Verify.NotNull(initialEvent, nameof(initialEvent));


in general wondering if for initialEvent there should also be a check for the Id not not be empty

This is a good point. I've updated KernelProcessEvent.Id so its no longer nullable, but it might be whitespace or empty string. I suppose my thought is that this condition is relevant for any event that is emitted, so perhaps the check might be more useful as part of the processing loop. I've updated LocalProcess.RunOnceAsync to check for this (as opposed to the factory) and also LocalStep.EmitEventAsync and KernelProcessStepContext.EmitEventAsync.

Just to be clear: This change didn't introduce or regress the ability for null or empty event-id to be created, but its a good improvment. As you seen, I've been incrementally adjusting assumptions around our internal data flow (removing !, for example). I expect there's still more gaps to discover and address.

dotnet/src/Experimental/Process.UnitTests/KernelProcessSerializationTests.cs

esttenorio · 2024-11-08T19:57:39Z

dotnet/src/Experimental/Process.IntegrationTests.Shared/ProcessMapTests.cs

+        ProcessBuilder process = new(nameof(TestMapAsync));
+
+        ProcessStepBuilder computeStep = process.AddStepFromType<ComputeStep>();
+        ProcessMapBuilder mapStep = process.AddMapForTarget(new ProcessFunctionTargetBuilder(computeStep));


[brainstorming hypothetical scenario]
wondering if these 2[line 40-41] should be a single function? since compute is not actually used by the root process directly having it instantiated on the level computeStep could in theory also be used as a regular step:

graph TD root["root process"]-->map-->compute1["compute"] root-->compute2["compute"]

Loading

Good question. Ben and I were discussing this also. There is a case where an edge may be defined outside of the map-operation: https://github.com/microsoft/semantic-kernel/blob/processes-map-step/dotnet/src/Experimental/Process.UnitTests/Runtime.Local/LocalMapTests.cs#L295

graph TD root["root process"]--N-->map subgraph map compute1["compute"] end compute1["compute"]--1-->orthogonal compute1["compute"]--N-->union root--extra/noharm-->compute2["compute"]

Loading

The other factor to consider is when the map operation is a subprocess...such a pattern would also result in being explicitly handled by the caller (first create sub-process, then associate with map). The difference for step is that the builders only allow creating one from the process builder...which in turn always adds it to the step collection of the process.

I'm am certainly open to exploring any potential improvements, but it also feels like we need to be careful to consider the breadth of patterns that make sense and avoid disrupting others that are useful.

FYI - I'll planning on spending some time exploring alternative patterns here...just want to be careful.

esttenorio · 2024-11-08T20:00:54Z

dotnet/src/Experimental/Process.LocalRuntime/LocalMap.cs

+    {
+        this._map = map;
+        this._logger = this._kernel.LoggerFactory?.CreateLogger(this._map.State.Name) ?? new NullLogger<LocalStep>();
+        this._mapEvents = [.. map.Edges.Keys.Select(key => key.Split('.').Last())];


was this supposed to use the EventIdSeparator I saw somewhere?

Yes, I think so...thanks for spotting this. (Plus I'll move this to our constants file now)...updated several usage points.

esttenorio · 2024-11-08T20:02:38Z

dotnet/src/Experimental/Process.LocalRuntime/LocalMap.cs

+                LocalKernelProcessContext processContext = new(process, this._kernel, context.Filter);
+                Task processTask =
+                    processContext.StartWithEventAsync(
+                        new KernelProcessEvent


so all events emitted from a map are internal right? maybe if one was marked as public at this level a warning can be raised so people know

Correct, events specific to the map-operation (map-step edges) are internal as they target the process in which the map step is defined. Any other proxied event will retain its original visibility.

esttenorio · 2024-11-08T20:09:21Z

dotnet/src/Experimental/Process.UnitTests/Core/ProcessMapBuilderTests.cs

@@ -0,0 +1,223 @@
+// Copyright (c) Microsoft. All rights reserved.


wondering how the error messages are dealt with the map step?
same as you get an [] of nullable results, when subscribing to onError you get an [] of errors?

yes, I believe so. if out of 5 steps 3 result in some type of error and 2 don't, three invocations of the error handler seems natural. i'm hestant to intercept and interpret external failure. If the mapstep itself experience an exception, I'd expect that to raise a single error event.

crickman added 6 commits October 16, 2024 14:09

Checkpoint

b263e0a

Merge from main

3638c4d

Checkpoint

230c456

Merge branch 'main' into processes-map-step

ccf99af

Merge branch 'main' into processes-map-step

ecfa86d

Checkpoint

d5dea83

crickman added PR: in progress Under development and/or addressing feedback .NET Issue or Pull requests regarding .NET code experimental Associated with an experimental feature enhancement processes labels Oct 20, 2024

crickman self-assigned this Oct 20, 2024

github-actions bot changed the title ~~.NET Processes - Map Step Feature~~ .Net Processes - Map Step Feature Oct 20, 2024

crickman had a problem deploying to integration October 20, 2024 23:39 — with GitHub Actions Error

crickman added 2 commits October 20, 2024 16:43

Clean-up

991c5d1

Cleanup

04f6ee7

crickman had a problem deploying to integration October 20, 2024 23:44 — with GitHub Actions Error

Cleanup

ccd6418

crickman had a problem deploying to integration October 20, 2024 23:47 — with GitHub Actions Error

Namespace

59aad71

crickman had a problem deploying to integration October 20, 2024 23:49 — with GitHub Actions Error

namespace

3a74058

crickman temporarily deployed to integration October 20, 2024 23:54 — with GitHub Actions Inactive

crickman requested a review from alliscode October 20, 2024 23:58

crickman added 2 commits October 20, 2024 17:06

Clean-up

47fa435

Cleanup

13fd520

crickman had a problem deploying to integration October 21, 2024 00:07 — with GitHub Actions Error

Subprocess validation

b8b676f

crickman had a problem deploying to integration October 21, 2024 00:11 — with GitHub Actions Error

Clean

d327f0a

crickman had a problem deploying to integration November 8, 2024 06:52 — with GitHub Actions Failure

Namespace

3b5781e

crickman had a problem deploying to integration November 8, 2024 07:04 — with GitHub Actions Failure

crickman had a problem deploying to integration November 8, 2024 07:30 — with GitHub Actions Error

Stablize unit-tests

091f5ad

crickman had a problem deploying to integration November 8, 2024 07:35 — with GitHub Actions Error

Namespace

813a842

crickman temporarily deployed to integration November 8, 2024 07:41 — with GitHub Actions Inactive

Merge branch 'main' into processes-map-step

349c41d

crickman temporarily deployed to integration November 8, 2024 16:46 — with GitHub Actions Inactive

Serialization tweaks

b370a6f

crickman temporarily deployed to integration November 8, 2024 17:35 — with GitHub Actions Inactive

Merge branch 'main' into processes-map-step

03cf6ba

crickman temporarily deployed to integration November 8, 2024 17:56 — with GitHub Actions Inactive

crickman added 2 commits November 8, 2024 10:13

else

3110acf

Merge branch 'processes-map-step' of https://github.com/microsoft/sem…

85f44a4

…antic-kernel into processes-map-step

crickman temporarily deployed to integration November 8, 2024 18:14 — with GitHub Actions Inactive

esttenorio reviewed Nov 8, 2024

View reviewed changes

dotnet/src/Experimental/Process.UnitTests/KernelProcessSerializationTests.cs Show resolved Hide resolved

esttenorio reviewed Nov 8, 2024

View reviewed changes

Update based on PR comments

560c427

crickman had a problem deploying to integration November 8, 2024 20:34 — with GitHub Actions Error

crickman requested a review from esttenorio November 8, 2024 20:35

Merge branch 'main' into processes-map-step

c99bd2e

crickman deployed to integration November 8, 2024 20:40 — with GitHub Actions Active

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.Net Processes - Map Step Feature #9339

.Net Processes - Map Step Feature #9339

crickman commented Oct 20, 2024 •

edited

Loading

esttenorio Nov 8, 2024

crickman Nov 8, 2024

esttenorio Nov 8, 2024

crickman Nov 8, 2024 •

edited

Loading

esttenorio Nov 8, 2024

crickman Nov 8, 2024 •

edited

Loading

esttenorio Nov 8, 2024 •

edited

Loading

crickman Nov 8, 2024 •

edited

Loading

esttenorio Nov 8, 2024

crickman Nov 8, 2024 •

edited

Loading

esttenorio Nov 8, 2024

crickman Nov 8, 2024

esttenorio Nov 8, 2024

crickman Nov 8, 2024 •

edited

Loading

		@@ -66,6 +66,8 @@ public ProcessStepEdgeBuilder OnFunctionError(string functionName)

		#endregion

		internal const char EventIdSeparator = '.';

		@@ -0,0 +1,223 @@
		// Copyright (c) Microsoft. All rights reserved.

.Net Processes - Map Step Feature #9339

Are you sure you want to change the base?

.Net Processes - Map Step Feature #9339

Conversation

crickman commented Oct 20, 2024 • edited Loading

Motivation and Context

Description

Contribution Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crickman Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crickman Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

esttenorio Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

crickman Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crickman Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crickman Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

crickman commented Oct 20, 2024 •

edited

Loading

crickman Nov 8, 2024 •

edited

Loading

crickman Nov 8, 2024 •

edited

Loading

esttenorio Nov 8, 2024 •

edited

Loading

crickman Nov 8, 2024 •

edited

Loading

crickman Nov 8, 2024 •

edited

Loading

crickman Nov 8, 2024 •

edited

Loading