-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat(headless): improve Generate Answer documentation #4069
Conversation
Pull Request ReportPR Title✅ Title follows the conventional commit spec. Live demo linksBundle Size
SSR Progress
Detailed logssearch : buildInteractiveResultsearch : buildInteractiveInstantResult search : buildInteractiveRecentResult search : buildInteractiveCitation search : buildGeneratedAnswer recommendation : missing SSR support product-recommendation : missing SSR support product-listing : missing SSR support case-assist : missing SSR support insight : missing SSR support commerce : missing SSR support |
@@ -23,6 +23,8 @@ export interface GeneratedResponseFormat { | |||
answerStyle: GeneratedAnswerStyle; | |||
/** | |||
* The content formats that are supported for rendering the answer. If no values are provided, `text/plain` is used. | |||
* The answer is generated using the first format supported by the front-end component, from left to right. |
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 seems a tad silly since there are only two supported formats at the moment, one of which will, if I understand correctly, always be supported 😅
In essence, you can/should either:
- leave this empty, or
- pass
['text/markdown', 'text/plain']
because:
- passing
['text/plain', 'text/markdown']
is silly (text/plain
will always be used) - passing
['text/markdown']
is not recommended (you should add the fallback) - passing
['text/plain']
is useless (it's the same as not passing anything)
Still, granted that we may eventually add support for additional formats, I understand why using an array makes sense.
That being said, this makes me wonder: instead of recommending to always include text/plain
as the last value in the array as a fallback, should we just fallback to text/plain
regardless?
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.
Still, granted that we may eventually add support for additional formats, I understand why using an array makes sense.
At some point new formats will have to be supported, but they are not yet known.
That being said, this makes me wonder: instead of recommending to always include text/plain as the last value in the array as a fallback, should we just fallback to text/plain regardless?
We can do that, but I feel it would be confusing to specify contentFormat: ['text/markdown']
and receive an answer in text/plain
. The current behavior makes it pretty clear. If you specify ['text/markdown']
and the model can't generate an answer for that format, then the request fails. If you specify ['text/markdown', 'text/plain']
, then you expect to receive text/markdown
or text/plain
.
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 also find it odd to have to specify the fallback format. It feels stranger to request one of the two formats than to request one format and have the default fallback used if the specified format is unavailable.
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.
The contentFormat
attribute is not meant to request a specific format. It represents the capabilities of the front-end component. It is the platform that decides which format to return.
It was implemented that way so the feature rollout could be controlled from the platform.
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.
It still seems to me like it would be less error-prone to fallback to text/plain
even if it's not specified as the last value in the array.
As it is, contentFormat
is optional, and yet, if I understand correctly, the requests will fail when no value is specified. Given that we have a strong opinion of what he fallback format should be (i.e., It is recommended to include
text/plain as the last value in the array to act as a fallback format
), I think we might as well just fallback to this value, and explain the behavior in the documentation.
Unless there is a reason why someone would actually want requests to fail... but this seem dubious to me.
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.
Again, sorry for the delay.
As it is, contentFormat is optional, and yet, if I understand correctly, the requests will fail when no value is specified.
To be clear, the request will fail if contentFormat
is explicitely set to an empty array.
Unless there is a reason why someone would actually want requests to fail... but this seem dubious to me.
Ok, I can modify it so we can always fallback to text/plain
.
@@ -23,6 +23,8 @@ export interface GeneratedResponseFormat { | |||
answerStyle: GeneratedAnswerStyle; | |||
/** | |||
* The content formats that are supported for rendering the answer. If no values are provided, `text/plain` is used. | |||
* The answer is generated using the first format supported by the front-end component, from left to right. | |||
* The request fails when no matching format is found. It is recommended to include `text/plain` as the last value in the array to act as a fallback format. |
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.
when no matching format is found
found by what?
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.
It is the first match between formats supported by the front-end component, and formats supported by the model.
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.
@fbeaudoincoveo I was trying to keep it short, and I felt that adding too much would make it more confusing. Do you have a suggestion to make it clearer?
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.
Sorry for the super late reply, @lbergeron !
How about:
The request fails when none of the specified formats are supported by the front-end component or by the model.
Closing this PR and will open a new one to add the automatic fallback to |
SVCC-3877
This PR clarifies how to set the
contentFormat
attribute for the Generated Answer controller.