Compatibility fixes with distributed 1.21.3 #63

azjps · 2018-03-15T02:10:00Z

Support passing kwargs to distributed.Adaptive.__init__, which now includes arguments like minimum and maximum [number of workers].
Add an optional workers argument to _retire_workers() to match Adaptive min max distributed#1797 (currently throws a TypeError)

jakirkham · 2018-03-15T03:21:55Z

Are you able to test this, @azjps? Can we enable any of the tests that are currently marked as skipped or xfailed?

- Support passing kwargs to distributed.Adaptive.__init__, which now takes keyword arguments like minimum and maximum [number of workers]. - Add an optional workers argument to _retire_workers() to match dask/distributed#1797 -- currently Adaptive raises a TypeError.

In dask/distributed#1594, the scheduler's internal maps of task objects were changed from using their keys to using TaskState objects. However, dask_drmaa.Adaptive was still querying for keys, causing new workers to never find the memory resource constraints for pending tasks and consequently tasks to never find workers with sufficient resources. This was causing the unit test test_adaptive_memory to wait indefinitely. Try to fix this to support both distributed pre- and post- 1.21.0, and un-skip test_adaptive_memory.

azjps · 2018-03-15T17:30:31Z

I don't use SGE @jakirkham but I was able to make some small modifications to try running the unit tests. test_adaptive_memory was still hanging and test_dont_request_on_many_short_tasks still seems to fail non-deterministically (around 15% of the time from what I've seen locally). I tracked down the issue for test_adaptive_memory to this refactor in distributed==1.21.0: dask/distributed#1594 which was causing worker startup to not see pending tasks' resource constraints, and thus tasks would not match to workers after they were started up, causing the test to hang. I have not tried to debug test_dont_request_on_many_short_tasks.

Then again, I'm not positive Travis CI is using distributed>=1.21.0, but it does seem like its pinned to the latest version? Maybe we need to version distributed a bit more closely in Travis, since this repository does seem relatively tied to the internals of distributed, which seems to be undergoing substantial changes recently.

azjps · 2018-03-15T18:00:40Z

Well drat, this commit does work fine for me locally (test_adaptive_memory passes in < 90 seconds), but Travis doesn't seem to like it as much. Won't be able to poke further then.

jakirkham · 2018-03-15T20:15:08Z

Travis CI just uses docker compose to spin up Docker images for testing on SGE. Should be able to do the same locally.

azjps · 2018-03-19T18:58:43Z

I ran into an error with docker-compose in the step where its running ./setup-master.sh (command not foundelections and user not found: sgeadmin). Haven't had a chance to look more deeply yet.

Edit: This was because I was running docker from a Windows machine, had to remove all of the carriage returns from all of the *.sh files. Re-trying setup.

(Was testing on python2, switching to python2/3-compatible)

azjps · 2018-03-21T04:57:40Z

Okay, so after fixing some python2-specific syntax, Travis seems happier. Even though the builds state they are using both python2 and python3, it would seem as if both builds are deferring to a shared docker image which is using python3. Based on my local testing this seems to work correctly for python2 also. So this should close #58.

Unrelatedly, I still am not able to get the docker-compose build to work locally on my Windows desktop. I'll file a new issue for this.

jakirkham · 2018-03-21T05:05:34Z

dask_drmaa/adaptive.py

@@ -6,6 +6,7 @@
 from distributed import Scheduler
 from distributed.utils import log_errors
 from distributed.deploy import adaptive
+from six import string_types


Please add six to requirements.txt.

FYI six is in the requirements.txt for distributed, but I suppose there is no harm and it is more clear in duplicating here.

Sorry I spend part of my days with package management issues. Have generally found explicit requirements makes things easier to manage.

jakirkham · 2018-03-21T05:17:55Z

dask_drmaa/adaptive.py

@@ -93,7 +105,17 @@ def get_scale_up_kwargs(self):
        return kwargs

    @gen.coroutine
-    def _retire_workers(self):
+    def _retire_workers(self, workers=None):


Why add this option?

