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

feat: Add convenience function for LlamaGuard Filter #555

Merged
merged 21 commits into from
Feb 24, 2025

Conversation

KavithaSiva
Copy link
Contributor

@KavithaSiva KavithaSiva commented Feb 17, 2025

Context

Closes SAP/ai-sdk-js-backlog#205.

What this PR does and why it is needed

This PR adds a convenience function to enable filter categories for Llama guard filter.

Screenshot 2025-02-20 at 14 41 31

The built filter can then be used in the filtering configuration of the Orchestration client.

Copy link
Contributor

@deekshas8 deekshas8 left a 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.

@KavithaSiva
Copy link
Contributor Author

KavithaSiva commented Feb 18, 2025

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.

I would personally prefer if the user explicitly sets the properties to true as even if it is verbose, it is clear that we are turning some categories on.

buildLlamaGuardFilter({ hate: true, sexual_content: true})  //Very clear
buildLlamaGuardFilter([hate', 'sexual_content']) //Not clear, if we are enabling or disabling the category 

@deekshas8
Copy link
Contributor

buildLlamaGuardFilter([hate', 'sexual_content']) //Not clear, if we are enabling or disabling the category

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.

@KavithaSiva KavithaSiva marked this pull request as ready for review February 21, 2025 07:48
@ZhongpinWang ZhongpinWang force-pushed the feat/add-llama-guard-filtering-option branch from 64184ec to 84e982b Compare February 24, 2025 13:31
@ZhongpinWang
Copy link
Contributor

Side note: Don't worry about the long commit history above.. I did a git rebase and it makes me the co-author of all previous commits..

Copy link
Contributor

@deekshas8 deekshas8 left a comment

Choose a reason for hiding this comment

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

Left 2 minor comments regarding docs. Else LGTM

@ZhongpinWang ZhongpinWang merged commit bc51f59 into main Feb 24, 2025
10 checks passed
@ZhongpinWang ZhongpinWang deleted the feat/add-llama-guard-filtering-option branch February 24, 2025 14:49
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.

3 participants