azurerm_machine_learning_inference_cluster support identity, azurerm_machine_learning_compute_cluster support ssh, ssh_public_access_enabled enhancement, identity #12833

ms-henglu · 2021-08-04T03:16:46Z

website/docs/r/machine_learning_compute_cluster.html.markdown

ms-henglu · 2021-08-30T01:51:51Z

@katbyte , Hi, I've renamed these fields, please take another look, thanks!

katbyte

denniseik

Thanks for all changes and getting this out! We requires some updates in terms of property naming to be aligned with our upcoming V2 API's and for clarity.

denniseik · 2021-09-07T22:29:17Z

internal/services/machinelearning/machine_learning_compute_cluster_resource.go

 				Type:     pluginsdk.TypeList,
 				Required: true,
 				ForceNew: true,
 				MaxItems: 1,
 				Elem: &pluginsdk.Resource{
 					Schema: map[string]*pluginsdk.Schema{
-						"type": {
-							Type:     pluginsdk.TypeString,
+						"max_node_count": {


Requires renaming to 'max_instances' to align with upcoming V2 API for Azure ML: https://github.com/Azure/azureml_run_specification/blob/master/schemas/AmlCompute.json.

This property is not introduced in this PR, so we can't change it.

Could you please elaborate why we are unable to change the name?

We can change the name, but in this PR.

Because this property is already released, so if it's really necessary to change the name, we must add a new property max_instances and deprecate the old one.

IMO, we can handle v1-to-v2 upgrade inside the tf resource, we don't have to expose them to end users, so they can have a smooth upgrade.

denniseik · 2021-09-07T22:29:36Z

internal/services/machinelearning/machine_learning_compute_cluster_resource.go

-						"principal_id": {
-							Type:     pluginsdk.TypeString,
-							Computed: true,
+						"min_node_count": {


Requires renaming to 'min_instances' to align with upcoming V2 API for Azure ML: https://github.com/Azure/azureml_run_specification/blob/master/schemas/AmlCompute.json.

This property is not introduced in this PR, so we can't change it.

denniseik · 2021-09-07T22:30:00Z

internal/services/machinelearning/machine_learning_compute_cluster_resource.go

 						},
-						"tenant_id": {
+						"scale_down_nodes_after_idle_duration": {


Requires renaming to 'idle_time_before_scale_down' to align with upcoming V2 API for Azure ML: https://github.com/Azure/azureml_run_specification/blob/master/schemas/AmlCompute.json.

This property is not introduced in this PR, so we can't change it.

Could you please elaborate why we are unable to change the name?

It would be a breaking change. Once a property has been released to users we do not change it until a major version change - otherwise we would break users terraform configurations during a .x release which we do not do.

additionally, like the portal, we do not always match the API naming as we want tp be clear on what properties do as well as provide the best ux for terraform users as many users do not use IDEs that pull in property descriptions. ie something like like cluster.ssh_setting.ssh_value has redundant ssh in it as its already has ssh in the path nor is it as clear as clear as cluster.ssh.key_value.

Makes sense, thanks for clarifying @katbyte

internal/services/machinelearning/machine_learning_compute_cluster_resource.go

denniseik · 2021-09-07T23:18:14Z

website/docs/r/machine_learning_inference_cluster.html.markdown

@@ -130,10 +130,19 @@ The following arguments are supported:

 * `description` - (Optional) The description of the Machine Learning compute. Changing this forces a new Machine Learning Inference Cluster to be created.


Whenever we speak about "Machine Learning Inference Cluster", could we say "Azure Kubernetes Service cluster" instead? Azure Machine Learning merely helps create an Azure Kubernetes Service cluster programmatically as described on this page: https://docs.microsoft.com/en-us/azure/machine-learning/how-to-create-attach-kubernetes?tabs=python. The name Machine Learning Inference Cluster may create the impression that Inference Cluster is a managed offering although it is not.

I see. But "Azure Kubernetes Service cluster" sounds like https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/kubernetes_cluster

We actually are deploying https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/kubernetes_cluster it, and then register it to the machine learning workspace. Customers can also register an existing AKS cluster. To our end users, its the same thing.

This is the description assigned to this resource? and this resource is a Machine Learning Inference Cluster regardless of what it is behind the scenes.

@katbyte We never expected Machine Learning Inference Cluster by name as its concept is not existing in our service.

internal/services/machinelearning/machine_learning_compute_cluster_resource.go

katbyte · 2021-09-09T17:59:38Z

website/docs/r/machine_learning_inference_cluster.html.markdown

@@ -130,10 +130,19 @@ The following arguments are supported:

 * `description` - (Optional) The description of the Machine Learning compute. Changing this forces a new Machine Learning Inference Cluster to be created.


This is the description assigned to this resource? and this resource is a Machine Learning Inference Cluster regardless of what it is behind the scenes.

katbyte · 2021-09-14T00:18:39Z

looks like we have some test failures:

ms-henglu · 2021-09-14T05:15:03Z

@katbyte , I've fixed the tests. please check.

katbyte

THanks @ms-henglu - LGTM 🚀

github-actions · 2021-09-17T02:16:37Z

This functionality has been released in v2.77.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2021-10-18T02:11:30Z

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 bot added documentation size/XL labels Aug 4, 2021

ms-henglu force-pushed the ticket-1859724-enhancement-for-ml-compute-part branch from 0e3e748 to a600ebe Compare August 4, 2021 07:10

ms-henglu force-pushed the ticket-1859724-enhancement-for-ml-compute-part branch from a600ebe to e736766 Compare August 12, 2021 08:14

katbyte added a commit that referenced this pull request Aug 17, 2021

CHANGELOG.md for #12833

2fc500f

enhancement for compute_cluster and inference_cluster

df102ad

ms-henglu force-pushed the ticket-1859724-enhancement-for-ml-compute-part branch from e736766 to df102ad Compare August 17, 2021 08:31

katbyte requested changes Aug 28, 2021

View reviewed changes

website/docs/r/machine_learning_compute_cluster.html.markdown Outdated Show resolved Hide resolved

website/docs/r/machine_learning_compute_cluster.html.markdown Outdated Show resolved Hide resolved

rename fields

b1bee69

katbyte approved these changes Aug 30, 2021

View reviewed changes

denniseik suggested changes Sep 7, 2021

View reviewed changes

ms-henglu added 2 commits September 8, 2021 10:49

rename and remove some props

fd78a89

update field name and remove isolatedNetwork

5ad1efc

katbyte requested changes Sep 9, 2021

View reviewed changes

rename ssh

c96dfb8

katbyte added enhancement service/machine-learning labels Sep 13, 2021

katbyte added this to the v2.77.0 milestone Sep 13, 2021

remove "private_ip_only_enabled" property

c54c510

katbyte approved these changes Sep 16, 2021

View reviewed changes

katbyte changed the title ~~enhancement for compute_cluster and inference_cluster~~ azurerm_machine_learning_inference_cluster support identity, azurerm_machine_learning_compute_cluster support ssh, ssh_public_access_enabled enhancement, identity Sep 16, 2021

katbyte merged commit 447d7ea into hashicorp:main Sep 16, 2021

katbyte added a commit that referenced this pull request Sep 16, 2021

CHANGELOG.md for #12833

d65bbc6

github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_machine_learning_inference_cluster support identity, azurerm_machine_learning_compute_cluster support ssh, ssh_public_access_enabled enhancement, identity #12833

azurerm_machine_learning_inference_cluster support identity, azurerm_machine_learning_compute_cluster support ssh, ssh_public_access_enabled enhancement, identity #12833

ms-henglu commented Aug 4, 2021 •

edited

Loading

ms-henglu commented Aug 30, 2021

katbyte left a comment

denniseik left a comment

denniseik Sep 7, 2021

ms-henglu Sep 8, 2021

denniseik Sep 8, 2021

ms-henglu Sep 9, 2021

denniseik Sep 7, 2021

ms-henglu Sep 8, 2021

denniseik Sep 7, 2021

ms-henglu Sep 8, 2021

denniseik Sep 8, 2021

katbyte Sep 9, 2021 •

edited

Loading

denniseik Sep 10, 2021

denniseik Sep 7, 2021

ms-henglu Sep 8, 2021

denniseik Sep 8, 2021

katbyte Sep 9, 2021

denniseik Sep 9, 2021

katbyte Sep 9, 2021

katbyte commented Sep 14, 2021

ms-henglu commented Sep 14, 2021

katbyte left a comment

github-actions bot commented Sep 17, 2021

github-actions bot commented Oct 18, 2021

		@@ -130,10 +130,19 @@ The following arguments are supported:

		* `description` - (Optional) The description of the Machine Learning compute. Changing this forces a new Machine Learning Inference Cluster to be created.

azurerm_machine_learning_inference_cluster support identity, azurerm_machine_learning_compute_cluster support ssh, ssh_public_access_enabled enhancement, identity #12833

azurerm_machine_learning_inference_cluster support identity, azurerm_machine_learning_compute_cluster support ssh, ssh_public_access_enabled enhancement, identity #12833

Conversation

ms-henglu commented Aug 4, 2021 • edited Loading

ms-henglu commented Aug 30, 2021

katbyte left a comment

Choose a reason for hiding this comment

denniseik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Sep 14, 2021

ms-henglu commented Sep 14, 2021

katbyte left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 17, 2021

github-actions bot commented Oct 18, 2021

ms-henglu commented Aug 4, 2021 •

edited

Loading

katbyte Sep 9, 2021 •

edited

Loading