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

Azdevify chat-with-your-data-solution-accelerator. #2

Merged
merged 45 commits into from
Feb 6, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
24be759
azdevify this sample and add azd new feature in devcontainer
Nov 5, 2023
fbfd9b7
instead parameters.json file with .bicepparam
Nov 5, 2023
d32d977
processing format
Nov 7, 2023
552f254
keep consistent casing for params
Nov 7, 2023
c00f55a
move appsettings
Nov 7, 2023
b2e62b2
remove some invalid changes
Nov 9, 2023
b1a97b7
fix format issues
Nov 9, 2023
625487e
Remove depandsOn
Nov 9, 2023
65ac3c5
remove resources file
Nov 10, 2023
43b09e7
remov keys from output
Nov 10, 2023
bd29d31
remove existing reference in module
Nov 10, 2023
8587450
update ReadMe
Nov 15, 2023
afd3ca8
add rgName in listkeys
Nov 22, 2023
654762e
add use keyvault option
Nov 23, 2023
cc3e957
set default value in .bicepparam
Nov 23, 2023
9dc6a78
fix some bugs
Nov 28, 2023
9bcb2a7
update rbac
Dec 14, 2023
1a9212b
update rbac
Dec 21, 2023
eb11eab
fix some format and update infra core
Jan 3, 2024
a403d25
Update openai to v1
ChenxiJiang333 Jan 3, 2024
b88ee5f
use resourcegroup.name() instead pass in rhName
Jan 3, 2024
4bdf8c3
Merge pull request #1 from zedy-wj/openaiv1
zedy-wj Jan 3, 2024
1ecb9be
Revert "Update openai to v1"
zedy-wj Jan 3, 2024
75f230e
Merge pull request #2 from zedy-wj/revert-1-openaiv1
zedy-wj Jan 3, 2024
969aa25
add token provider
ChenxiJiang333 Jan 4, 2024
ffee3f8
Merge branch 'azdevify' into openaiv1
ChenxiJiang333 Jan 4, 2024
778e1d4
Merge pull request #3 from zedy-wj/openaiv1
zedy-wj Jan 4, 2024
5892d5d
Update requirements.txt
ChenxiJiang333 Jan 4, 2024
ce7d48d
Merge pull request #4 from zedy-wj/ChenxiJiang333-patch-1
zedy-wj Jan 4, 2024
2b683de
revert
Jan 4, 2024
397ab11
fix chat
ChenxiJiang333 Jan 4, 2024
1e21f2a
Update TextProcessingTool.py
ChenxiJiang333 Jan 4, 2024
9481a66
Merge pull request #5 from zedy-wj/newcommit
zedy-wj Jan 5, 2024
b25cce5
update KeyVault options
Jan 10, 2024
332402a
abstract auth type with define method
Jan 11, 2024
bd092df
remove invalid infra/core
Jan 11, 2024
5406c5a
update readme and storage.bicep in infra/core
Jan 12, 2024
d2145ec
Update output format
Jan 17, 2024
c83a409
default to use rbac
Jan 22, 2024
aa86081
add some comments
Jan 26, 2024
06bb879
solve conflicts
ChenxiJiang333 Jan 31, 2024
1a7303a
add readme and speech service related code in main.bicep
Feb 1, 2024
2c8abfb
fix azd deploy problem
Feb 2, 2024
5dcf10c
move code folder structure
Feb 5, 2024
779065f
fix conflict
Feb 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions .devcontainer/devcontainer.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

Can you investigate this flow:

  1. Use RBAC
  2. Store and Use Keys in KeyVault. If the KV isn't specified, then try to read the values from the service with listKeys.

The above adds more complexity, but allows the user to store the keys in KV as an option.

Choose a reason for hiding this comment

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

Just linking this as we should revisit the Speech Service using this method too: - Azure-Samples#101 (not for this PR!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use of KeyVault has been added and is currently testing well.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the back and forth on this. Let's do this:

Default to using rbac.

