Skip to content

health insights sdk for net#34812

Merged
chunyu3 merged 19 commits intoAzure:mainfrom
asaflevi-ms:asaflevi/feature/healthinsights-sdk-for-net
Mar 23, 2023
Merged

health insights sdk for net#34812
chunyu3 merged 19 commits intoAzure:mainfrom
asaflevi-ms:asaflevi/feature/healthinsights-sdk-for-net

Conversation

@asaflevi-ms
Copy link
Member

Contributing to the Azure SDK

Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.

For specific information about pull request etiquette and best practices, see this section.

@asaflevi-ms
Copy link
Member Author

This is my first PR for Health Insights - first public preview.

@pallavit
Copy link
Contributor

I do not see an autorest.md file in the PR. Can you please add it and share what settings were used to generate the autorest client?

@pallavit pallavit self-assigned this Mar 10, 2023
@chunyu3
Copy link
Member

chunyu3 commented Mar 13, 2023

I do not see an autorest.md file in the PR. Can you please add it and share what settings were used to generate the autorest client?

This RP is generated from CADL, not swagger, so there is no autorest.md to set configuration. But @asaflevi-ms You need to add cadl-location.yaml to configure the cadl project. please refer to https://github.com/Azure/azure-sdk-for-net/blob/cabc120930f32aabc57a3556fa9bd60cbd6e4314/doc/DataPlaneCodeGeneration/AzureSDKCodeGeneration_DataPlane_Quickstart.md#configuration-optional

@asaflevi-ms
Copy link
Member Author

/azp run prepare-pipelines

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@asaflevi-ms
Copy link
Member Author

I do not see an autorest.md file in the PR. Can you please add it and share what settings were used to generate the autorest client?

This RP is generated from CADL, not swagger, so there is no autorest.md to set configuration. But @asaflevi-ms You need to add cadl-location.yaml to configure the cadl project. please refer to https://github.com/Azure/azure-sdk-for-net/blob/cabc120930f32aabc57a3556fa9bd60cbd6e4314/doc/DataPlaneCodeGeneration/AzureSDKCodeGeneration_DataPlane_Quickstart.md#configuration-optional

Thank you @chunyu3 for your comment.
@pallavit , PR updated with cadl-project.yaml files (one for each package)
prepare-pipelines run successfully.

if needed, this is my Cadl PR: Azure/azure-rest-api-specs#22990

@azure-sdk
Copy link
Collaborator

azure-sdk commented Mar 14, 2023

API change check

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

Azure.Health.Insights.ClinicalMatching
Azure.Health.Insights.CancerProfiling

@asaflevi-ms
Copy link
Member Author

asaflevi-ms commented Mar 15, 2023

@chunyu3, @pallavit

Eventually, the CI fails (https://dev.azure.com/azure-sdk/internal/_build/results?buildId=2631978&view=logs&j=b70e5e73-bbb6-5567-0939-8415943fadb9&t=a880e989-7d1a-5c96-a41f-d540b383cc43.)

I checked and it looks like the TempCadlFiles contains only single directory which is not enough.

I tested it locally and updated cadl-location.yaml to contains additionalDirectories:

"additionalDirectories: specification/cognitiveservices/HealthInsights/."

Now the entire HealthInsights folder is downloaded as expected.
However, since the main directory for the client.cadl is ...../healthinsights.trialmatcher.
node_modules is created under this folder, thus when trying to build other depended cadl files, I am getting:
error import-not-found: Couldn't resolve import "@cadl-lang/rest"

I am not sure how to overcome this issue.
I need your advice for how to resolve this and be able to build multiple packages .
(check my cadl project layout: Azure/azure-rest-api-specs#22990)

@pallavit pallavit removed their assignment Mar 15, 2023
ghost pushed a commit to Azure/azure-sdk-tools that referenced this pull request Mar 15, 2023
@chunyu3
Copy link
Member

chunyu3 commented Mar 16, 2023

@chunyu3, @pallavit

Eventually, the CI fails (https://dev.azure.com/azure-sdk/internal/_build/results?buildId=2631978&view=logs&j=b70e5e73-bbb6-5567-0939-8415943fadb9&t=a880e989-7d1a-5c96-a41f-d540b383cc43.)

I checked and it looks like the TempCadlFiles contains only single directory which is not enough.

I tested it locally and updated cadl-location.yaml to contains additionalDirectories:

"additionalDirectories: specification/cognitiveservices/HealthInsights/."

Now the entire HealthInsights folder is downloaded as expected. However, since the main directory for the client.cadl is ...../healthinsights.trialmatcher. node_modules is created under this folder, thus when trying to build other depended cadl files, I am getting: error import-not-found: Couldn't resolve import "@cadl-lang/rest"

I am not sure how to overcome this issue. I need your advice for how to resolve this and be able to build multiple packages . (check my cadl project layout: Azure/azure-rest-api-specs#22990)

Hello @asaflevi-ms If you need additional directory, you can set additionalDirectories in the configuration. you can refer to https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/openai/Azure.AI.OpenAI/cadl-location.yaml.

@lirenhe
Copy link
Member

lirenhe commented Mar 22, 2023

cc @Azure/dpg-devs for awareness

@asaflevi-ms
Copy link
Member Author

Hi @pallavit , @chunyu3
What else need to do to get approved and merged

@chunyu3 chunyu3 merged commit 94b509a into Azure:main Mar 23, 2023
@asaflevi-ms asaflevi-ms deleted the asaflevi/feature/healthinsights-sdk-for-net branch March 30, 2023 13:42
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.

6 participants