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

Updated QnAMaker to use v3 API #151

Merged
merged 1 commit into from
Feb 19, 2018
Merged

Conversation

garypretty
Copy link
Contributor

Updated QnAMaker to use v3 API (http://bit.ly/2EA7syR) which brings optional support for metadata to filter / boost answers returned. I can't see any downsides to implementing v3, given that it works exactly like v2, but with optional metadata properties on the request and in the response. If the user does not specify any metadata on the QnAMakerOptions.

@cleemullins As mentioned on the previous pull request to split out the QnAMaker from the middleware, I tried running the QnAMaker tests, but they are commented out at the moment. I have however ran the QnA sample against my own QnA service (which has metadata against some of the items in the KB) and all seems to be working well.

@daltskin

public Metadata[] MetadataBoost { get; set; }
}

[Serializable]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a JSON Serialization Test around this, just to make sure the outputted JSON is of the right format.

}

[Serializable]
public class Metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a silly question, but the QnA Maker JSON element for this is "metadata" (lowercase m). When this is serialized, will that matter? I see this is covered via the JsonProperty attributes on name/value, but I'm not sure about the class name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just following up on this point. We only serialise the contents of the Metadata object (hence needing the json attributes on the name / value). When building up the request for the QnA Maker we explicitly build the object using the name "metadata". If this were to change then we would need the json property on the metadata object, so I might add it for good measure in a follow up PR when I add some additional tests mentioned above.

@cleemullins cleemullins merged commit 8d4aecc into microsoft:master Feb 19, 2018
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.

2 participants