[AACS][GA] 2024-09-01 Azure AI Content Safety #29866
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Next Steps to Merge✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge. |
Swagger Generation Artifacts
|
Generated ApiView
|
This was referenced Jul 17, 2024
bowgong
added
the
PublishToCustomers
Member
|
If some commit is for testing, it is advised to be done in a different branch and on a draft PR (draft so it does not loop in reviewers, PR as the automation runs so you can see the result). We'd hope this PR to be clean for review. |
AzureRestAPISpecReview
added
the
BreakingChangeReviewRequired
Member
Author
|
@mikekistler , can you help review and approve this PR? |
mikekistler
requested changes
Aug 2, 2024
Member
mikekistler
left a comment
mikekistler
left a comment
There was a problem hiding this comment.
Some of these comments are minor but I think splitting the result schema is an important change.
You'll also need to take this through breaking change review.
| "description": "BlocklistItem content.", | ||
| "maxLength": 128 | ||
| "description": "BlocklistItem content. The length is counted using Unicode code point.", | ||
| "maxLength": 1000 |
Member
There was a problem hiding this comment.
Should not change maxLength in released API definitions.
Member
Author
There was a problem hiding this comment.
How can we just apply the new maxLength for the new version and keep the original in released version?=
Member
Author
There was a problem hiding this comment.
Also I don't quite understand why a larger maxLength is considered as a breaking change. With a larger maxLength, the existing client implementation won't be affected.
Member
There was a problem hiding this comment.
I think Mike's concern is that this change applies to 2023-10-01 api-version, not the new 2024-09-01. And the general rule is there should be no API change to existing api-version.
What is the maxLength for this API in 2023-10-01?
Currently I am not aware of a way to versioning the decorator in TypeSpec. Maybe you can check in TypeSpec Discussion channel.
An ugly fallback (if no other alternative) would be @remove then @add.
Member
Author
There was a problem hiding this comment.
maxLength in 2023-10-01 is 128, and we don't want to change it, but we're going to increase it to 1000 in 2024-09-01.
@mikekistler , can you help advise further?
Member
There was a problem hiding this comment.
Please come to Breaking Change office hours today at 10AM PT so we can discuss.
Member
Author
There was a problem hiding this comment.
@weidongxu-microsoft , thank you for the suggestion. I tried below but both not work. Can you help advise further how to use the remove/add to do the fallback? I posted this question into the TypeSpec Discussion as well.
@removed(ContentSafety.Versions.v2024_09_01)
@doc("BlocklistItem content. The length is counted using Unicode code point.")
@maxlength(128)
text: string;
@added(ContentSafety.Versions.v2024_09_01)
@doc("BlocklistItem content. The length is counted using Unicode code point.")
@maxlength(1000)
text: string;
@removed(ContentSafety.Versions.v2024_09_01)
@added(ContentSafety.Versions.v2024_09_01)
@doc("BlocklistItem content. The length is counted using Unicode code point.")
@maxlength(1000)
text: string;
I think Mike's concern is that this change applies to 2023-10-01 api-version, not the new 2024-09-01. And the general rule is there should be no API change to existing api-version.
What is the maxLength for this API in 2023-10-01?
Currently I am not aware of a way to versioning the decorator in TypeSpec. Maybe you can check in TypeSpec Discussion channel. An ugly fallback (if no other alternative) would be
@removethen@add.
Member
There was a problem hiding this comment.
I assume option1 would give you a "duplicate entity" error?
The answer from Timothee and Mark is better than mine (they are the expert on TypeSpec).
I guess you would have to discuss this with Mike on breaking change office hour. Do show this discussion in TypeSpec Discussion.
specification/cognitiveservices/data-plane/ContentSafety/stable/2024-09-01/contentsafety.json
Outdated
Show resolved
Hide resolved
specification/cognitiveservices/data-plane/ContentSafety/stable/2024-09-01/contentsafety.json
Outdated
Show resolved
Hide resolved
| "type": "array", | ||
| "description": "The documents needs to be analyzed, which may contain direct or indirect injection attacks.", | ||
| "items": { | ||
| "type": "string" |
Member
There was a problem hiding this comment.
Is there a maxLength for the documents?
Member
Author
There was a problem hiding this comment.
the maxItems is 5 currently, while we are going to increase it soon. I'm wondering if adding the limit here is the best practice, or we just do the validation at runtime and avoid changing the schema.
Member
There was a problem hiding this comment.
My question was actually about the maxLength of individual documents -- not the number of elements in the array. You previously had maxLength: 10000 on userPrompt so I was suggesting something similar here. But I see that you have removed that. I'm not a fan of omitting information from a REST API definition so that it can be "silently" changed later, but that's maybe a discussion we can put off.
specification/cognitiveservices/data-plane/ContentSafety/stable/2024-09-01/contentsafety.json
Show resolved
Hide resolved
…e/2024-09-01/contentsafety.json Co-authored-by: Mike Kistler <[email protected]>
…e/2024-09-01/contentsafety.json Co-authored-by: Mike Kistler <[email protected]>
…esult and DocumentInjectionAnalysisResult
…re-rest-api-specs into aacs-stable-20240901
bowgong
changed the title
AzureRestAPISpecReview
removed
the
BreakingChangeReviewRequired
Member
Author
|
@mikekistler , given that we have decided to keep the maxLength in new API version, I think this PR is ready to merge. Can you help review again and sign-off? |
mikekistler
approved these changes
Aug 8, 2024
| "type": "array", | ||
| "description": "The documents needs to be analyzed, which may contain direct or indirect injection attacks.", | ||
| "items": { | ||
| "type": "string" |
Member
There was a problem hiding this comment.
My question was actually about the maxLength of individual documents -- not the number of elements in the array. You previously had maxLength: 10000 on userPrompt so I was suggesting something similar here. But I see that you have removed that. I'm not a fan of omitting information from a REST API definition so that it can be "silently" changed later, but that's maybe a discussion we can put off.
mikekistler
added
the
APIStewardshipBoard-SignedOff
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Data Plane API Specification Update Pull Request
Tip
Overwhelmed by all this guidance? See the
Getting helpsection at the bottom of this PR description.PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your API Spec engagement record.
Is this review for (select one):
Change Scope
This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous API Spec document (if applicable), and the root paths that have been updated.
azure-rest-api-specs/specification/cognitiveservices/ContentSafety/models.tsp
Line 385 in 9280478
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiViewcomment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
Swagger-Suppression-Process
to get approval.
❔Got questions? Need additional info?? We are here to help!
Contact us!
The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.
Click here for links to tools, specs, guidelines & other good stuff
Tooling
Guidelines & Specifications
Helpful Links
Getting help
write accessper aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Mergecomment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.and https://aka.ms/ci-fix.
queuedstate, please add a comment with contents/azp run.This should result in a new comment denoting a
PR validation pipelinehas started and the checks should be updated after few minutes.fix #29891