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

Open ai with Locked down bot azd template #168

Merged

Conversation

graemefoster
Copy link
Contributor

@graemefoster graemefoster commented Jul 16, 2023

Please fill out this template! There are three different types of contributions, feel free to delete the checklists that are not applicable to your contribution type.

If you are submitting a new azd template to the gallery

Fill this out if you want your template to be added to the awesome-azd gallery!

  • Added an entry to https://github.com/Azure/awesome-azd/blob/main/website/src/data/users.tsx that includes:

    • Template title - A short title that reflects the local application stack that someone could use to get their application on Azure (e.g. "Containerized React Web App with Java API and MongoDB")
    • Description - 1-2 sentence description of the architecture (e.g. Azure services) or solution that is defined by the template.
    • Architecture Diagram or Application Screenshot - Used as display image for gallery card. The image should include all services and their connections (example). You should add the image to the website/src/data/images/.
    • Link to Author's GitHub or other relevant website - Used for attribution
    • Author's Name - Name to credit on the gallery card
    • Link to template source - Link to the template GitHub repo
    • Tags - One or more tags representing the template. Provide at least 1 tag for programming language used and at least 1 tag for Azure services integrated. Also tag the IaC provider (Bicep or Terraform). If you don't see a relevant tag for your template? Feel free to add one!
  • In the PR comment, if you can also add a link to the PR where you made your repo azd compatible this will allow us to provide feedback on your template and speed up the review process.

If you are submitting a resource to be added to the awesome-azd README:

  • [] Name of resource
  • [] Resource author - who created this resource? (so we can credit them!)
  • [] What section should this resource be included in? -Is the resource an article? A video? Something else?

@hemarina
Copy link
Contributor

@v-xuto Would you test this template when you get a chance?
@jongio Would you take a review on this template?

@v-xuto
Copy link
Member

v-xuto commented Jul 25, 2023

@v-xuto Would you test this template when you get a chance?

Sure. We will start testing this template today.

@v-xuto
Copy link
Member

v-xuto commented Jul 28, 2023

@hemarina We have finished the template test, and we have filed two issues. Please review.

@graemefoster
Copy link
Contributor Author

graemefoster commented Jul 28, 2023

@hemarina We have finished the template test, and we have filed two issues. Please review.

Great @v-xuto - let me fix these up. Thanks for the initial attempt and apologies it didn't deploy.

@graemefoster
Copy link
Contributor Author

@v-xuto - should be good to test again. I've updated my repo's readme.md file to show the various ways to use this template (controlled by azd env variables).

@v-xuto
Copy link
Member

v-xuto commented Aug 3, 2023

@v-xuto - should be good to test again. I've updated my repo's readme.md file to show the various ways to use this template (controlled by azd env variables).

@graemefoster We have retested this template, we found one new issue. Please help review and resolve.

@savannahostrowski savannahostrowski removed their request for review August 3, 2023 17:36
@graemefoster
Copy link
Contributor Author

Thanks @v-xuto . This looks like a capacity problem on your subscription rather than a template problem so I don't think I can do anything to help here.... See Azure-Samples/azure-search-openai-demo#313 for a similar problem.

I've lowered the default capacity on mine to 20, but have you purged the other OpenAI services you've created from other templates? FWIW this one (https://github.com/pascalvanderheiden/ais-apim-openai) just got approved with pretty much the same bicep mine has.

Hopefully this helps you get mine merged in.

@v-jiaodi
Copy link
Member

v-jiaodi commented Aug 4, 2023

@graemefoster For graemefoster/LockedDownChatBot#3, I had added new comments.

@graemefoster
Copy link
Contributor Author

graemefoster commented Aug 4, 2023

@graemefoster For graemefoster/LockedDownChatBot#3, I had added new comments.

Thanks @v-xuto. I've pushed a new version that allows you to specify OpenAi location using the env var "OPENAI_LOCATION", and added doco for it. I've kept the default as 'eastus'. Problem I have is that OpenAI isn't available in most locations, so I have to choose a default at the very least.

When I deploy I default to australiaeast which doesn't have OpenAi, but if you default to eastus2 then you might be letting templates through that will break for OpenAi in locations that don't have it.

Thanks,

Graeme

@v-xuto
Copy link
Member

v-xuto commented Aug 8, 2023

@hemarina Issue graemefoster/LockedDownChatBot#3 has been fixed, currently this template has no issues.

Copy link
Contributor

@hemarina hemarina left a comment

Choose a reason for hiding this comment

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

LGTM. CC @jongio in case for more suggestions on repro or bicep files.

@jongio
Copy link
Member

jongio commented Aug 14, 2023

Not required, but it would be great if you could use the azd core infra that we use in other azd templates. There's some good modules and code in yours that would be good to share with other azd users as well. You could replace a lot of the bicep code in your template as well. You can find them here: https://github.com/Azure/azure-dev/tree/main/templates/common/infra/bicep

@jongio
Copy link
Member

jongio commented Aug 14, 2023

In order for a user to use an existing resource in a different location you'll need to expose a location param for each resource group. You can see how we do that in this template here: https://github.com/Azure-Samples/azure-search-openai-demo-csharp/blob/main/infra/main.bicep#L37

@graemefoster
Copy link
Contributor Author

graemefoster commented Aug 17, 2023

In order for a user to use an existing resource in a different location you'll need to expose a location param for each resource group. You can see how we do that in this template here: https://github.com/Azure-Samples/azure-search-openai-demo-csharp/blob/main/infra/main.bicep#L37

Are you sure about this @jongio? The template as-is works fine with an existing OpenAI in a different resource-group / location and BIcep doesn't allow you to provide a 'location' when the resource is marked as existing.

Whilst developing I had all new resources in AU-East, and OpenAI in EastUS. The template allows you to override the eastus location using the openAiLocation parameter.

(The template you linked to doesn't reference an existing resource - it creates a new one in a different location.)

@graemefoster
Copy link
Contributor Author

Not required, but it would be great if you could use the azd core infra that we use in other azd templates. There's some good modules and code in yours that would be good to share with other azd users as well. You could replace a lot of the bicep code in your template as well. You can find them here: https://github.com/Azure/azure-dev/tree/main/templates/common/infra/bicep

I'll see what I can re-use. Would be great to package these into a container so we could reference them. They look to change quite often.

@graemefoster
Copy link
Contributor Author

graemefoster commented Aug 17, 2023

Not required, but it would be great if you could use the azd core infra that we use in other azd templates. There's some good modules and code in yours that would be good to share with other azd users as well. You could replace a lot of the bicep code in your template as well. You can find them here: https://github.com/Azure/azure-dev/tree/main/templates/common/infra/bicep

This might take a bit more time... Unfortunately what Bicep gives with one hand, it takes with the other... As an example, I tried an easy one and found that my RBAC would need to move either into the OpenAI module (complicating it), or into another module.

I then have to pass loosey-goosey object arrays around and lose all type safety making the Bicep hard to debug... I'd use strong-typing but I've had issues before with it, and I think it is still in preview. :(

image

@jongio
Copy link
Member

jongio commented Aug 22, 2023

Okay, you are right, it looks like location isn't required when you reference an existing resource, but is when you add a resource to an existing resource group.

Yes, the modules will be in a registry soon. Tracked: Azure/azure-dev#751

What I suggest is that we get this in awesome-azd as-is and then we can integrate modules later.

@hemarina
Copy link
Contributor

Thank you all for cooperating and getting this template added to the gallery. I'll merge it. Let me know if there're any questions or concerns.

@hemarina hemarina merged commit 2a793dc into Azure:main Aug 23, 2023
2 checks passed
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.

5 participants