If AUTH_TYPE is rbac, then assume the developer wants to also use key vault. Put the keys in keyvault and then read them from keyvault.

If the user sets AUTH_TYPE to keys, then read the keys from the environment and set the keys using listKeys function.

I think that will be cleaner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In our understanding, if AUTH_TYPE is rbac, then some App Services and User should be assigned corresponding permissions in the main.bicep file. This means that keys will no longer be needed in the code, but the service will be accessed directly through something like DefaultAzureCredential().

So regarding what you mention here, If AUTH_TYPE is rbac, then assume the developer wants to also use key vault., which confuses us. Because in our concept, one of them is enough.

If we have any misunderstandings about rbac or your comments, please let me know, thanks.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, only use a key vault if there's no RBAC option for the service.

Copy link
Collaborator Author

@zedy-wj zedy-wj Jan 22, 2024

Choose a reason for hiding this comment

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

That's the logic in our code so far, please re-review it, thanks!

Yes, only use a key vault if there's no RBAC option for the service.

Default to using rbac: https://github.com/jongio/chat-with-your-data-solution-accelerator/pull/2/files#diff-c274a6091f4ca06948f0fe1ab8681ec4eb6b4e98c4be09717a2e3cacd1344727R9-R12

If AUTH_TYPE is rbac, the developer must choose not to use the KeyVault, if AUTH_TYPE is keys, the developer can choose to use the KeyVault or directly use listKeys().

"name": "Chat with your data Solution Accelerator",
"image": "mcr.microsoft.com/devcontainers/python:3.11",
"features": {
"ghcr.io/devcontainers/features/node:1": {
"version": "16",
"nodeGypDependencies": false
},
"ghcr.io/devcontainers/features/powershell:1.1.1": {},
"ghcr.io/devcontainers/features/azure-cli:1.2.1": {},
"ghcr.io/azure/azure-dev/azd:latest": {}
},
"customizations": {
"vscode": {
"extensions": [
"ms-azuretools.azure-dev",
"ms-azuretools.vscode-bicep",
"ms-python.python",
"esbenp.prettier-vscode"
]
}
},
"forwardPorts": [
50505
],
"postCreateCommand": "",
"remoteUser": "vscode",
"hostRequirements": {
"memory": "8gb"
}
}
38 changes: 38 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,44 @@ Out-of-the-box, you can upload the following file types:

![A screenshot of the chat app.](./media/chat-app.png)

## Running the sample using Azd template
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved

The Azure Developer CLI (`azd`) is a developer-centric command-line interface (CLI) tool for creating Azure applications.

You need to install it before running and deploying with Azure Developer CLI.
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved

### Windows

```powershell
powershell -ex AllSigned -c "Invoke-RestMethod 'https://aka.ms/install-azd.ps1' | Invoke-Expression"
```

### Linux/MacOS

```
curl -fsSL https://aka.ms/install-azd.sh | bash
```

After logging in with the following command, you will be able to use the azd cli to quickly provision and deploy the application.
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved

```
azd auth login
```

Then, execute the `azd init` command to initialize the environment.
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved
```
azd init -t jongio/chat-with-your-data-solution-accelerator
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved
```
According to the prompt, enter an `env name`.
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved

Run `azd up` to provision all the resources to Azure and deploy the code to those resources.
```
azd up
```

According to the prompt, select `subscription` and `location`, these are the necessary parameters when you create resources. After that, choose a resource group created or create a new resource group. Wait a moment for the resource deployment to complete, click the Website endpoint and you will see the web app page.
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved

You can also run the sample directly locally (See below).

## Development and run the accelerator locally
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
26 changes: 26 additions & 0 deletions azure.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# yaml-language-server: $schema=https://raw.githubusercontent.com/Azure/azure-dev/main/schemas/v1.0/azure.yaml.json

name: chat-with-your-data-solution-accelerator

zedy-wj marked this conversation as resolved.
Show resolved Hide resolved
services:
web:
project: .
language: py
host: appservice
hooks:
prepackage:
windows:
shell: pwsh
run: cd ./frontend;npm install;npm run build
interactive: true
continueOnError: false

