Skip to content
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

feat: lg naming migration #2597

Merged
merged 43 commits into from
Apr 20, 2020
Merged

feat: lg naming migration #2597

merged 43 commits into from
Apr 20, 2020

Conversation

zhixzhan
Copy link
Contributor

@zhixzhan zhixzhan commented Apr 10, 2020

Description

LG team recently changed the naming policy. microsoft/botbuilder-dotnet#3712

  • Remove dash in identifier (template name)

It's an incompatible change, and previous LG file must change their template name, probably use _ instead.

e.g, replace dash - to underline _

bfdactivity-76HgfJ 
-> 
bfdactivity_76HgfJ

Likely there are 3 part work.

  1. update all sample bot.
  2. update generate LG template pattern.
  3. migrate user's bot

For 3, one migration solution is before we switch to new lg parser (which would failed to parse), do replacement at current lg parser ( which both support dash - & underline _ ).

Renaming check list

LG

  1. bfdactivity-1234 -> SendActivity_1234
  2. bfdprompt-1234 -> TextInput_Prompt_1234
  3. bfdinvalidPrompt-1234 -> TextInput_InvalidPrompt_1234
  4. bfdunrecognizedPrompt-1234 -> TextInput_UnrecognizedPrompt_1234
  5. bfddefaultValueResponse-1234 -> TextInput_DefaultValueResponse_1234

LU

  1. TextInput.response-RuZhXe -> TextInput_Response_RuZhXe

In this PR, we've also updated the sdk packages and schems which includes some features and bug fixes:

  1. Enable composer to create a skill.
  2. LanguagePolicy fix.
  3. Schema update, Microsoft.Recognizer renamed to Microsoft.IRecognizer, which have some coresponding SDKKinds fixes.
  4. Some other SDK fix like, foreach, RepeatOption, some lg expression fixes, oauth fix....

fixes #2499, #2624

Task Item

refs #2596
refs #778

Screenshots

After load a old bot, replace results:
*.dialog:
Screen Shot 2020-04-14 at 3 25 56 PM

*.lg:
Screen Shot 2020-04-14 at 3 11 12 PM

adaptiveCard.json
Screen Shot 2020-04-14 at 3 39 42 PM

Won't auto-replace case:

  1. reference is user customize defined lg template name,
- ${my-template()}

technically can not distinguish it is an expression or a template reference.

@github-actions
Copy link

Coverage Status

Coverage remained the same at 41.186% when pulling d2590dc on lg-naming-migration into 9f2c161 on master.

@cwhitten
Copy link
Member

cwhitten commented Apr 12, 2020

hey @zhixzhan. thanks for taking this one. I'd like to take the opportunity to change the LG template naming scheme to be more inline with where we want to go. instead of bfdactivity_${id} let's write SendActivity_${id}. Can you include this change here?

Additionally, let's make sure to also update the following:

Inline LG naming conventions in the prompts
# bfdprompt-RuZhXe() -> ${inputType}_Prompt_${id}

Inline LU naming conventions in the prompts
# TextInput.response-RuZhXe -> ${inputType}_Response_${id}

cc @tdurnford do you have any comments here?

@zhixzhan
Copy link
Contributor Author

hey @zhixzhan. thanks for taking this one. I'd like to take the opportunity to change the LG template naming scheme to be more inline with where we want to go. instead of bfdactivity_${id} let's write SendActivity_${id}. Can you include this change here?

Additionally, let's make sure to also update the following:

Inline LG naming conventions in the prompts
# bfdprompt-RuZhXe() -> ${inputType}_Prompt_${id}

Inline LU naming conventions in the prompts
# TextInput.response-RuZhXe -> ${inputType}_Response_${id}

cc @tdurnford do you have any comments here?

@cwhitten Okay, will update it.

@tdurnford
Copy link
Collaborator

@zhixzhan @cwhitten I think it's a good idea to standardize the naming conventions for lg and lu in the prompts, and ${inputType}_Prompt_${id} and ${inputType}_Response_${id} respectively makes sense to me.

@cwhitten
Copy link
Member

@zhixzhan I like your plan to migrate between LG parsers. Will you include that in this change?

@zhixzhan
Copy link
Contributor Author

@zhixzhan I like your plan to migrate between LG parsers. Will you include that in this change?

@cwhitten you mean update the latest lgParser include this? I know the js version is on the way, probably ready at middle of this week. if so, yes we will include this.

