SGE implementation by lesteve · Pull Request #27 · dask/dask-jobqueue

lesteve · 2018-04-04T13:15:00Z

I revived #6 and added simple tests. I was able to get test_basic to pass but not test_adaptive yet.

I'll wait that #25 is merged before working on this further.

Fix #3. Closes #6.

jhamman

Thanks for resurrecting this one. Just a few questions on the tests for now.

jhamman · 2018-04-05T05:09:39Z

+
+
+@pytest.mark.skipif('SGE_ACCOUNT' in os.environ, reason='SGE_ACCOUNT defined')  # noqa: F811
+def test_errors(loop):


I don't think this test is relevant anymore.

Yeah I was wondering about that and I felt the same way.

I think that this test came from a cluster where project= was necessary to run, and was often set implicitly with an environment variable (perhaps PBS_ACCOUNT on the cluster on which I was working at the time). If this is not generally true for SGE clusters then I agree that this test should be removed.

jhamman · 2018-04-05T05:12:25Z

+            start = time()
+            while cluster.jobs:
+                sleep(0.100)
+                assert time() < start + 10


What is this testing exactly? @mrocklin - given the recent development of Adaptive, can you help us flesh out what kind of tests we should be looking at here?

I've simplified the test for now.

(I hope you don't mind my pushing to your fork @lesteve )

I've simplified the test for now.

Thanks!

(I hope you don't mind my pushing to your fork @lesteve )

Not at all, pushing into a PR's fork is a feature I use this feature quite often myself, it is very convenient and saves a lot of back and forth for minor things!

* use submit_command="qsub -terse" to make sure only the job id is returned.

Remove test_errors. PBS_ACCOUNT does not have an equivalent in SGE.

lesteve · 2018-04-11T15:03:45Z

OK I I think this is in a mergeable state, comments more than welcome! It is a first implementation of SGE (probably by no means perfect) and allows to test some of the common code in dask_jobqueue/core.py across schedulers.

A few comments:

I could not find an equivalent of PBS_ACCOUNT in SGE so I removed the associated code and test_errors test
AFAICT adaptive is currently broken, so I am leaving test_adaptive for later.

More details about the adaptive problems I saw (maybe related to #26 not sure):
According to

dask-jobqueue/dask_jobqueue/core.py

Lines 220 to 224 in bdb2c26

    
           def scale_down(self, workers): 
        
               if isinstance(workers, dict): 
        
                   names = {v['name'] for v in workers.values()} 
        
                   job_ids = {name.split('-')[-2] for name in names} 
        
                   self.stop_workers(job_ids)

, scale_down only does something if isinstance(workers, dict). The problem is that workers is a list of tcp://... worker addresses. So scale_down does nothing i.e. self.jobs stays the same so that scale_up doesn't see the need to schedule more jobs:

dask-jobqueue/dask_jobqueue/core.py

Lines 217 to 218 in bdb2c26

    
           def scale_up(self, n, **kwargs): 
        
               return self.start_workers(n - len(self.jobs))

In other words your *Cluster Python object think he still have enough workers but your scheduling system has no jobs in running state that can process the work.

I guess this is all fixable by looking at the doc, the distributed.LocalCluster implementation and possibly dask_drmaa as well. I may not find time to look at this for a few days so if anyone wants to pick this up, please be my guest!

guillaumeeb

Some minor fixes and one question, but this looks really fine to me. Thanks!

guillaumeeb · 2018-04-11T20:20:39Z

+                 project=None,
+                 resource_spec=None,
+                 walltime='0:30:00',
+                 interface=None,


I believe interface=None should not be here. It is a keyword from the parent class, and you don't seem to use it.

Good catch, done.

guillaumeeb · 2018-04-11T20:22:14Z

+    project : str
+        Accounting string associated with each worker job. Passed to
+        `#$ -A` option.
+    threads_per_worker : int


For all the parameters inherited from JobQueueCluster, you should use the same mechanism as in PBSCluster or SLURMCluster, with the docstrings module: %(JobQueueCluster.parameters)s

I believe the sge.py file still misses some import and annotation, e.g.

dask-jobqueue/dask_jobqueue/pbs.py

Line 5 in 2e406ae

from .core import JobQueueCluster, docstrings

and

dask-jobqueue/dask_jobqueue/pbs.py

Line 10 in 2e406ae

@docstrings.with_indent(4)

You also still have the description of interface kw in the docstring!

Ah right I fixed that, I am not so familiar with the docrep magic!

guillaumeeb · 2018-04-11T20:24:13Z

            with self.job_file() as fn:
-                out = self._call([self.submit_command, fn])
-                job = out.decode().split('.')[0]
+                out = self._call(shlex.split(self.submit_command) + [fn])


I don't know shlex, is this mandatory? It serves what purpose?

In SGE, by default qsub returns quite a verbose output, e.g. something like Your job 56 ("test.sh") has been submitted. In order for just the job id to be returned, you need to use qsub -terse. This is why submit_cmd = 'qsub -terse'.

That means that you need to split submit_cmd. I think shlex.split is the way to do it for sh commands. We could just do submit_cmd.split(' ') but it may break e.g. if one of the arguments is quoted with a space inside.

Ok that's perfect then!

guillaumeeb · 2018-04-11T20:27:24Z

+                       'resource_spec': resource_spec,}
+        self.job_header = self._header_template % self.config
+
+        logger.debug("Job script: \n %s" % self.job_script())


We should probably debug log only the header here, but this is also true for PBS or SLURM, and it is really a minor detail...

guillaumeeb · 2018-04-11T20:44:50Z

And about your other remarks:

I totally agree to remove the PBS_ACCOUNT part and according test,
For adaptive, you're completely right about the scale_down method, don't know though if it also breaks the scale_up. If you started with lets say only 1 worker, the scale_up should probably be called at some point, and not the scale_down, so something should happen. But yeah, we should look elsewhere!

* Remove interface parameters * Update docstring which was out of date

lesteve · 2018-04-12T08:49:34Z

@guillaumeeb thanks for the review! I tackled your comments, let me know if you have some further comments!

guillaumeeb · 2018-04-12T09:58:04Z

+        option.
+    walltime : str
+        Walltime for each worker job.
+    interface : str


You still have the interface keyword in this docstring, once remove I think we're done :)

Arrggh good point, I just pushed the fix.

lesteve · 2018-04-12T11:18:37Z

-                out = self._call([self.submit_command, fn])
-                job = out.decode().split('.')[0]
+                out = self._call(shlex.split(self.submit_command) + [fn])
+                job = out.decode().split('.')[0].strip()


And by the way the .strip() here is necessary because qsub -terse output finishes with a \n.

jhamman · 2018-04-12T21:00:57Z

Thanks @lesteve!

mrocklin · 2018-04-12T21:01:43Z

Thanks @lesteve!

…

On Thu, Apr 12, 2018 at 4:00 PM, Joe Hamman ***@***.***> wrote: Thanks @lesteve <https://github.com/lesteve>! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszDrftRGzVzTkfiIGwcwuuQh9mQKPks5tn8CKgaJpZM4TGxk-> .

lesteve · 2018-04-13T04:53:35Z

Nice to see this merged! I'll try to find some time to fix the adaptive problem.

jhamman reviewed Apr 5, 2018

View reviewed changes

First attempt at SGE implementation

4392a0b

* use submit_command="qsub -terse" to make sure only the job id is returned.

lesteve force-pushed the sge branch from a946e9f to 4392a0b Compare April 10, 2018 14:10

lesteve added 3 commits April 11, 2018 16:43

Leave test_adaptive for later.

c83c29d

Remove test_errors. PBS_ACCOUNT does not have an equivalent in SGE.

Need to specify loop in the Client

27032ec

Clean up in sge.py

a7b26c8

lesteve force-pushed the sge branch from 412564e to a7b26c8 Compare April 11, 2018 14:49

lesteve changed the title ~~[WIP] SGE implementation~~ SGE implementation Apr 11, 2018

lesteve added 3 commits April 11, 2018 17:07

Fix flake8

3013547

Forgotten another self.name fixed in master

cea22cc

Retriggering CIs

10f7c72

guillaumeeb reviewed Apr 11, 2018

View reviewed changes

guillaumeeb mentioned this pull request Apr 11, 2018

Release #31

Closed

Clean-up

271d78c

* Remove interface parameters * Update docstring which was out of date

lesteve added 3 commits April 12, 2018 10:52

Cosmetic changes to harmonize with pbs.py

f4cf1ea

docrep-related fixes

222f157

Make SGECluster importable from dask_jobqueue

e9d0cb8

guillaumeeb approved these changes Apr 12, 2018

View reviewed changes

lesteve added 2 commits April 12, 2018 13:15

Remove interface parameter from docstring

bcf492b

Group stdlib imports together

a4cd9d3

lesteve commented Apr 12, 2018

View reviewed changes

Remove unused 'dirname' variable

2706776

jhamman merged commit ef15363 into dask:master Apr 12, 2018

lesteve deleted the sge branch April 13, 2018 04:52

This was referenced Apr 20, 2018

CI for PBSPro using Docker #41

Closed

CI for SLURM using Docker #42

Closed

CI for Schedulers #2

Closed

lesteve mentioned this pull request May 2, 2018

Pbs docker ci #47

Merged



		@pytest.mark.skipif('SGE_ACCOUNT' in os.environ, reason='SGE_ACCOUNT defined') # noqa: F811
		def test_errors(loop):

Uh oh!

Uh oh!

Conversation

lesteve commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhamman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guillaumeeb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guillaumeeb commented Apr 11, 2018

Uh oh!

lesteve commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhamman commented Apr 12, 2018

Uh oh!

mrocklin commented Apr 12, 2018 via email

Uh oh!

lesteve commented Apr 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lesteve commented Apr 4, 2018 •

edited

Loading

lesteve commented Apr 11, 2018 •

edited

Loading

lesteve Apr 12, 2018 •

edited

Loading