adminweb:
project: ./backend
language: py
host: appservice

function:
project: ./backend
language: py
host: function
40 changes: 40 additions & 0 deletions infra/app/adminweb.bicep
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
param name string
param location string = resourceGroup().location
param tags object = {}

param allowedOrigins array = []
param appServicePlanId string
param appCommandLine string = 'python -m streamlit run Admin.py --server.port 8000 --server.address 0.0.0.0 --server.enableXsrfProtection false'
param applicationInsightsName string = ''
param keyVaultName string = ''
param azureOpenAIName string = ''
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved
param azureCognitiveSearchName string = ''

@secure()
param appSettings object = {}
param serviceName string = 'adminweb'

module adminweb '../core/host/appservice.bicep' = {
name: '${name}-app-module'
params: {
name: name
location: location
tags: union(tags, { 'azd-service-name': serviceName })
allowedOrigins: allowedOrigins
appCommandLine: appCommandLine
applicationInsightsName: applicationInsightsName
appServicePlanId: appServicePlanId
appSettings: union(appSettings, {
AZURE_OPENAI_KEY: listKeys('Microsoft.CognitiveServices/accounts/${azureOpenAIName}', '2023-05-01').key1
AZURE_SEARCH_KEY: listAdminKeys('Microsoft.Search/searchServices/${azureCognitiveSearchName}', '2021-04-01-preview').primaryKey
Copy link
Owner

Choose a reason for hiding this comment

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

This only works if the openai and search are in the same resource group. You will have to pass the keys into this from main.bicep or better yet, put the keys in keyvault and then just store the names here to be retrieved by the application.

Copy link
Collaborator Author

@zedy-wj zedy-wj Nov 14, 2023

Choose a reason for hiding this comment

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

Hi, @jongio - We've tried a lot of things here on how to set keys to appSettings. Before making this change, I'd like to discuss best practices with you.

1.If we want to set resources.keys to appSettings in the main.bicep file, there are three methods we have tried so far:

  • Create relevant resources directly in the main.bicep file, and then use resources.listKeys().key1. In this way, we cannot use templates in core to create related resources.
  • Use module in main.bicep to reference related templates in core to create resources, but at this time, resource.listKeys().key1 cannot be used because the resource is not created at the level of main.bicep; listKeys(resourceId).key1 cannot be used either ( At present, the priority of the listKeys method is higher than that of module, unless we create the resource first).
  • After we use the module to create the resource, use existing in main.bicep to call the relevant resources and then use resource.listKeys().key1. But this will also cause problems. The priority of existing is higher than module. We need to ensure that the resource is created in advance before we use existing.

Based on the results of our investigation so far, if you have any questions or if you have other best practices on how to set keys to appSettings in main.bicep, please let us know. Thank you very much!

2.About put the keys in keyvault and then just store the names here to be retrieved by the application:

  • We also tried this in another azdevify PR. We refer to the approach in todo template. If we want to store the key of a resource, we need to reference KeyVault when creating the resource. Currently, the template for creating openAi in core does not have this function, which means that we cannot use the template in core when creating related resources.

Do you have any ideas on this? Our current idea is to add the function of storing resource's keys in the relevant templates in core: add a param keyVaultName, which is empty by default. If this value is empty, the resource 'Microsoft.KeyVault/vaults/secrets@2022-07-01' will not be called. If keyVaultName is passed in externally, the key of this resource can be stored in KeyVault.

Copy link

Choose a reason for hiding this comment

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

@jongio Any ideas about this issue?

Copy link
Owner

@jongio jongio Nov 17, 2023

Choose a reason for hiding this comment

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

We really shouldn't be using KEYS and instead use RBAC like we do in all the other samples.

Example:

You assign the right roles to the service principal of the azure resources that need to access those services and don't use KEYs in app settings at all.

  1. Add a parameter to the main.bicep that allows the user to either use RBAC or KEYS. Call it AUTH_TYPE and default it to rbac. And set @Allowed to 'rbac' or 'keys'
  2. If the auth type is rbac, then use roles. If auth type is keys, then use keys.

Use this as example for how to use RBAC https://github.com/Azure-Samples/azure-search-openai-demo/blob/main/infra/main.bicep#L353

For 'keys'
You can look up the keys with this listKeys method. Example for storage.

listKeys(resourceId(subscription().subscriptionId, rgName, 'Microsoft.Storage/storageAccounts', storageAccountName), '2021-09-01').keys[0].value

It's like what you had before, but you also include the subscriptionId and RgName

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently we use KeyVault to store all keys and then obtain the keys from the application. What do you think of this? If you insist on using rbac, the way of creating various service clients in the application code needs to be changed accordingly. Then the gap between local deployment and deployment using azd will be slightly too large.

Copy link
Owner

Choose a reason for hiding this comment

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

I see what you mean. Could you come up with a version of the code that shows what it would look like to instantiate the objects based on rbac or keys? Just so I can see what the code would look like. Doesn't have to be for them all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jongio - Sorry for the delay. We have done some exploration and attempts on how to use rbac. Details as following:

Currently, a total of five Keys are used in our appSettings, which are: AZURE_OPENAI_KEY, AZURE_SEARCH_KEY, AZURE_BLOB_ACCOUNT_KEY, AZURE_FORM_RECOGNIZER_KEY, AZURE_CONTENT_SAFETY_KEY.

We divide it into three categories:

  1. Cognitive Services (AZURE_OPENAI_KEY, AZURE_FORM_RECOGNIZER_KEY, AZURE_CONTENT_SAFETY_KEY):
  • For this type of Keys, we can add rbac permissions to openai, and then use the following method to assign values to the Keys:
    image
  1. AZURE_BLOB_ACCOUNT_KEY:
  • We only need to add relevant permissions to storage and change the way to create BlobServiceClient:
    image
  1. AZURE_SEARCH_KEY:
  • It needs to be used to create the request body in the application. We have not found a suitable replacement method yet:
    imageIf we want to change it, we can refer to azure-search-openai-demo, but the change will be large.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, I didn't know they aren't using the search SDK. I'm asking them about that and will get back to you.

})
keyVaultName: keyVaultName
runtimeName: 'python'
runtimeVersion: '3.11'
scmDoBuildDuringDeployment: true
}
}

