Skip to content
This repository was archived by the owner on Jan 5, 2026. It is now read-only.

Conversation

@daltskin
Copy link
Contributor

I started this work then realized there was already an issue open for this #1906

@stevengum stevengum requested a review from johnataylor June 5, 2019 20:47
@cleemullins cleemullins added blocked Current progress is blocked on something else. Parity with JS Required labels Jun 12, 2019
@cleemullins cleemullins requested a review from Zerryth June 12, 2019 19:50
@cleemullins
Copy link
Contributor

This branch has Conflicts that need to be resolved.
The PR must also link to the JS and Python changes before we can merge it.

@Zerryth
Copy link
Contributor

Zerryth commented Jun 13, 2019

@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.

Copy link
Contributor

@Zerryth Zerryth left a 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.
Copy link
Contributor

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:

  • qna is a QnADTO
  • if there's QnaId, then QnADTO object 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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@johnataylor
Copy link
Member

@trojenguri says since, multi-turn is still in preview, we should not add that to SDK.

@johnataylor
Copy link
Member

johnataylor commented Jun 17, 2019

However this may be great material for the samples repo. @trojenguri maybe can comment on that.

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.

:shipit:

@cleemullins
Copy link
Contributor

@trojenguri Can you please chime in and let me know what to do with this PR?

@gurvsing
Copy link
Contributor

gurvsing commented Jul 3, 2019

Since our Multi-turn feature is currently in preview, lets hold on to this PR.
In next SDK rollout we can take forward these changes.

@cleemullins
Copy link
Contributor

Per Gurvinder:

Since our Multi-turn feature is currently in preview, lets hold on to this PR.
In next SDK rollout we can take forward these changes.

@cleemullins cleemullins closed this Jul 3, 2019
@cleemullins
Copy link
Contributor

Closed for now, and we can revisit in the future.

@hansmbakker
Copy link

This PR is probably replaced by #2397.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

blocked Current progress is blocked on something else.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants