Skip to content

Comments

HealthDataAIServices Deidentificiation Data Plane - GA Stable API Changes#30518

Merged
GrahamMThomas merged 71 commits intoAzure:mainfrom
GrahamMThomas:healthdataaiservices/customization-options
Dec 6, 2024
Merged

HealthDataAIServices Deidentificiation Data Plane - GA Stable API Changes#30518
GrahamMThomas merged 71 commits intoAzure:mainfrom
GrahamMThomas:healthdataaiservices/customization-options

Conversation

@GrahamMThomas
Copy link
Contributor

@GrahamMThomas GrahamMThomas commented Sep 11, 2024

Data Plane API Specification Update Pull Request

Tip

Overwhelmed by all this guidance? See the Getting help section at the bottom of this PR description.

Goal of this PR is to add a few features we are looking to add in GA. Going from a preview version to GA. Want to make a few breaking changes to the schema before we release our first stable version.

  • Created surrogateLocale to allow customer's to request their output in a certain locale
  • Moved dataType into SourceStorageLocation model
  • Introduced an overwrite flag in TargetStorageLocation model
  • Moved redactionFormat, operation into DeidentificationCustomizationOption models
  • Changed outputPrefix behavior from including jobName to prefix replacement method.
  • Removed Path and Etag from TaggerResult output files in favor of DeidentificationDocumentDetails.

PR review workflow diagram

Please understand this diagram before proceeding. It explains how to get your PR approved & merged.

spec_pr_review_workflow_diagram

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.

  • Link to API Spec engagement record issue:

Is this review for (select one):

  • a private preview
  • a public preview
  • GA release

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.

  • Design Document:
  • Previous API Spec Doc:
  • Updated paths:

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 ApiView comment 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

  • First, please carefully read through this PR description, from top to bottom.
  • If you don't have permissions to remove or add labels to the PR, request write access per aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositories
  • To understand what you must do next to merge this PR, see the Next Steps to Merge comment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.
  • For guidance on fixing this PR CI check failures, see the hyperlinks provided in given failure
    and https://aka.ms/ci-fix.
  • If the PR CI checks appear to be stuck in queued state, please add a comment with contents /azp run.
    This should result in a new comment denoting a PR validation pipeline has started and the checks should be updated after few minutes.
  • If the help provided by the previous points is not enough, post to https://aka.ms/azsdk/support/specreview-channel and link to this PR.

@openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Sep 11, 2024

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ The required check named Swagger Avocado has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide

@openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Sep 11, 2024

@GrahamMThomas GrahamMThomas marked this pull request as ready for review October 18, 2024 18:01
@GrahamMThomas GrahamMThomas requested a review from a team as a code owner October 18, 2024 18:01
@GrahamMThomas GrahamMThomas requested review from marclerwick and tg-msft and removed request for a team October 18, 2024 18:01
@AzureRestAPISpecReview AzureRestAPISpecReview added new-api-version and removed VersioningReviewRequired <valid label in PR review process>add this label when versioning review is required labels Oct 21, 2024
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft left a comment

Choose a reason for hiding this comment

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

I wasn't in the review meeting. If any of my feedback is already resolved there, feel free to "resolve" that comment.

I didn't see response to review meeting feedback of #30515 (comment)
I'd expect a simple response to it (e.g. which item is resolved, which is not).

@azure-sdk
Copy link
Collaborator

azure-sdk commented Nov 26, 2024

API change check

APIView has identified API level changes in this PR and created following API reviews.

HealthDataAIServices.DeidServices-HealthDataAIServices.DeidServices

@GrahamMThomas
Copy link
Contributor Author

I wasn't in the review meeting. If any of my feedback is already resolved there, feel free to "resolve" that comment.

I didn't see response to review meeting feedback of #30515 (comment) I'd expect a simple response to it (e.g. which item is resolved, which is not).

I've added TODOs for the customizationOptions doc link as we don't have the docs built yet.

Regarding the "Recommend looking at Document Translator for naming consistency", do you remember what specifically which models we were trying to sync?

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Nov 27, 2024

I've added TODOs for the customizationOptions doc link as we don't have the docs built yet.

Regarding the "Recommend looking at Document Translator for naming consistency", do you remember what specifically which models we were trying to sync?