output WEBSITE_ADMIN_IDENTITY_PRINCIPAL_ID string = adminweb.outputs.identityPrincipalId
output WEBSITE_ADMIN_NAME string = adminweb.outputs.name
output WEBSITE_ADMIN_URI string = adminweb.outputs.uri
61 changes: 61 additions & 0 deletions infra/app/function.bicep
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
param name string
param location string = ''
param appServicePlanId string
param storageAccountName string = ''
param tags object = {}

@secure()
param appSettings object = {}
param serviceName string = 'function'
param runtimeName string = 'python'
param runtimeVersion string = '3.11'

@secure()
param clientKey string
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved
param azureOpenAIName string = ''
param azureCognitiveSearchName string = ''


module function '../core/host/functions.bicep' = {
name: '${name}-app-module'
params: {
name: name
location: location
tags: union(tags, { 'azd-service-name': serviceName })
appServicePlanId: appServicePlanId
storageAccountName: storageAccountName
runtimeName: runtimeName
runtimeVersion: runtimeVersion
appSettings: union(appSettings, {
AZURE_OPENAI_KEY: listKeys('Microsoft.CognitiveServices/accounts/${azureOpenAIName}', '2023-05-01').key1
AZURE_SEARCH_KEY: listAdminKeys('Microsoft.Search/searchServices/${azureCognitiveSearchName}', '2021-04-01-preview').primaryKey
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved
})
}
}

