.Net: Process Cloud Events - Publish Events Abstractions #10477
Conversation
github-actions
bot
changed the title
esttenorio
had a problem deploying
to
integration
GitHub Actions
Failure
esttenorio
had a problem deploying
to
integration
GitHub Actions
Error
esttenorio
had a problem deploying
to
integration
GitHub Actions
Failure
esttenorio
had a problem deploying
to
integration
GitHub Actions
Failure
esttenorio
had a problem deploying
to
integration
GitHub Actions
Failure
| /// <param name="proxyEvent">event data passed to proxy step</param> | ||
| /// <returns></returns> | ||
| [KernelFunction(Functions.EmitExternalEvent)] | ||
| public async Task EmitExternalEventAsync(KernelProcessStepContext context, KernelProcessProxyMessage proxyEvent) |
There was a problem hiding this comment.
You could remove the async qualifier from the method and just return the Task generated by EmitExternalEventAsync. (Same for EmitInternalProcessEventAsync) I'd expect the compiler to result in the same code, but with no await you know for sure there's no extra byte-code.
| @@ -0,0 +1,28 @@ | |||
| // Copyright (c) Microsoft. All rights reserved. | |||
|
|
|||
| namespace Microsoft.SemanticKernel.Process.Internal; | |||
There was a problem hiding this comment.
Putting a public class inside the *.Internal namespace may be a mismatch.
| /// <summary> | ||
| /// Factory that helps create <see cref="KernelProcessProxyMessage"/> | ||
| /// </summary> | ||
| public static class KernelProcessProxyMessageFactory |
There was a problem hiding this comment.
I think this can be internal to Process.Core. (I only see it consumed on ProcessStepBuilder.)
|
|
||
| // TODO-estenori: these 2 could potentially be merged into 1 dict later | ||
| // maybe sourceStep is not needed? need to check | ||
| this.EventTopicMap[eventData.EventName] = topicName; |
There was a problem hiding this comment.
Agree that these can be merged. I'd do so now since it also impacts the public surface of KernelProcessProxyStateMetadata.
| this.EventTopicMap[eventData.EventName] = topicName; | ||
| this.EventDataMap[eventData.EventName] = eventData.EventId; | ||
|
|
||
| this.ExternalTopicUsage[topicName] = true; |
There was a problem hiding this comment.
This can be HashSet<string> and void the faux value.
| /// <inheritdoc/> | ||
| internal override KernelProcessStepInfo BuildStep(KernelProcessStepStateMetadata? stateMetadata = null) | ||
| { | ||
| KernelProcessProxyStateMetadata? proxyMetadata = new() |
There was a problem hiding this comment.
Extraneous ? (KernelProcessProxyStateMetadata? => KernelProcessProxyStateMetadata)
| throw new KernelException("No IExternalKernelProcessMessageChannel found, need at least 1 to emit external messages"); | ||
| } | ||
|
|
||
| await this.ExternalMessageChannel.Initialize().ConfigureAwait(false); |
There was a problem hiding this comment.
Can different proxy-steps share the same channel? I see this is on the base step and passed down from the process-context. How many times will it be initialized?
| /// Provides functionality to allow emitting external messages from within the SK | ||
| /// process. | ||
| /// </summary> | ||
| public sealed class ProcessProxyBuilder : ProcessStepBuilder<KernelProxyStep> |
There was a problem hiding this comment.
Might be useful to lay a sample down to illustrate how this is being used. I'm curious because pretty much everything is internal. (Currious on how the maps are utilized)
| @@ -118,7 +119,7 @@ internal IEnumerable<KernelProcessEdge> GetEdgeForEvent(string eventId) | |||
| /// </summary> | |||
| /// <param name="processEvent">The event to emit.</param> | |||
| /// <returns>A <see cref="ValueTask"/></returns> | |||
| public ValueTask EmitEventAsync(KernelProcessEvent processEvent) | |||
| public virtual ValueTask EmitEventAsync(KernelProcessEvent processEvent) | |||
Motivation and Context
Description
Changes
Fixes #10328
Contribution Checklist