I went a recap of the meeting. I think the "naming consistency" is about the name of locale property. From what I get, the decision is to use industrial standard for the name.

PS: ideally, during SDK review, the changes would be limited to the customization within client.tsp (rename, restructure the clients, etc.), which only need SDK dev approval.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft left a comment

Choose a reason for hiding this comment

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

LGTM.

A few minor comments, but no major problem to me.

@XiaofeiCao
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@weidongxu-microsoft
Copy link
Member

SDK automation for Java is expected to fail, as the customization is not yet in Java SDK repo

customization-class: "customization/src/main/java/ListJobsCustomization.java"

Copy link
Member

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

"schema": {
"$ref": "#/definitions/Azure.Core.Foundations.ErrorResponse"
},
"headers": {
Copy link
Member

Choose a reason for hiding this comment

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

You probably want the x-ms-client-request-id defined for error responses as well -- that's where it is really needed. This is a bug in TypeSpec and there is already an open issue for it:

https://github.com/Azure/typespec-azure-pr/issues/3427

cc: @allenjzhang @mario-guerra

@mikekistler mikekistler added the APIStewardshipBoard-SignedOff The Azure API Stewardship team has reviewed and approved the changes. label Dec 6, 2024
@GrahamMThomas GrahamMThomas merged commit 45baf6e into Azure:main Dec 6, 2024
pjpatel12 pushed a commit to pjpatel12/azure-rest-api-specs that referenced this pull request Apr 29, 2025
…nges (Azure#30518)

* Adds CustomizationOptions

* Regenerates Examples

* Model tweaks

* Updates examples

* Updates after team meeting

* Renames customization options

* Introduces new version

* Revert old version

* Fixes pipeline failures

* Recompile

* Location is no longer a sas uri

* Fixes models prettier

* Rename variables in csharp only

* tsp format

* Latest updates from SDK Review

* Add OperationType model for realtime

* Change Java service name from DeidServices (default) to Deidentification

* get*Count() for Java

* Rename DeidentificationClient in JS

* WIP

* Attempt switching to raw operation

* Round 2

* Round 3

* Round 4

* Initial examples

* Rename to CustomizationOptions

* Mark listJobs and listJobDocuments as internal

* csharp-specific changes

* Add customization for java in tspconfig

* npm update

* Changes for python

* No longer need python method to be internal

* Re-ran e2e script

* Revert "Merge branch 'main' into healthdataaiservices/customization-options"

This reverts commit a355b96, reversing
changes made to 3803319.

* tsp format

* Delete unnecessary files

* Reapply "Merge branch 'main' into healthdataaiservices/customization-options"

This reverts commit db141b4.

* Restore package-lock.json

* Remove HTTPS prefix

* Rename body to content in C#

* Change endpoint from str to url

* Typespec validation

* Deprecate 2024-07-12 examples; Updates examples

* Validation fixes 1

* Prefixes customizationoptions with deidentification

* Adds back in old spec

* Adds back in old spec to readme.md

* REVERT - Adds back in old spec to readme.md

* Moving Azure.ClientGenerator.Core to client.tsp

* Use clientName in routes.tsp for renaming "body"

* Delete specification/healthdataaiservices/HealthDataAIServices.DeidServices/.gitignore

* Use ResourceList and alias

* e2e with name instead of jobName

* Deprecates Partial Failed

* Adds todos for doc links

* Adds doc link to redaction format

* Recompiles

* Tsp format

* Removes minimum length restriction from prefix to allow whole container prefix

---------

Co-authored-by: Graham Thomas <gthomas@microsoft.com>
Co-authored-by: Alexa Thomases <athomases@microsoft.com>
Co-authored-by: alexathomases <55257302+alexathomases@users.noreply.github.com>
Co-authored-by: Mike Soennichsen <msoennichsen@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

APIStewardshipBoard-SignedOff The Azure API Stewardship team has reviewed and approved the changes. data-plane Health Deidentification new-api-version PublishToCustomers Acknowledgement the changes will be published to Azure customers. TypeSpec Authored with TypeSpec

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Health Data DeIdentification - Azure Health AI Deidentification service ] API Review

8 participants