Skill Updates First pass #2657
Merged
Skill Updates First pass #2657
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TLDR; I have new version which has no side storage for conversation reference routes all traffic up through the bot, so the bot is in control of routing Covers basic operations on Activity (send/update/delete) Is less code and has no GDPR issues (other then bots internal conversationState logic to decide to route traffic to skill) ---- This has greatly simplified the amount of logic: I don't use conversationReference context switching, instead the only change to payload is that I add a custom property to the Recipient and Conversation objects (both which allow for additional properties) recipient.skillId = "id of the skill" conversation.serviceUrl = "original serviceUrl for the conversation" Since those properties are always reflected back, I don't need to do context switching, but instead can create an appropriate conversationReference to bind to the adapter I route all activities up through the bot and the bot is in control of calling appropriate adapter.XXX() APIS to pushes them back down to the user. I turn DeleteActivity() => ActivityType.DeleteActivity. The bot then turns that artifical delete activity into a call to botAdapter.DeleteActivity() call I turn UpdateActivity() => ActivityType.UpdateActivity (with new activity in the Value payload), the bot then turns that artifical update activity into call to botAdapter.UpdateActivity() As I suspected, the APIs that we are able to implement without the side-car is less due to not having enough information to plumb through Fully Supported APIS SendToConversation()/ReplyToActivity() UpdateActivity() DeleteActivity() Not Supported APIS CreateConversation() - Not sure if we want this GetConversations() - not modeled by adapter, could support as pure connectClient call SendConversationHistory() - only has conversationId as arg, GetConversationMembers() - only has conversationId as arg GetConversationPagedMembers() - only has conversationId as arg DeleteConversationMember() - only has conversationId as arg GetActivityMembers() - only has conversationId as arg UploadAttachment() - only has conversationId as arg For the conversationId APIs there are 2 approaches to support these: If I have the original serviceUrl I can call straight through to the user using a connectorClient (bypassing the adapter) If I have the original conversationReference I can route through the adapter up to the bot -Tom
Moved skillscenarios to prototypes.
…up to the bot to process
…otbuilder-dotnet into gabog/SkillFirstPass
Removed experimental ConversationBridgeMiddleware and MultiAdapter. Added placholder for BotToDialog
Removed InvokeActivity jiggerapoo
Simplified project references Added cancelationTokens and cleaned up dead code.
…e unnecesary rules on whitespace.
Excluded anotehr whitespace rule that didn't make a lot of sense.
…to see changes better.
Removed dependencies on old skills library. Updated Adapter inherit from BF adapter rather than streaming.
…ver. First pass at S2S auth. Removed hardcoded URLs from SDK. Moved skill definition to the schema library. Removed Microsoft.Bot.Builder.Skills from the library.
…to allow passing a custom resouce (known issue, skill throws an exception on "stop"), will fix that later. Removed projects that don't compile yet
lzc850612
reviewed
Oct 16, 2019
...es/integration/Microsoft.Bot.Builder.Integration.AspNet.Core/BotFrameworkHttpSkillsServer.cs
Outdated
Show resolved
Hide resolved
Fixed some warnings and corrected some code.
…placed the code with the SDK primitives (we'll use dialog manager eventually).
Fixed some code based on steves comments. More code cleanup and fixes.
lzc850612
reviewed
Oct 16, 2019
| /// <param name="activity">activity to forward.</param> | ||
| /// <param name="cancellationToken">cancellation Token.</param> | ||
| /// <returns>Async task with optional invokeResponse.</returns> | ||
| public override async Task<InvokeResponse> ForwardActivityAsync(ITurnContext turnContext, string skillId, Activity activity, CancellationToken cancellationToken) |
There was a problem hiding this comment.
need to add a telemetry logging to log the latency for a skill call
There was a problem hiding this comment.
K, thanks, I'll do this in another push.
Tomlm/skill adapter
…is no good but will get updated when carlos is done)
…ier to spot changes.
Some cleanup
Moved HttpSkillServer to the skills package.
* Renamed SkillAdapter to SkillHostAdapter * Reverted change to BotAdapter.cs methods * Additional APIs only work if bot adapter is BotFrameworkAdapter * Changed SkillsCallbackEndpoint to SkillHostEndpoint to match rest of naming convention * Added Exception return so we can throw appropriately on not implemented.
…d left it as notsupported in the host for now. Added additional constructors to BotFrameworkSkillHostAdapter Updated names and documentation in some Auth classes as discussed with carlos.
Downgraded test project to 2.1 Applied changes fot enginetests from master
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.5.3 |
cleemullins
approved these changes
Oct 17, 2019
| throw new ArgumentNullException(nameof(ServiceUrl)); | ||
| } | ||
|
|
||
| return Convert.ToBase64String(Encoding.UTF8.GetBytes(JsonConvert.SerializeObject(new[] |
There was a problem hiding this comment.
Please file an issue to track this, as I believe this string is too large for our Blob and CosmosDB Storage providers. We've hit a similar bug w/ Teams in the past.
There was a problem hiding this comment.
Created Github issue 2736 to track.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref #2774
This PR contains some of the changes we are planning for Skills.
Creating it as a draft so we can start viewing the changes.