NVM. Comes from PR ( dask/distributed#1797 ).

We might want to revisit whether we should be carrying this function at all or just using the parent class' functionality.

@jakirkham Currently dask_drmaa==0.1.2 raises a TypeError when run with distributed==1.21.3 on this line of code in Adaptive. I guess the unit tests don't cover this or the distributed version is not pinned high enough in Travis.

Yep, sorry, missed where this was coming from on the first pass.

Would we be able to reuse that method or do you see issues with that approach?

Yeah, agreed that we should try to move back to the base implementation. I am not really sure why the implementation diverged from the base class to begin with, although it definitely seemed intentional: c51a15a#diff-d2ee7bfcb2312cc404b8b4953eaa2576L47. I haven't had a chance to step through the dask/distributed internals to see if there's still any behavioral change here. Perhaps @nevermindewe can shed some light? Anyway for now, like you suggested, it may be safer to move back to the base class in a separate PR.

Added as issue ( #65 ).

jakirkham · 2018-03-21T05:24:18Z

dask_drmaa/adaptive.py

+                    # instead of string keys in its task collections:
+                    # https://github.com/dask/distributed/pull/1594
+                    key = task.key
+                    prefix = task.prefix


Any thoughts on bumping our distributed requirement?

I am not sure, I think eventually we will definitely want to so that we don't have to deal with backwards-compatibility, but as of right now distributed==1.21.0 has only been out for a month and a half so maybe the dust hasn't settled yet. cc @mrocklin if you have any quick recommendations.

I'm comfortable bumping up requirements. If people want to use old versions of distributed then they can use old versions of dask-drmaa as well. I think we should maintain a little bit of slack between the various dask-foo projects, but not too much.

jakirkham · 2018-03-21T05:31:46Z

dask_drmaa/tests/test_adaptive.py

@@ -8,8 +8,6 @@
 from distributed import Client
 from distributed.utils_test import loop, inc, slowinc

-
-@pytest.mark.skip(reason="currently times out for an unknown reason")
 def test_adaptive_memory(loop):


Does this seem to be working reliably now or is it still a little flaky?

As far as I've tried, it seems to work reliably now 😁

If it acts up again, we can always reopen issue ( #58 ). Guessing you fixed it though as the issue cropped up with Distributed 1.21. Thanks for working on it.

jakirkham · 2018-03-21T05:35:14Z

Thanks @azjps. This looks pretty good. Would just add six in requirements.txt.

Ideally would like us to simplify this code a bit more and reuse things from Adaptive and the new Cluster from upstream, but that can be saved for another PR.

Also a couple of miscellaneous comments, including Windows-specific comment for running docker-based tests.

jakirkham · 2018-03-21T06:36:42Z

README.rst

-as follows
+Engine cluster with a master and two slaves.  You can initialize this system
+as follows (for Windows, make sure to remove any carriage returns from all
+`*.sh` shell scripts and `docker-compose.yml`):


Maybe we should add these to the .gitattributes file to standardize these by default?

Yeah, good idea -- I'll look into and split off into a separate PR.

Update requirements.txt to require distributed >= 1.21.0, since there are some internal changes in the way tasks are stored. Also drop the corresponding backwards- compatibility fixes. Feel free to revert if distributed 1.20.x support if desired.

jakirkham · 2018-03-24T16:59:15Z

Thanks @azjps!

azjps added 2 commits March 15, 2018 13:24

azjps force-pushed the master branch from bc48690 to 10677a3 Compare March 15, 2018 17:25

basestring -> six.string_types

cc20f43

(Was testing on python2, switching to python2/3-compatible)

jakirkham reviewed Mar 21, 2018

View reviewed changes

Add six to requirements.txt

ed352bd

Also a couple of miscellaneous comments, including Windows-specific comment for running docker-based tests.

jakirkham reviewed Mar 21, 2018

View reviewed changes

azjps added 2 commits March 22, 2018 23:42

Undo windows comments (moving to a separate PR)

feda1f7

Drop support for distributed < 1.21.0

3d6f165

Update requirements.txt to require distributed >= 1.21.0, since there are some internal changes in the way tasks are stored. Also drop the corresponding backwards- compatibility fixes. Feel free to revert if distributed 1.20.x support if desired.

jakirkham merged commit ed71052 into dask:master Mar 24, 2018

This was referenced Mar 24, 2018

test_adaptive's test_adaptive_memory times out #58

Closed

Move back towards Distributed's _retire_workers method #65

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility fixes with distributed 1.21.3 #63

Compatibility fixes with distributed 1.21.3 #63

azjps commented Mar 15, 2018 •

edited

jakirkham commented Mar 15, 2018

azjps commented Mar 15, 2018 •

edited

azjps commented Mar 15, 2018

jakirkham commented Mar 15, 2018

azjps commented Mar 19, 2018 •

edited

azjps commented Mar 21, 2018

jakirkham Mar 21, 2018

azjps Mar 21, 2018 •

edited

jakirkham Mar 21, 2018

jakirkham Mar 21, 2018

jakirkham Mar 21, 2018

azjps Mar 21, 2018

jakirkham Mar 21, 2018

azjps Mar 21, 2018

jakirkham Mar 24, 2018

jakirkham Mar 21, 2018

azjps Mar 21, 2018

mrocklin Mar 21, 2018

jakirkham Mar 21, 2018

azjps Mar 21, 2018

jakirkham Mar 24, 2018

jakirkham commented Mar 21, 2018

jakirkham Mar 21, 2018

azjps Mar 22, 2018

jakirkham commented Mar 24, 2018

Compatibility fixes with distributed 1.21.3 #63

Compatibility fixes with distributed 1.21.3 #63

Conversation

azjps commented Mar 15, 2018 • edited

jakirkham commented Mar 15, 2018

azjps commented Mar 15, 2018 • edited

azjps commented Mar 15, 2018

jakirkham commented Mar 15, 2018

azjps commented Mar 19, 2018 • edited

azjps commented Mar 21, 2018

Choose a reason for hiding this comment

azjps Mar 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Mar 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Mar 24, 2018

azjps commented Mar 15, 2018 •

edited

azjps commented Mar 15, 2018 •

edited

azjps commented Mar 19, 2018 •

edited

azjps Mar 21, 2018 •

edited