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 descriptive constants for Azure content filter configuration - Option 1 #462

Closed

Conversation

KavithaSiva
Copy link
Contributor

@KavithaSiva KavithaSiva commented Jan 16, 2025

Context

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

What this PR does and why it is needed

This PR adds self descriptive constansts for improving Azure content filter configuration.

Screenshot 2025-01-23 at 12 28 28

Autocompletion works with the string key values of AzureFilterThreshold and the numeric allowed values 0,2,4 and 6 coming up in hints.

@KavithaSiva KavithaSiva changed the title feat: Add descriptive constants for Azure content filter configuration feat: Add descriptive constants for Azure content filter configuration - Option 1 Jan 17, 2025
@KavithaSiva KavithaSiva changed the title feat: Add descriptive constants for Azure content filter configuration - Option 1 feat: Add descriptive constants for Azure content filter configuration Jan 17, 2025
@KavithaSiva KavithaSiva marked this pull request as ready for review January 17, 2025 09:26
* A descriptive type for AzureThreshold input.
* @internal
*/
export interface AzureContentSafetyThresholdType {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also another option:

export type AzureContentSafetyThresholdKeys =
  | 'ALLOW_SAFE'
  | 'ALLOW_SAFE_LOW'
  | 'ALLOW_SAFE_LOW_MEDIUM'
  | 'ALLOW_ALL';

export const AzureFilterThreshold: Record<
  AzureContentSafetyThresholdKeys,
  AzureThreshold
> = {
  ALLOW_SAFE: 0,
  ALLOW_SAFE_LOW: 2,
  ALLOW_SAFE_LOW_MEDIUM: 4,
  ALLOW_ALL: 6
};

But then autocomplete looks like this (It doesn't give the value hints, even if I add comments):
Screenshot 2025-01-17 at 11 42 27

And I didn't find a convenient way to use enum and map it to the values of AzureThreshold

Copy link
Contributor

Choose a reason for hiding this comment

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

[q] Any reason why we are not using enum here and modify the type for, e.g., Hate to that enum? Otherwise user would need to know this constant exist without autocompletion's help.

Copy link
Member

Choose a reason for hiding this comment

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

Would modifying the type be a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discussed with @ZhongpinWang, modifying the type doesn't seem to be a breaking change as in the end they would both resolve to be the same code in JS.

@KavithaSiva
Copy link
Contributor Author

Closing this PR, in favour of #476, introducing an enum with a new type instead.

@KavithaSiva KavithaSiva deleted the feat/descriptive-const-orchestration-filtering branch January 21, 2025 13:16
@KavithaSiva KavithaSiva restored the feat/descriptive-const-orchestration-filtering branch January 22, 2025 08:44
@KavithaSiva KavithaSiva reopened this Jan 22, 2025
@KavithaSiva KavithaSiva changed the title feat: Add descriptive constants for Azure content filter configuration feat: Add descriptive constants for Azure content filter configuration - Option 1 Jan 22, 2025
@KavithaSiva
Copy link
Contributor Author

Closed in favour of #485

@KavithaSiva KavithaSiva deleted the feat/descriptive-const-orchestration-filtering branch January 24, 2025 09:37
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