-
Notifications
You must be signed in to change notification settings - Fork 283
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
Add logPersonalInformation to recognizers #3296
Conversation
Pull Request Test Coverage Report for Build 638064761
💛 - Coveralls |
@@ -63,13 +64,15 @@ export class RecognizerSet extends Recognizer implements RecognizerSetConfigurat | |||
|
|||
// merge intents | |||
for (const intentName in result.intents) { |
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.
Since you're here - can we rewrite this to use for...of
instead of for...in
? It's generally a more sane option.
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.
An example might be:
for (const [intentName, intent] of Object.entries(result.intents)) {
...
}
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.
Refactored to use for-of loops! Also renaming the variables to what you specified really helps clarify what parts are going where, as it gets confusing with the class name of an intent is IntentScore
, which includes more than just the score @_@...
Although separately, going through this recognizer for the refactor, it merges the RecognizerResult
's of multiple recognizers in a really bizarre manner:
RecognizerResult
- Text: whatever is the last
RecognizerResult
in the array of results from multiple recognizers registered, will "win" the spot of Text in the mergedRecognizerResult
- Intent: In C# the method doc includes this "
Intents will be merged by picking the intent with the MaxScore.
". However they both actually just push all intents recognized from eachRecognizerResult
in the array of results into the mergedRecognizerResult
- Entities: Contrast with the doc in C# regarding entities "
Entities are merged as a simple union of all of the Entities.
" -- this is the actual behavior what's happening (all entities recognized by all recognizers are included in the mergedRecognizerResult
)
The behavior is the same in both C# and JS, however it seems buggy in both, if we're going off by what the C# doc says vs. what is actually happening...iunno. I don't know enough about the design decision around RecognizerSet
to make any behavioral changes.
So refactored, keeping behavior the same--just what's happening in this recognizer seems odd to begin with
libraries/botbuilder-dialogs-adaptive/src/recognizers/recognizerSet.ts
Outdated
Show resolved
Hide resolved
libraries/botbuilder-dialogs-adaptive/src/recognizers/adaptiveRecognizer.ts
Outdated
Show resolved
Hide resolved
libraries/botbuilder-dialogs-adaptive/src/recognizers/adaptiveRecognizer.ts
Outdated
Show resolved
Hide resolved
libraries/botbuilder-dialogs-adaptive/src/recognizers/adaptiveRecognizer.ts
Outdated
Show resolved
Hide resolved
libraries/botbuilder-dialogs-adaptive/src/recognizers/adaptiveRecognizer.ts
Outdated
Show resolved
Hide resolved
…nstead of undefined
…de + tests + tests cleanup
libraries/botbuilder-dialogs-adaptive/src/recognizers/crossTrainedRecognizerSet.ts
Outdated
Show resolved
Hide resolved
libraries/botbuilder-dialogs-adaptive/tests/testTelemetryProperties.js
Outdated
Show resolved
Hide resolved
libraries/botbuilder-ai-orchestrator/tests/recognizerTelemetryUtils.js
Outdated
Show resolved
Hide resolved
libraries/botbuilder-dialogs-adaptive-testing/tests/testTelemetryProperties.js
Outdated
Show resolved
Hide resolved
libraries/botbuilder-dialogs-adaptive-testing/tests/testTelemetryProperties.js
Outdated
Show resolved
Hide resolved
Phew. Ok, updated with round of requested changes! Ready for a another review @joshgummersall whenever you're available |
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 work!
Fixes #3295
Description
Specific Changes
AdaptiveRecognizer
class that has:logPersonalInformation
flagfillRecognizerResultTelemetryProperties
override that takes into account the logPii flag when creating telemetry propertiesAdaptiveRecognizerConfiguration
interfaceContext
See microsoft/botbuilder-dotnet#5066 (comment)
Testing
This PR is 90% tests. Generally each recognizer tested 3 scenarios: