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

refactor: Content filter helper function #441

Merged
merged 57 commits into from
Feb 3, 2025
Merged

Conversation

ZhongpinWang
Copy link
Contributor

@ZhongpinWang ZhongpinWang commented Jan 10, 2025

Context

SAP/ai-sdk-js-backlog#202.

What this PR does and why it is needed

Deprecate buildAzureContentFilter() because it restricts filtering to have only one filter.

Implement buildAzureContentSafetyFilter() which returns Azure content safety filters instead.

.changeset/afraid-cooks-shave.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/src/internal.ts Outdated Show resolved Hide resolved
packages/orchestration/src/orchestration-filtering.ts Outdated Show resolved Hide resolved
sample-code/README.md Outdated Show resolved Hide resolved
packages/orchestration/src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@tomfrenken tomfrenken left a comment

Choose a reason for hiding this comment

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

Concept looks good and even allows for adding streaming options.

LGTM, after making the adjustments Deeksha has requested.

@KavithaSiva
Copy link
Contributor

KavithaSiva commented Jan 17, 2025

Proposed API looks good to me too, although I didn't like it that the user has to explicitly wrap the calls inside an array even for using a single filter.

I feel typing { filters: [ContentFilters.azure({ Hate: 4, SelfHarm: 2 })] } to use a single filter is redundant.

An alternate I could think of was:

filtering: {
  input: ContentFilters.azure({ Hate: 4, SelfHarm: 0 }),
  // or
  input: ContentFilters.llamaGuard({ ... }),
  // or
  input: ContentFilters.addMultipleFilters(
    ContentFilters.azure({ Hate: 4, SelfHarm: 0 }),
    ContentFilters.llamaGuard({ ... })
  )
}

WDY think?
But yeah, the behaviour would then be similar to buildAzureContentFilter() and I would then think ContentFilters then only helps with discoverability.

@ZhongpinWang
Copy link
Contributor Author

ZhongpinWang commented Jan 20, 2025

Proposed API looks good to me too, although I didn't like it that the user has to explicitly wrap the calls inside an array even for using a single filter.

I feel typing { filters: [ContentFilters.azure({ Hate: 4, SelfHarm: 2 })] } to use a single filter is redundant.

An alternate I could think of was:

filtering: {
  input: ContentFilters.azure({ Hate: 4, SelfHarm: 0 }),
  // or
  input: ContentFilters.llamaGuard({ ... }),
  // or
  input: ContentFilters.addMultipleFilters(
    ContentFilters.azure({ Hate: 4, SelfHarm: 0 }),
    ContentFilters.llamaGuard({ ... })
  )
}

WDY think? But yeah, the behaviour would then be similar to buildAzureContentFilter() and I would then think ContentFilters then only helps with discoverability.

I get your point. But as you mentioned, we then need some kind of ContentFilters.addMultipleFilters() function to concatenate filters, which feels just a bit of overkilling the problem and makes the helper function less helpful.

Copy link
Contributor

@KavithaSiva KavithaSiva left a comment

Choose a reason for hiding this comment

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

LGTM for me.

Agreed that the 2 import behaviors should be documented

But, @deekshas8 also wrote this, can you check with her what needs to be documented here?

Personally for me though I would prefer if the user always uses it via the ContentFilters for better discoverability of other functions too.

.changeset/afraid-cooks-shave.md Outdated Show resolved Hide resolved
Copy link
Contributor

@KavithaSiva KavithaSiva left a comment

Choose a reason for hiding this comment

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

Some behaviour changes have been made to buildAzureContentFilter (The old convenience function). This has to be reverted as it's a breaking change.

packages/orchestration/src/util/filtering.ts Outdated Show resolved Hide resolved

describe('Content filter util', () => {
// TOOD: Remove this test collection once `buildAzureContentFilter` is removed.
describe('buildAzureContentFilter', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we dont need to test this anymore as it's deprecated. It's not like we're going to provide a bug fix/update here ? @tomfrenken

Copy link
Contributor

Choose a reason for hiding this comment

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

I would vote to remove the tests once the function gets removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing is it will only be removed in the next major version and I dont want unnecessary tested code building up in the codebase

.changeset/afraid-cooks-shave.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/src/util/filtering.ts Outdated Show resolved Hide resolved
packages/orchestration/src/util/request-config.test.ts Outdated Show resolved Hide resolved
sample-code/src/orchestration.ts Outdated Show resolved Hide resolved
sample-code/src/orchestration.ts Outdated Show resolved Hide resolved
KavithaSiva and others added 9 commits February 3, 2025 15:23
Co-authored-by: Deeksha Sinha <88374536+deekshas8@users.noreply.github.com>
Co-authored-by: Deeksha Sinha <88374536+deekshas8@users.noreply.github.com>
Co-authored-by: Deeksha Sinha <88374536+deekshas8@users.noreply.github.com>
Co-authored-by: Deeksha Sinha <88374536+deekshas8@users.noreply.github.com>
Co-authored-by: Deeksha Sinha <88374536+deekshas8@users.noreply.github.com>
@deekshas8
Copy link
Contributor

deekshas8 commented Feb 3, 2025

Apart from the file naming issue, rest LGTM.

@KavithaSiva KavithaSiva enabled auto-merge (squash) February 3, 2025 15:20
@KavithaSiva KavithaSiva merged commit 54a9044 into main Feb 3, 2025
10 checks passed
@KavithaSiva KavithaSiva deleted the refactor-content-filters branch February 3, 2025 15:23
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.

4 participants