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

Updates to azure autoscaler for authentication and dependency updates #19603

Merged
merged 8 commits into from
Dec 16, 2021

Conversation

gramhagen
Copy link
Contributor

Why are these changes needed?

  1. moves authentication for azure autoscaler to azure-identity package
  2. quiets some http logging
  3. provides better backwards compatibility for all azure sdk functions
  4. updates azure sdk dependencies
  5. moves vm images to use the latest version to handle frequent deprecation of vm offer versions

Related issue number

Closes #19523
Fixes #15274

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

…oving to azure-identity based authentication
@franklsf95
Copy link
Contributor

Thanks for the fix!

@gramhagen
Copy link
Contributor Author

@richardliaw what is needed to update the ci runs to include the azure sdk packages added here?

@richardliaw
Copy link
Contributor

@ijrsvt @DmitriGekhtman could you help shepherd this PR?

@ijrsvt
Copy link
Contributor

ijrsvt commented Oct 25, 2021

@gramhagen If you update https://github.com/ray-project/ray/blob/master/python/requirements.txt, then CI will run with the correct versions!

Copy link
Contributor

@ijrsvt ijrsvt left a comment

Choose a reason for hiding this comment

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

Overall looks good--just two comments :)

python/requirements.txt Outdated Show resolved Hide resolved
python/ray/autoscaler/_private/_azure/node_provider.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ijrsvt ijrsvt left a comment

Choose a reason for hiding this comment

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

One minor comment, but otherwise LGTM!

@franklsf95
Copy link
Contributor

Can this PR be merged now && will it be in the next Ray release? Thanks!

@ijrsvt
Copy link
Contributor

ijrsvt commented Nov 1, 2021

@gramhagen Can you rebase this on master? I'm unsure why CI is not wanting to build for this

@gramhagen
Copy link
Contributor Author

bumping this, what needs to happen to get this merged? @ijrsvt

@ijrsvt
Copy link
Contributor

ijrsvt commented Dec 2, 2021

@gramhagen Sorry! Can you rebase against master?

@franklsf95
Copy link
Contributor

Bump

@gramhagen
Copy link
Contributor Author

@ijrsvt Ian, I'm a bit lost on how to resolve these tests, any suggestions?

@ijrsvt
Copy link
Contributor

ijrsvt commented Dec 7, 2021

Hmm @gramhagen, I'm not 100% sure what the best way is to reproduce this, but it looks like some timeout with pip installs?
https://buildkite.com/ray-project/ray-builders-pr/builds/20494#fedc36da-6e58-4445-a381-6ebe19b3c5d8

@gramhagen
Copy link
Contributor Author

merging again, hopefully that was due to aws outage?

@ijrsvt
Copy link
Contributor

ijrsvt commented Dec 9, 2021

Hmm, I tried to make the hanging command print more output here: https://buildkite.com/organizations/ray-project/pipelines/ray-builders-pr/builds/20646/jobs/a6d7f7e9-d580-4fd4-bab7-3952a8666dfd/raw_log

It appears to be hanging on finding a version for pyasn1-modules. I'm trying to add some pinned dependencies in #20940

@gramhagen
Copy link
Contributor Author

i expect pyasn1-modules is just the straw that broke the camels back, once pip starts trying to resolve different dependency versions it's super slow, and it looked like there were a few instances of that happening. pinning the dependencies is probably the right way to go, but it will be a bit tedious to find them all. is this replicable in a docker env?

@ijrsvt
Copy link
Contributor

ijrsvt commented Dec 9, 2021

@gramhagen I'm not 100% sure of the exact docker env setup, but using these requirements.txt and pip==21.0.1.

Some possible resolutions:

  • Loosening the pinned requirements (i.e. using package~=version or package>=version)
  • Removing unnecessary pinned requirements (from the ones added)

@ijrsvt ijrsvt mentioned this pull request Dec 15, 2021
6 tasks
@ijrsvt
Copy link
Contributor

ijrsvt commented Dec 15, 2021

Alright @gramhagen Sorry for the confusion and delays here, but after various attempts at pinning dependencies (#20940 & #21105), it looks like just updating pip was the solution #21111

I can't seem to push to your branch, but I ran tests with merging master here. **If you can merge master again, this should fly through CI =) 🤞 **
#21121

Closes
#20940
#21105
#21121

@gramhagen
Copy link
Contributor Author

great news, thanks @ijrsvt !
trying agian🤞

@ijrsvt ijrsvt merged commit 7153d58 into ray-project:master Dec 16, 2021
@gramhagen gramhagen deleted the gramhagen/az_cli_fix branch March 14, 2022 14:30
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.

[Bug] Ray Autoscaler does not work with Azure due to deprecated method
5 participants