-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
Avoid deprecated method to get Azure subscription ID #378
Conversation
This removes the use of a deprecated method from azure-cli-core to get the default subscription ID. It also removes the dependency on azure-cli-core. For the common case, where the user has the Azure CLI installed and configured, we can get the subscription ID by parsing its output. This PR also enables users to configure the subscription ID either in code or through the Dask config system. A few related documentation updates for good measure.
For reference, here's my test setup:
I build that image and run it docker run -it --rm -v (pwd):/dask-cloudprovider \
-e DASK_CLOUDPROVIDER__AZURE__LOCATION="..." \
-e DASK_CLOUDPROVIDER__AZURE__RESOURCE_GROUP="..." \
-e DASK_CLOUDPROVIDER__AZURE__SUBSCRIPTION_ID="..." \
-e DASK_CLOUDPROVIDER__AZURE__AZUREVM__VNET="..." \
-e DASK_CLOUDPROVIDER__AZURE__AZUREVM__SECURITY_GROUP="..." \
-e AZURE_TENANT_ID=$PC_ARM_TENANT_ID -e AZURE_CLIENT_ID=$PC_ARM_CLIENT_ID -e AZURE_CLIENT_SECRET=$PC_ARM_CLIENT_SECRET \
tomaugspurger/dask-cloudprovider-azure pytest -vs /dask-cloudprovider/dask_cloudprovider/azure/tests --create-external-resources That container image doesn't have Azure CLI setup / configured. I'll also test it locally, once I get a python environment that matches close enough. |
There were some failures. I'll look into those later.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great thanks @TomAugspurger.
The tests here are not in a good place so I'm not surprise you're seeing failures. The CI only runs a subset due to either needing credentials or mocking so heavily that the tests aren't really testing anything. This means the tests drift a little as it's not always possible to get everything running locally for every contribution.
There have also been a bunch of asyncio changes in distributed
that we likely haven't kept up with here.
"azure-mgmt-network>=16.0.0,<17", | ||
"azure-cli-core>=2.15.1,<2.21.0", | ||
"msrestazure", | ||
"azure-mgmt-compute>=18.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we keep the upper bound pins here, just move them on to a future major version? The azure packages don't seem afraid of large breaking changes and major version bumps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually know if these libraries use semver, so I don't know what "upper" would be here (the one change that actual did break things was the change to azure-cli-core in a "minor" release, if it is indeed following semver).
Either way, I'd probably prefer not to put an upper bound since if things aren't broken the user doesn't have an easy way to override the pin. If things do break, then the user can set an upper bound in their requirements while we (I) investigate (https://iscinumpy.dev/post/bound-version-constraints/ goes through this in detail).
LMK what you think: happy to add them if you feel strongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, I've definitely been bitten by breaking changes in the Azure tools, and the major version seems to bump a lot so I assumed it was because of that. But if it isn't let's go without the pins for now.
The The |
The RAPIDS docker image is also pretty old here so might need updating. I appreciate you exploring this but I'm definitely happy to merge as is. |
This removes the use of a deprecated method from azure-cli-core to get the default subscription ID. It also removes the dependency on azure-cli-core.
For the common case, where the user has the Azure CLI installed and configured, we can get the subscription ID by parsing its output.
This PR also enables users to configure the subscription ID either in code or through the Dask config system.
A few related documentation updates for good measure.
FYI, I'm running the azure tests now. I'll let you know when they pass.
Closes #376