-
Notifications
You must be signed in to change notification settings - Fork 199
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
Resurrecting Azure Execution provider #1121
Conversation
Maximum number of blocks to maintain. Default is 10. | ||
region : str | ||
Azure region to launch machines. Default is 'westus'. | ||
key_name : str |
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.
does this get used anywhere?
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.
Azure requires an "admin password" to be set for Linux VM instances (which I really don't like) but optionally allows for SSH keys to be used as well. I am open to opinions on whether it would be useful to have keys implemented, knowing that passwords are required anyways.
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'm a big fan of SSH keys, so I'm not going to say no.
I've made a bunch of comments - I think they're all usability related rather than anything to do with functionality. I'm not set up for azure so I can't run this code so I won't click approve. |
I have asked questions on a couple of them - will fix the rest later today. Thanks for the review ! |
…into benhg-azure-provider
I have fiddled with azure enough to have this start up a VM for me, using the below configuration. However it doesn't seem to successfully start up the htex workers - from Killing parsl with ctrl-C doesn't kill the VMs, which is worrying as that leaves a drain on real money behind at the end of the run.
|
Using the below config I am able to run some simple test
|
|
||
Required structure: | ||
{ | ||
'publisher': VM OS publisher |
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 found it quite awkward to find a userful publisher/offer/sku/version combo - it involved a lot of messing round in the Azure Marketplace. I don't know if there is a better way. But make some note about where to get these values from.
Killing parsl with ctrl-C should send cancel() requests and thus should close all of the VMs. In my testing, this worked as it should. Perhaps your VMs running out of memory caused this to not occur. Did the VMs go away with the updated, working config? With regards to the rest of these changes, I will make them this evening. |
I've done some more testing on the issue of not deleting on ctrl-C. If a submit() call is not still running, the VMs will indeed delete themselves. Unfortunately, Azure's API for running scripts on the VM (required for connecting back to Parsl controller) is both synchronous and very slow (30 sec min), so if you ctrl-C within 30 sec or so of a submit() request, Parsl doesn't know that an instance has been started, and so can't clean it up on shutdown. Any ideas on how to deal with this? |
There's enough information stored at this point, before the script is run, to be able to kill the VM, even though parsl doesn't know about it. submit, line 255-256
Once you have that info, maybe you could put a The idea being that until submit returns, it is the responsibility of that |
This makes sense to me. I will make those changes later today. Done |
ok this is looking good. I will give it another try out probably tomorrow and hopefully will merge it then |
This pull request re-adds the Azure execution provider.