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

AzureMLCluster #67

Merged
merged 259 commits into from Jun 25, 2020
Merged

AzureMLCluster #67

merged 259 commits into from Jun 25, 2020

Conversation

drabastomek
Copy link
Contributor

Add AzureMLCluster as one of the providers. The class derives from dask.distributed.deploy.Cluster and makes creating, scaling and working with Dask on Azure ML.

Tom Drabas and others added 30 commits January 29, 2020 15:51
Show "Complete" status for canceled runs.
Add comment to explain why we need complete()
Handling API change in asyncio between Python 3.6 and 3.7
Fix some bugs
Merging Fred's changes to master
Make env object required
@jacobtomlinson
Copy link
Member

I think we are nearly there with this. I'd really like to push on getting it merged next week. I'm pretty busy this week but should be able to do some final testing next week.

Given the size and age of this PR I think as long as we can successfully run through the instructions then any future changes can be made in extra PRs.

@drabastomek
Copy link
Contributor Author

drabastomek commented Jun 18, 2020 via email

@dask dask deleted a comment from quasiben Jun 24, 2020
Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

I've run through the instructions again and everything seemed ok. I made a couple of minor comments but nothing huge.

I think there is still work to be done here. A few things that spring to mind:

  • The Jupyter environment should have the Dask extension installed
  • The Jupyter environment should be pre-configured to find the Dask cluster when running client = Client(). We do it like this in the Helm Chart
  • We shouldn't be printing output as actions are happening by default. Perhaps this should be a kwarg to toggle on for those that find it useful.
  • I'd love to explore pulling all of the boilerplate code from the instructions into the AzureMLCluster class. We should create the Workspace, Compute and Environment objects inside AzureMLCluster for people, unless they want to pass their own via kwargs. I appreciate this will probably explode the number of kwargs AzureMLCluster needs to take, but this will be necessary for widespread adoption.

With the few changes I've suggested inline I think we should merge this now and I'll raise issues for the other points I've made above.

Setup
~~~~~

Next, create the ``Workspace`` object given your AzureML ``Workspace`` parameters. Check
Copy link
Member

Choose a reason for hiding this comment

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

Could we add some instructions here on how to find this info.

For example I grabbed the config.json file from the Azure Portal.

image

"""""""""""""""
If you do not have a virtual network yet there are two ways to create one.

1. Using `https://azure.portal.com <https://azure.portal.com>`_:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Using `https://azure.portal.com <https://azure.portal.com>`_:
1. Using `https://portal.azure.com <https://portal.azure.com>`_:

.. code-block:: python

amlcluster = AzureMLCluster(
workspace=ws,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
workspace=ws,
workspace=ws

Comment on lines 346 to 347
.. code-block:: python
env = ws.environments[env_name]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. code-block:: python
env = ws.environments[env_name]
.. code-block:: python
env = ws.environments[env_name]

, admin_ssh_key=admin_ssh_key_priv ### path, not contents of the key
)

Once the cluster has started, the Dask Cluster widget will print out two links:
Copy link
Member

Choose a reason for hiding this comment

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

We should also give instructions on how to make this cluster stop. If I restart my notebook kernel or del the cluster object what happens, is anything cleaned for me? Do I need to kill the run and delete the compute resource in AzureML myself?

@lostmygithubaccount
Copy link
Member

@jacobtomlinson the comments should be addressed now

@jacobtomlinson
Copy link
Member

Awesome work, thanks @lostmygithubaccount!

I think I'm going to hit merge and look to get a release out later today.

I just want to say thanks again for all the effort here. This has been a mammoth PR and has taken us quite a while to get it merged.

@jacobtomlinson jacobtomlinson merged commit 896da13 into dask:master Jun 25, 2020
lukaszo pushed a commit to lukaszo/dask-cloudprovider that referenced this pull request Oct 12, 2023
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.

None yet

7 participants