@cwhitten
Copy link
Member

@zhixzhan I like your plan to migrate between LG parsers. Will you include that in this change?

@cwhitten you mean update the latest lgParser include this? I know the js version is on the way, probably ready at middle of this week. if so, yes we will include this.

No I mean the strategy to update old templates to the new naming convention.

@zhixzhan zhixzhan marked this pull request as ready for review April 14, 2020 08:02
@zhixzhan zhixzhan requested a review from a-b-r-o-w-n as a code owner April 14, 2020 08:02
@luhan2017
Copy link
Contributor

I've tested this branch. All the luis bot are not working correctly because such settings are auto generated by lubuild, "=settings.luis.additem_en-us_lu", but we don't support - in expressions right now.

Talked with @boydc2014 and @feich-ms , fei is going to make a change in lubuild to replace all the - with _ in expressions. We need to wait until fei's update is ready.

Copy link
Contributor

@luhan2017 luhan2017 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update bf lu package

@zhixzhan
Copy link
Contributor Author

Need to update bf lu package

@luhan2017 Luis package has been updated, can you have another try.

@cwhitten
Copy link
Member

do we need to migrate old luis settings that use the '-' character?

@zhixzhan
Copy link
Contributor Author

do we need to migrate old luis settings that use the '-' character?

@luhan2017 if re-run lubuild or publish, can previous settings be erased?

@luhan2017
Copy link
Contributor

@luhan2017 if re-run lubuild or publish, can previous settings be erased?

Yes they will be re-generated.

luhan2017
luhan2017 previously approved these changes Apr 18, 2020
@cwhitten
Copy link
Member

cwhitten commented Apr 18, 2020

@zhixzhan in the todowithluis sample I'm seeing broken behavior attempting to navigate between the all-up lg page and the design page, and also when trying to delete dialogs. I don't see the same behavior when using other samples.

Screen Shot 2020-04-18 at 11 21 21 AM

Also seeing the following in the console:

LU Language Client: The server is initialized.
console-window.js:61 LU Language Client: Section todobotwithluissample-3.lu#newSection do not exist

@zhixzhan
Copy link
Contributor Author

zhixzhan commented Apr 19, 2020

@zhixzhan in the todowithluis sample I'm seeing broken behavior attempting to navigate between the all-up lg page and the design page, and also when trying to delete dialogs. I don't see the same behavior when using other samples.

Screen Shot 2020-04-18 at 11 21 21 AM

it comes from re-open current project, the last file write maybe not finished. I add a try_catch to mute this error.

Also seeing the following in the console:

LU Language Client: The server is initialized.
console-window.js:61 LU Language Client: Section todobotwithluissample-3.lu#newSection do not exist

@cwhitten updated.

@cwhitten cwhitten merged commit 4704385 into master Apr 20, 2020
@cwhitten cwhitten deleted the lg-naming-migration branch April 20, 2020 15:23
@yeze322 yeze322 mentioned this pull request Apr 21, 2020
7 tasks
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
* change - in template name to _ for all sample bots

* update lg naming pattern

* update tests

* add method to convert template name when loading

* Inline LU naming conventions in the prompts
# TextInput.response-RuZhXe -> ${inputType}_Response_${id}

* update lg name pattern

* replace bfdactivity -> SendActivity

* activity -> SendActivity

* bfdprompt_1234 -> TextInput_Prompt_1234

* designerId no dash -

* lgField use schema.$kind as lgNameType

* use standalone method generate designer id

* Update packages

* Update sdk.schema

* more replacement

* change LG package version

* upgrade lg package

* fix api of Expression

* Update BotProject.csproj

update to the latest which include:
1. skill support
2. foreach and foreachPage nested fix.

* update sample bot

* use SDKKinds

* use shared lu name builder

* Update LanguagePolicy and IRecognzier

* Recognizer -> IRecognizer

* Update to the latest 200416

which include repeat option fix and OAuth ifx

* Fix for skill AutoEndDialog

* upgrade luis package

* replace cover ${ } and @{ } expression syntax

* upgrade luis package fix dash -

* handle file write error

* use a placeholder luSectionName

Co-authored-by: Shuai Wang <shuwan@microsoft.com>
Co-authored-by: Lu Han <32191031+luhan2017@users.noreply.github.com>
Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants