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

azurerm_cognitive_deployment - Add support for VersionUpgradeOption #22520

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

lonegunmanb
Copy link
Contributor

Add support for VersionUpgradeOption so the users can set the model's version they like. This pr should fix #22501 .

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @lonegunmanb, this change is a good start but we should write a test for this as well. It also looks like the API defaults this value to OnceNewDefaultVersionAvailable so we'll want to account for that as well

@lonegunmanb
Copy link
Contributor Author

Roger that, will do

@lonegunmanb
Copy link
Contributor Author

Hi @mbfrahry I have to change the model used in e2e test since the original model is deprecated now. Would you please give this pr a review? Thanks!

@lonegunmanb lonegunmanb requested a review from mbfrahry July 15, 2023 11:27
@manicminer
Copy link
Contributor

@lonegunmanb Thanks for updating the tests. It looks like the model got reverted to the older one when main got merged in, was this intentional?

@lonegunmanb
Copy link
Contributor Author

@lonegunmanb Thanks for updating the tests. It looks like the model got reverted to the older one when main got merged in, was this intentional?

Whoops... My bad, I must mess it up when I tried to merge the code, thanks for pointing out.

@lonegunmanb
Copy link
Contributor Author

Hi @manicminer @mbfrahry I've rebased the pr to the latest main branch, and I've upgraded the model version in the e2e test so all acc tests could pass on my side. Would you please give this r another review? Thanks!

@manicminer
Copy link
Contributor

Thanks @lonegunmanb. I'm getting a test failure with a 400 response.

Screenshot 2023-07-20 at 00 06 24

@lonegunmanb
Copy link
Contributor Author

I'll see what I can do to solve this testing issue.

@lonegunmanb
Copy link
Contributor Author

Hi @manicminer I've tried to switch the region but still got no luck. I've checked the team city's history, the same errors occurred before on the main branch. The tests got the green light on our testing subscription:

=== RUN   TestAccCognitiveDeploymentSequential
--- PASS: TestAccCognitiveDeploymentSequential (784.22s)
=== RUN   TestAccCognitiveDeploymentSequential/deployment
    --- PASS: TestAccCognitiveDeploymentSequential/deployment (784.22s)
=== RUN   TestAccCognitiveDeploymentSequential/deployment/basic
        --- PASS: TestAccCognitiveDeploymentSequential/deployment/basic (262.07s)
=== RUN   TestAccCognitiveDeploymentSequential/deployment/requiresImport
        --- PASS: TestAccCognitiveDeploymentSequential/deployment/requiresImport (269.03s)
=== RUN   TestAccCognitiveDeploymentSequential/deployment/complete
        --- PASS: TestAccCognitiveDeploymentSequential/deployment/complete (253.12s)
PASS

Could we approve this pr for now? Or maybe we need to open a support ticket on your side I guess? Anyway, thanks for reading, waiting for your further instructions.

@manicminer
Copy link
Contributor

manicminer commented Jul 20, 2023

@lonegunmanb We need to be able to test this. It looks like we'll have to submit an application for access to the service

@manicminer manicminer added this to the Blocked milestone Jul 20, 2023
@jayendranarumugam
Copy link
Contributor

Hello @manicminer , Is there any timeline for when this can be verified and merged? We are currently blocked with this. As there is no way for us to upgrade to a new version. Is there any way we can help ? Thanks!

@dkmiller
Copy link

dkmiller commented Aug 8, 2023

@manicminer is there any way we could help with testing?

@MChorfa
Copy link

MChorfa commented Oct 16, 2023

Hello @mbfrahry,
Is there a way to get this through? I can help with the testing as well
Thank you :)

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM ⚙️

