-
Notifications
You must be signed in to change notification settings - Fork 12
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: Add convenience function for LlamaGuard Filter #555
base: main
Are you sure you want to change the base?
Conversation
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.
A suggestion:
Since the default for is false for all properties of the filter, the user only needs to set categories which they want to enable. As such, setting to true
seems like a redundant effort. We could allow the user to just pass an array of strings (from union type of all existing properties) and we set them to true.
The pro here would be less typing and less confusion (setting false makes no sense, but this might not very obvious).
The con is it deviates from azure filter a bit where we have a key: value
style of input.
|
||
it('builds filter config with no config', async () => { | ||
const filterConfig = buildLlamaGuardFilter(); | ||
const expectedFilterConfig = { |
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 shouldnt be allowed as config is a mandatory property and requires a min of 1 key.
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.
We don't really account for minProperties
of 1, so the resulting type for LlamaGuard38b
ends up being a type with all properties as optional.
My idea was to apply default of false to all categories if the user calls buildLlamaGuardFilter()
similar to how we have default applied for azure, buildAzureContentSafetyFilter()
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 think we need to clarify what default means for each of the filters.
With azure, orchestration allows an empty config, because it sets a default of 2 for all categories.
With llama, as far as I understood, only categories set to true are even evaluated by the model, false are not considered and neither returned in the response. If I call orchestration with the below config:
{
"type": "llama_guard_3_8b",
"config": {
"violent_crimes": false,
"non_violent_crimes": true,
"sex_crimes": false
}
}
The response is:
"input_filtering": {
"message": "Input filter passed successfully.",
"data": {
"llama_guard_3_8b": {
"non_violent_crimes": false
}
}
}
When I pass everything as false, I imagine the input filter will always success, because not a single category is evaluated.
How is that helpful to the user?
Regarding the type issue, I believe it can be mitigated by creating a new type that requires user to set something. Allowing user to call the function with no params makes less sense imo.
const filterConfig = buildLlamaGuardFilter({ | ||
child_exploitation: false, | ||
hate: true, | ||
violent_crimes: false, |
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.
Setting properties to false can lead to confusion as skipping a property means it's false anyway
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.
Removed false
properties, but the user can still set false
as I directly use the generated type.
* @returns Filter config object. | ||
*/ | ||
export function buildLlamaGuardFilter( | ||
config?: LlamaGuard38B |
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.
[req] config should be a required property.
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.
My idea was to apply default if nothing is provided by the user.
Orchestration service does not allow an empty config {}
object to be sent. So I applied a default of false
for all categories.
I would personally prefer if the user explicitly sets the properties to buildLlamaGuardFilter({ hate: true, sexual_content: true}) //Very clear
buildLlamaGuardFilter([hate', 'sexual_content']) //Not clear, if we are enabling or disabling the category |
…P/ai-sdk-js into feat/add-llama-guard-filtering-option
From my understanding of the filter, you basically can enable a category, the default is false for all the rest categories. What I personally find more confusing is allowing to disable something thats going to be disabled anyway. |
Context
Closes SAP/ai-sdk-js-backlog#205.
What this PR does and why it is needed