resource functionNameDefaultClientKey 'Microsoft.Web/sites/host/functionKeys@2018-11-01' = {
name: '${name}/default/clientKey'
properties: {
name: 'ClientKey'
value: clientKey
}
dependsOn: [
function
waitFunctionDeploymentSection
]
}
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved

resource waitFunctionDeploymentSection 'Microsoft.Resources/deploymentScripts@2020-10-01' = {
kind: 'AzurePowerShell'
name: 'WaitFunctionDeploymentSection'
location: location
properties: {
azPowerShellVersion: '3.0'
scriptContent: 'start-sleep -Seconds 300'
cleanupPreference: 'Always'
retentionInterval: 'PT1H'
}
dependsOn: [
function
]
}
41 changes: 41 additions & 0 deletions infra/app/web.bicep
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
param name string
param location string = resourceGroup().location
param tags object = {}

param allowedOrigins array = []
param appCommandLine string = ''
param appServicePlanId string
param applicationInsightsName string = ''
param keyVaultName string = ''
param azureOpenAIName string = ''
param azureCognitiveSearchName string = ''

@secure()
param appSettings object = {}
param serviceName string = 'web'

module web '../core/host/appservice.bicep' = {
name: '${name}-app-module'
params: {
name: name
location: location
tags: union(tags, { 'azd-service-name': serviceName })
allowedOrigins: allowedOrigins
appCommandLine: appCommandLine
applicationInsightsName: applicationInsightsName
appServicePlanId: appServicePlanId
appSettings: union(appSettings, {
AZURE_OPENAI_KEY: listKeys('Microsoft.CognitiveServices/accounts/${azureOpenAIName}', '2023-05-01').key1
AZURE_SEARCH_KEY: listAdminKeys('Microsoft.Search/searchServices/${azureCognitiveSearchName}', '2021-04-01-preview').primaryKey
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved
})

keyVaultName: keyVaultName
runtimeName: 'python'
runtimeVersion: '3.11'
scmDoBuildDuringDeployment: true
}
}

output FRONTEND_API_IDENTITY_PRINCIPAL_ID string = web.outputs.identityPrincipalId
output FRONTEND_API_NAME string = web.outputs.name
output FRONTEND_API_URI string = web.outputs.uri
42 changes: 42 additions & 0 deletions infra/core/ai/cognitiveservices.bicep
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
metadata description = 'Creates an Azure Cognitive Services instance.'
zedy-wj marked this conversation as resolved.
Show resolved Hide resolved
param name string
param location string = resourceGroup().location
param tags object = {}
@description('The custom subdomain name used to access the API. Defaults to the value of the name parameter.')
param customSubDomainName string = name
param deployments array = []
param kind string = 'OpenAI'
param publicNetworkAccess string = 'Enabled'
param sku object = {
name: 'S0'
}

resource account 'Microsoft.CognitiveServices/accounts@2023-05-01' = {
name: name
location: location
tags: tags
kind: kind
properties: {
customSubDomainName: customSubDomainName
publicNetworkAccess: publicNetworkAccess
}
sku: sku
}

@batchSize(1)
resource deployment 'Microsoft.CognitiveServices/accounts/deployments@2023-05-01' = [for deployment in deployments: {
parent: account
name: deployment.name
properties: {
model: deployment.model
raiPolicyName: contains(deployment, 'raiPolicyName') ? deployment.raiPolicyName : null
}
sku: contains(deployment, 'sku') ? deployment.sku : {
name: 'Standard'
capacity: 20
}
}]

output endpoint string = account.properties.endpoint
output id string = account.id
output name string = account.name
17 changes: 17 additions & 0 deletions infra/core/host/appservice-appsettings.bicep
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
metadata description = 'Updates app settings for an Azure App Service.'
@description('The name of the app service resource within the current resource group scope')
param name string

@description('The app settings to be applied to the app service')
@secure()
param appSettings object

resource appService 'Microsoft.Web/sites@2022-03-01' existing = {
name: name
}

resource settings 'Microsoft.Web/sites/config@2022-03-01' = {
name: 'appsettings'
parent: appService
properties: appSettings
}
Loading