-
Notifications
You must be signed in to change notification settings - Fork 493
Conversation
|
This branch has Conflicts that need to be resolved. |
|
@daltskin Great start! However I do notice there's a like of the "multi-" in what's supposed to be updating the SDK to implement the multi-turn features of QnA Maker. Your unit tests written thus far demonstrate the case of a request returning the initial answer and follow-up prompts response. Please include a case that demonstrates the return of a non-initial answer and follow-up prompts. See Multi-turn conversations docs here for reference. |
Zerryth
left a comment
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.
See comments regarding non-initial answers & follow-up prompts as well as with comments regarding qna property in PromptDTO
| public int DisplayOrder { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the QnADTO returned from the API - is always null. |
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.
I don't believe that the last part of this comment is correct, "is always null"
The API reference docs here indicate that:
qnais aQnADTO- if there's
QnaId, thenQnADTOobject is ignored
The last bullet then implying that there are instances where it is not null.
I have emailed the QnA team directly to confirm, however
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.
This will be good to know for sure. I had already looked at the reference docs - but from experience of using the API, it has always passed back a null response.
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.
I've added a qna non initial context test.
I've also raised a question on the documentation around this here: https://github.com/MicrosoftDocs/azure-docs/issues/33335
|
@trojenguri says since, multi-turn is still in preview, we should not add that to SDK. |
|
However this may be great material for the samples repo. @trojenguri maybe can comment on that. |
cleemullins
left a comment
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.
![]()
|
@trojenguri Can you please chime in and let me know what to do with this PR? |
|
Since our Multi-turn feature is currently in preview, lets hold on to this PR. |
|
Per Gurvinder: |
|
Closed for now, and we can revisit in the future. |
|
This PR is probably replaced by #2397. |
I started this work then realized there was already an issue open for this #1906