@katbyte katbyte merged commit 48a2c87 into hashicorp:main Nov 7, 2023
1 check passed
katbyte added a commit that referenced this pull request Nov 7, 2023
@katbyte katbyte modified the milestones: Blocked, v3.80.0 Nov 7, 2023
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request Nov 10, 2023
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azurerm&#34; updated from &#34;3.79.0&#34; to
&#34;3.80.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.80.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.80.0&#xA;ENHANCEMENTS:&#xA;&#xA;*
`internal/sdk` - Added support for pointer Types in resource models
([#23810](hashicorp/terraform-provider-azurerm#23810
dependencies: updating to `v0.63.0` of
`github.com/hashicorp/go-azure-helpers`
([#23785](hashicorp/terraform-provider-azurerm#23785
dependencies: updating to `v0.20231106.1151347` of
`github.com/hashicorp/go-azure-sdk`
([#23787](hashicorp/terraform-provider-azurerm#23787
`azurerm_cognitive_deployment` - support for the
`version_upgrade_option` property
([#22520](hashicorp/terraform-provider-azurerm#22520
`azurerm_firewall_policy_rule_collection_group` - add support for the
property `http_headers`
([#23641](hashicorp/terraform-provider-azurerm#23641
`azurerm_kubernetes_cluster` - `fips_enabled` can be updated in the
`default_node_pool` without recreating the cluster
([#23612](hashicorp/terraform-provider-azurerm#23612
`azurerm_kusto_cluster` - the cluster `name` can now include dashes
([#23790](hashicorp/terraform-provider-azurerm#23790
`azurerm_postgresql_database` - update the validation of `collation` to
include support for `French_France.1252`
([#23783](https://github.com/hashicorp/terraform-provider-azurerm/issues/23783))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* Data Source: `azurerm_data_protection_backup_vault` -
removing `import` support, since Data Sources don&#39;t support being
imported
([#23820](hashicorp/terraform-provider-azurerm#23820
Data Source: `azurerm_kusto_database` - removing `import` support, since
Data Sources don&#39;t support being imported
([#23820](hashicorp/terraform-provider-azurerm#23820
Data Source: `azurerm_virtual_hub_route_table` - removing `import`
support, since Data Sources don&#39;t support being imported
([#23820](https://github.com/hashicorp/terraform-provider-azurerm/issues/23820))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
@jayendranarumugam
Copy link
Contributor

jayendranarumugam commented Nov 10, 2023

It also looks like the API defaults this value to OnceNewDefaultVersionAvailable so we'll want to account for that as well

Hey @mbfrahry May I know from where this default is defined?

From the doc

null is equivalent to AutoUpgradeWhenExpired. If the Version update policy option is not present in the properties for a model that supports model upgrades this indicates the value is currently null. Once you explicitly modify this value the property will be visible in the studio properties page as well as via the REST API.

AutoUpgradeWhenExpired will be like OnceCurrentVersionExpired. So the default should be OnceCurrentVersionExpired is it ? Due to this change in the PR. My existing tfstate deployment are causing forces replacement

09    # azurerm_cognitive_deployment.generic_deployment["gpt-35-turbo"] must be replaced
16:51:09  -/+ resource "azurerm_cognitive_deployment" "generic_deployment" {
16:51:09        ~ id                     = "/subscriptions/xx/resourceGroups/ai-services-rg-01/providers/Microsoft.CognitiveServices/-cog-01/dex/gpt-35-txurbo-deployment-01" -> (known after apply)
16:51:09          name                   = "gpt-35-turbo-deployment-01"
16:51:09        ~ version_upgrade_option = "OnceCurrentVersionExpired" -> "OnceNewDefaultVersionAvailable" # forces replacement
16:51:09          # (2 unchanged attributes hidden)
16:51:09  
16:51:09        ~ scale {
16:51:09              # (2 unchanged attributes hidden)
16:51:09          }
16:51:09  
16:51:09          # (1 unchanged block hidden)
16:51:09      }
16:51:09  

I can raise a PR for a quick fix. Do let me know thoughts @katbyte

Copy link

github-actions bot commented May 7, 2024

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_cognitive_deployment not able to deploy latest version(0613) of gpt3.5
7 participants