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

added timeout options for qna & luis for dotnet #1635

Merged
merged 2 commits into from
Apr 9, 2019

Conversation

Zerryth
Copy link
Contributor

@Zerryth Zerryth commented Apr 5, 2019

Part 2 of 2 PRs to fix microsoft/botframework-sdk#5196

Description

  • Added timeout option (in milliseconds) in LuisRecognizer and QnAMaker - dotnet
  • As in C# HttpClient, set timeout default to 100000 milliseconds
  • fixed missing " in docs of IBotTelemetryClient

Specific Changes

LuisRecognizer

  • added Timeout property to LuisPredictionOptions
  • LuisRecognizer now exposes a DefaultHttpClient property to set a default HttpClient for requests, as well as to expose for testing
  • in order to pass ServiceClientCredentials, HttpClient, HttpClientHandler, and LuisDelegatingHandler changes to LUISRuntimeClient, used MsRest's CreateRootHandler() and CreateHttpHandlerPipeline() to create HttpClient to send to LUISRuntimeClient constructor inside LuisRecognizer cosntructor

QnAMaker

  • QnAMakerOptions now has optional Timeout property
  • QnAMaker class constructor will now assign DefaultHttpClient to _httpClient field if no HttpClient arg is provided
    • This will allow the user to set Timeout without having to pass in their custom HttpClient
  • If user does pass in HttpClient into QnAMaker constructor, then it will take the timeout value set in HttpClient
  • DefaultHttpClient is public to expose for testing

@coveralls
Copy link
Collaborator

coveralls commented Apr 5, 2019

Pull Request Test Coverage Report for Build 53576

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 39 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.1%) to 76.248%

Files with Coverage Reduction New Missed Lines %
/libraries/Microsoft.Bot.Builder.AI.LUIS/LuisPredictionOptions.cs 1 40.0%
/libraries/Microsoft.Bot.Builder.AI.QnA/QnAMaker.cs 11 95.65%
/libraries/Microsoft.Bot.Builder.AI.LUIS/LuisRecognizer.cs 27 81.22%
Totals Coverage Status
Change from base Build 53546: 0.1%
Covered Lines: 4398
Relevant Lines: 5768

💛 - Coveralls

@cleemullins
Copy link
Contributor

No issues found in Microsoft.Bot.Builder.dll
No issues found in Microsoft.Bot.Builder.AI.Luis.dll
No issues found in Microsoft.Bot.Builder.AI.QnA.dll
No issues found in Microsoft.Bot.Builder.ApplicationInsights.dll
No issues found in Microsoft.Bot.Builder.Azure.dll
No issues found in Microsoft.Bot.Builder.Dialogs.dll
No issues found in Microsoft.Bot.Builder.TemplateManager.dll
No issues found in Microsoft.Bot.Configuration.dll
No issues found in Microsoft.Bot.Connector.dll
No issues found in Microsoft.Bot.Schema.dll

@cleemullins
Copy link
Contributor

@Zerryth Could you resolve the merge conflicts?

Copy link
Contributor

@cleemullins cleemullins left a comment

Choose a reason for hiding this comment

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

We should sync and review the usage of the HttpClient as you've coded it. I suspect we'll want to revisit the pattern, as the way you've written this may have some fairly major downstream performance implications.

@cleemullins
Copy link
Contributor

No issues found in Microsoft.Bot.Builder.dll
No issues found in Microsoft.Bot.Builder.AI.Luis.dll
No issues found in Microsoft.Bot.Builder.AI.QnA.dll
No issues found in Microsoft.Bot.Builder.ApplicationInsights.dll
No issues found in Microsoft.Bot.Builder.Azure.dll
No issues found in Microsoft.Bot.Builder.Dialogs.dll
No issues found in Microsoft.Bot.Builder.TemplateManager.dll
No issues found in Microsoft.Bot.Configuration.dll
No issues found in Microsoft.Bot.Connector.dll
No issues found in Microsoft.Bot.Schema.dll

@cleemullins
Copy link
Contributor

@johnataylor - Earlier today, you said you've been through this PR. Can you approve or suggest changes you would like to see?

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.

Add an optional timeout for LUIS and QnA calls
4 participants