Skip to content
This repository has been archived by the owner on Nov 3, 2020. It is now read-only.

Added initial support for spot instances #66

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

stumitchell
Copy link
Contributor

Hi I really don't know how the test harness is setup but this code does work in practice,

If I was to improve it I would make sure the try except handling for ec2 errors is around the right code,

and I would issue a warning if there are 0 non-spot instances (actually I haven't tested that so that probably fails too)

Stu

@quasiben
Copy link
Member

Thanks for the PR @stumitchell ! I suspect things aren't passing CI because Travis times out. Looking briefly over the code it looks like things probably halt in wait_for_fulfillment(..) . I suspect we can mock some of those things out with moto: https://github.com/spulec/moto/blob/master/tests/test_ec2/test_spot_instances.py

I think we also want defaults to not use spot pricing: spot_count=0.

Because this doesn't require running machine the unittest suite can be fully executed with the following:

py.test dask_ec2/tests -s -vv

or a particular file:

py.test dask_ec2/tests/test_cluster.py -s -vv

or a particular function in a file:

py.test dask_ec2/tests/test_cluster.py::test_from_boto3 -s -vv

@stumitchell
Copy link
Contributor Author

Looks like master is failing with the same errors I am getting on this pr, so I'll wait till it is fixed

@quasiben
Copy link
Member

quasiben commented Feb 3, 2017

@stumitchell thanks for looking at CI. Resolving shortly

@quasiben
Copy link
Member

quasiben commented Feb 3, 2017

@stumitchell finished fixing CI. Rebase against master and push to trigger CI again

@coveralls
Copy link

coveralls commented Feb 5, 2017

Coverage Status

Coverage decreased (-1.05%) to 73.004% when pulling ae76aa1 on stumitchell:spot-instances into 9c60d92 on dask:master.

@coveralls
Copy link

coveralls commented Feb 5, 2017

Coverage Status

Coverage decreased (-1.05%) to 73.004% when pulling ae76aa1 on stumitchell:spot-instances into 9c60d92 on dask:master.

@stumitchell
Copy link
Contributor Author

Right this now passes but the new code is not covered by tests.
It should be fairly simple to add tests to this, but I don't really have the time to do it.

Also I think the code would benefit if one of the team looked at it to add coverage, and perhaps check some of the corner cases i.e. --count=0 (if you don't have any non spots) and the placement of try except blocks.

@swamidass
Copy link

Any progress on this? It would be nice if this worked.

@mrocklin
Copy link
Member

mrocklin commented Dec 28, 2017 via email

@mrocklin
Copy link
Member

Ah, my apologies, I see now that this is a PR. It looks like this PR needs help with testing and coverage as mentioned by @stumitchell above. If you have time to help with this @swamidass that would be great

@mrocklin
Copy link
Member

In the mean time, I've generally been recommending Kubernetes to people: http://dask.pydata.org/en/latest/setup/kubernetes.html

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants