Template job scripts with Jinja by wtbarnes · Pull Request #370 · dask/dask-jobqueue

wtbarnes · 2019-11-24T06:56:49Z

This PR addresses #133 and is a first pass at using Jinja2 to template job submission scripts. See also #338. In general, the approach I've used here is to store the templates as separate files in templates/ such that there is a separate template for the worker command, the top level job script, and then a separate file for the job header for each scheduler. Rather than appending strings to a list, all of the formatting logic is now contained in the templates using the Jinja syntax.

Thus far, I've modified the top level job script and the worker command to use Jinja2. The following job scheduler headers have been retemplated with Jinja:

Closes #133, closes #321.

guillaumeeb · 2019-11-27T15:11:13Z

I don't have the time to check this right now, but I just wanted to say thanks for working on this!!

jacobtomlinson

This is really neat!

wtbarnes · 2019-11-27T16:57:56Z

@guillaumeeb no problem at all. I understand that PRs of this size require a lot of effort on the part of maintainers!

It looks like the failing SGE job is due to problem with installing some packages from apt. Could someone restart that job to see if that fixes things? I don't think the failure is related to anything in this PR.

lesteve · 2019-11-28T15:47:50Z

Thanks a lot, as for @guillaumeeb I may not have a lot of time to look at this in detail before some time.

What I would like in an ideal world:

fix a few things and release a 0.7.1 version, hopefully in a matter of weeks.
review this PR, merge it and maybe release a 0.8, hopefully in January

lesteve · 2019-11-28T15:50:42Z

Hmmm I have restarted the build and the SGE error is similar. Probably something to look into ...

lesteve · 2019-12-01T15:21:17Z

OK restarting the SGE job a few more times fixed the issue ...

Note this intermittent issue has been seen on master from time to time, e.g. https://travis-ci.org/dask/dask-jobqueue/jobs/617519102.

wtbarnes · 2019-12-01T15:38:57Z

Thanks @lesteve! I know this is a big PR and will take some time to review.

I currently have access to a PBS cluster and will test this out "in the wild." It'd be great if anyone with access to systems running Condor, LSF, or OAR could give this a go as well. I'm sure there are some hangups related to job script formatting that need to be worked out.

wtbarnes · 2020-02-20T04:31:07Z

Just wanted to check back in and see if anyone has had time to test this out or look it over 🙂 Happy to incorporate any feedback!

If accepted, is the plan still to include this in the 0.8 release and is there a rough timeline for that release?

guillaumeeb · 2020-02-21T13:54:39Z

Hey @wtbarnes, we're not forgetting you and are again really happy you took a stab at this.

I guess the first planning describe by @lesteve above got a bit delayed, we're probably too busy currently to make the small progress we wanted to achieve. It is thus difficult to give you a timeline as we're already late... We definitely plan to take a closer look at this one for the 0.8 release.

In the mean time and if you feel like it, you could take a look at the few issues identified by @lesteve for a 0.7.1 release and try to fix them? I think there are easy enough to fix (but may be wrong).

guillaumeeb

A first review with a couple of questions:

Shouldn't we add jinja2 in the requirement file?
If a user wants to customize the jinja templates for any reason (which is the final goal here, if a cluster job queuing system has some specific quirk, we want a elegant solution to bypass it), how is that done? Maybe this is already feasible by dropping a new template somewhere, or we need to add a way to override the default templates. In any case this should be documented somewhere in the docs. Can you work on this?

wtbarnes · 2020-02-21T15:50:15Z

I guess the first planning describe by @lesteve above got a bit delayed, we're probably too busy currently to make the small progress we wanted to achieve. It is thus difficult to give you a timeline as we're already late... We definitely plan to take a closer look at this one for the 0.8 release.

Thanks for the quick response @guillaumeeb. I'm not really in any hurry here, just wanted to check in. I know that release timelines are always a bit optimistic 😄, especially when all the work is done by volunteers!

In the mean time and if you feel like it, you could take a look at the few issues identified by @lesteve for a 0.7.1 release and try to fix them? I think there are easy enough to fix (but may be wrong).

Sure, I can try to look these over. These would be just those issues/PRs milestoned for 0.7.1?

wtbarnes · 2020-02-21T16:10:43Z

Shouldn't we add jinja2 in the requirement file?

Yes! I meant to do this.

If a user wants to customize the jinja templates for any reason (which is the final goal here, if a cluster job queuing system has some specific quirk, we want a elegant solution to bypass it), how is that done? Maybe this is already feasible by dropping a new template somewhere, or we need to add a way to override the default templates. In any case this should be documented somewhere in the docs. Can you work on this?

Jinja allows you to specify multiple possible template loaders. In my current implementation, the templates are only being loading from the package,

https://github.com/wtbarnes/dask-jobqueue/blob/934e403bb81b929458c240e7f1a0aa3a02108fda/dask_jobqueue/core.py#L244

This can be easily modified to accept custom templates that override the default choices,

loader = ChoiceLoader([
    DictLoader(custom_templates),
    PackageLoader("dask-jobqueue", "templates")
])
env = Environment(loader=loader)

where custom_templates is a dictionary containing any templates the user wants to override. If the dictionary is empty, each template will just fall back to those specified in the package.

The bigger question is what is the best entry point for the user to specify these templates? The Jinja environment lives on the Job base class so we would need to work out how best to propagate this information from cluster instantiation to the instantiation of each job.

My personal opinion is that this could probably be its own PR as it is complicated enough and really tackles a separate (though certainly related) issue. However, if people would rather see these changes all merged in at once, I'm happy to give that a go as well.

guillaumeeb · 2020-02-21T19:32:45Z

My personal opinion is that this could probably be its own PR as it is complicated enough and really tackles a separate (though certainly related) issue. However, if people would rather see these changes all merged in at once, I'm happy to give that a go as well.

Okay, that sounds fair!

lesteve · 2020-03-04T18:00:14Z

@wtbarnes sorry I haven't had time to look at this one indeed ...

This is the kind of PR that is quite hard to review because the diff is quite sizeable. One thing that would make me a bit a lot more relax about this PR would be if you could generate some reference submission scripts (with cluster.job_script()) from master exploring a bit different parameters and compare the submission scripts you get in this PR.

If you manage to get something going along these lines, feel free to post a snippet here!

lesteve · 2020-03-04T18:01:25Z

Also just for completeness, my current dask-jobqueue priority is to try to make some progress towards 0.7.1 in the next few weeks!

guillaumeeb · 2020-05-22T08:43:04Z

I think we should take another look at this one. There's been some little change on the master branch code. Would you be willing to update your PR @wtbarnes?

wtbarnes · 2020-05-22T18:26:54Z

Sorry, I've not had the time to come back to this in a while!

I see there are quite a few conflicts now with several of the different scheduler files. I'm happy to rebase this against the master branch and try to fix those if that is the preferred workflow,

jacobtomlinson · 2020-05-27T12:52:38Z

@wtbarnes yes please! :)

wtbarnes · 2020-05-27T19:06:53Z

Ok I've rebased this against master and added Jinja2 to the requirements file, but now it seems all of the tests are failing so I've a bit more work to do to get this into good shape.

lesteve · 2020-06-01T06:30:17Z

@wtbarnes sorry I introduced a conflict by merging a PR. Here is my current thinking:

release a 0.7.2 with the few improvements that went in since 0.7.1. Maybe in a week or two 🤞
look at this PR more seriously for 0.8

Sorry this is taking such a long time, but I am afraid this PR is not trivial to review at all and I don't have that much time for the Dask-Jobqueue maintenance these days ...

Something that would make this PR easier to review as I mentioned a few months ago (already? 😓)

One thing that would make me a bit a lot more relax about this PR would be if you could generate some reference submission scripts (with cluster.job_script()) from master exploring a bit different parameters and compare the submission scripts you get in this PR.

guillaumeeb · 2021-01-23T22:54:58Z

@wtbarnes if you're stiull around, this would be the good time to finish this one!

wtbarnes · 2021-07-05T21:01:02Z

I've just done a rebase against main. The conflicts were not as bad as I suspected and I've tried to make sure all of my changes are still consistent with any changes that have happened on the particular job schedulers since I opened this PR.

I'm not sure what the merge policy is on this repo, but this should probably be squash-merged

EDIT: It looks like there are quite a few test failures here, likely because of things that have not been included in the new jinja templates. @sdtaylor I wonder if your proposed changes will take care of those?

Re: the seemingly extraneous JOB_ID, I'm not sure where that came from or why exactly it seems to show up in so many of the templates.

…nsion

jacobtomlinson

I hope you don't mind I've pushed in a quick fix for the failing LFS test (template was missing quotes).

I also renamed the templates to have a .j2 file extension which helps with syntax highlighting in many editors and GitHub.

sdtaylor · 2021-07-06T14:19:36Z

for the JOB_ID it looks like those were used when the jinja templates were first made, but have since been removed. So those lines in the template can be taken out. See #396

sdtaylor · 2021-07-06T14:58:55Z

I made some small adjustments here to drop JOB_ID and fix the interface double entry https://github.com/sdtaylor/dask-jobqueue/tree/jinja-templates. I don't have write permissions to this repo so if someone can merge them.

With those changes the current comparison looks pretty good. The only minor thing is various newlines where entries in the jinja template are skipped:

######################
Script mismatch for PBS1:
######################
--- 

+++ 

@@ -1,13 +1,13 @@

-***EXPECTED***
+***CURRENT***
 #!/usr/bin/env bash
 
 #PBS -N dask-worker
 #PBS -q myqueue
 #PBS -A myproject
 #PBS -l select=1:ncpus=1:mem=1908MB
 #PBS -l walltime=00:02
+
 #PBS -x extra-option1
 #PBS -z extraoption2
 
 /usr/bin/python -m distributed.cli.dask_worker tcp://127.0.0.1:54321 --nthreads 1 --memory-limit 1.86GiB --name dummy-name --nanny --death-timeout 60 --local-directory /foo --interface lo --protocol tcp://
-
######################
Script mismatch for MOAB1:
######################
--- 

+++ 

@@ -1,13 +1,13 @@

-***EXPECTED***
+***CURRENT***
 #!/usr/bin/env bash
 
 #PBS -N dask-worker
 #PBS -q myqueue
 #PBS -A myproject
 #PBS -l select=1:ncpus=1:mem=1908MB
 #PBS -l walltime=00:02
+
 #PBS -x extra-option1
 #PBS -z extraoption2
 
 /usr/bin/python -m distributed.cli.dask_worker tcp://127.0.0.1:54321 --nthreads 1 --memory-limit 1.86GiB --name dummy-name --nanny --death-timeout 60 --local-directory /foo --interface lo --protocol tcp://
-
######################
Script mismatch for SGE1:
######################
--- 

+++ 

@@ -1,14 +1,15 @@

-***EXPECTED***
+***CURRENT***
 #!/usr/bin/env bash
 
 #$ -N dask-worker
 #$ -q myqueue
 #$ -P myproject
+
 #$ -l h_rt=00:02
+
 #$ -cwd
 #$ -j y
 #$ -x extra-option1
 #$ -z extraoption2
 
 /usr/bin/python -m distributed.cli.dask_worker tcp://127.0.0.1:54321 --nthreads 1 --memory-limit 1.86GiB --name dummy-name --nanny --death-timeout 60 --local-directory /foo --protocol tcp://
-
######################
Script mismatch for HTCondor:
######################
--- 

+++ 

@@ -1,18 +1,18 @@

-***EXPECTED***
+***CURRENT***
 #!/usr/bin/env bash
 
 MY.DaskWorkerName = "htcondor--$F(MY.JobId)--"
 RequestCpus = MY.DaskWorkerCores
 RequestMemory = floor(MY.DaskWorkerMemory / 1048576)
 RequestDisk = floor(MY.DaskWorkerDisk / 1024)
 MY.JobId = "$(ClusterId).$(ProcId)"
 MY.DaskWorkerCores = 1
 MY.DaskWorkerMemory = 2000000000
 MY.DaskWorkerDisk = 1000000000
 
+
 Environment = ""
 Arguments = "-c '/usr/bin/python -m distributed.cli.dask_worker tcp://127.0.0.1:54321 --nthreads 1 --memory-limit 1.86GiB --name dummy-name --nanny --death-timeout 60 --local-directory /foo --protocol tcp://'"
 Executable = /bin/sh
 
 Queue
-
######################
Script mismatch for OAR1:
######################
--- 

+++ 

@@ -1,12 +1,11 @@

-***EXPECTED***
+***CURRENT***
 #!/usr/bin/env bash
 
 #OAR -n dask-worker
 #OAR -q myqueue
 #OAR --project myproject
 #OAR -l /nodes=1/core=1,walltime=00:02
 #OAR -x extra-option1
 #OAR -z extraoption2
 
 /usr/bin/python -m distributed.cli.dask_worker tcp://127.0.0.1:54321 --nthreads 1 --memory-limit 1.86GiB --name dummy-name --nanny --death-timeout 60 --local-directory /foo --protocol tcp://
-
######################
Script mismatch for LSF1:
######################
--- 

+++ 

@@ -1,14 +1,15 @@

-***EXPECTED***
+***CURRENT***
 #!/usr/bin/env bash
 
 #BSUB -J dask-worker
+
 #BSUB -q myqueue
 #BSUB -P "myproject"
 #BSUB -n 1
+
 #BSUB -M 1
 #BSUB -W 00:02
 #BSUB -x extra-option1
 #BSUB -z extraoption2
 
 /usr/bin/python -m distributed.cli.dask_worker tcp://127.0.0.1:54321 --nthreads 1 --memory-limit 1.86GiB --name dummy-name --nanny --death-timeout 60 --local-directory /foo --protocol tcp://
-
######################
Script mismatch for SLURM1:
######################
--- 

+++ 

@@ -1,15 +1,15 @@

-***EXPECTED***
+***CURRENT***
 #!/usr/bin/env bash
 
 #SBATCH -J dask-worker
+
 #SBATCH -p myqueue
 #SBATCH -A myproject
 #SBATCH -n 1
 #SBATCH --cpus-per-task=1
 #SBATCH --mem=2G
 #SBATCH -t 00:02
 #SBATCH -x extra-option1
 #SBATCH -z extraoption2
 
 /usr/bin/python -m distributed.cli.dask_worker tcp://127.0.0.1:54321 --nthreads 1 --memory-limit 1.86GiB --name dummy-name --nanny --death-timeout 60 --local-directory /foo --protocol tcp://
-

wtbarnes · 2021-07-08T15:48:08Z

I've removed the JOB_ID from all of the templates. Are the newline differences important in any of these submission scripts? My experience is limited to SLURM and PBS and as far as I know neither of them care too much about extraneous newlines.

guillaumeeb · 2021-07-12T19:49:16Z

Thanks everyone for reviving this one!!

I've got two questions:

Is it in a state to review?
One of the goal of this is to make it easy to customize job_script to adapt to specific job scheduler setup. How can one change a jinja template for a given scheduler?

wtbarnes · 2021-07-13T01:18:09Z

Is it in a state to review?

I believe so though I would defer to @sdtaylor or @lesteve as well

One of the goal of this is to make it easy to customize job_script to adapt to specific job scheduler setup. How can one change a jinja template for a given scheduler?

I think this old comment is still relevant: #370 (comment). It is very easy to modify the Jinja Environment to accept custom templates (that can then override the default templates). However, I'm still not sure what the ideal entrypoint would be for these custom templates. My original thinking was that this PR would simply reproduce the current functionality and then a separate PR would add the ability to supply custom templates.

sdtaylor · 2021-07-13T19:08:22Z

Yes, I feel it's ready for review.

guillaumeeb

This looks good, but I have some comments on potentially simplifying things, or relying more on templates.

guillaumeeb · 2021-09-03T14:46:04Z

+        return env
+
+
+def env_lines_to_dict(env_lines):


Couldn't this be done directly in the template file?

guillaumeeb · 2021-09-03T14:47:44Z

            return result


+def set_ncpus(ncpus, worker_cores, logger):


Same for LSF, couldn't we code this in the template file?

It's possible though the templating logic can easily get messy and it is often easier and cleaner to offload more complicated logic to filters (which is exactly what is done here). Additionally, I don't think you can easily log things from within a Jinja template.

Yeah, I was thinking of removing the log, which don't seem really useful to me.

But actually, in this case, we could have kept the ncpus and mem settings in the __init__ method, don't we?

Definitely agree regarding ncpus and mem. They're so simple they should just be in the __init__.

The format_memory function should probably remain as a filter because of the complexity in lsf_format_bytes_ceil and lsf_detect_units

guillaumeeb · 2021-09-03T14:48:10Z

    return "%dB" % n


+def pbs_format_resource_spec(resource_spec, worker_cores, worker_memory):


Same for PBS, in the template file?

See above comment on the LSF templating/filtering

Just giving it a try here. I think in the template it would look something like:

{% if resource_spec is not none -%} #PBS -l {{ resource_spec }} {% else -%} #PBS -l select=1:ncpus={{ worker_cores }}:mem={{ worker_memory | pbs_format_bytes_ceil }} {% endif -%}

I like that because it's clear with a single look at the template file what is done. Thoughts?

Agreed! That's much more simple. And then pbs_format_bytes_ceil would just need to be added as a filter.

I wonder if we should filter on worker_cores that ensures that it is an integer?

#PBS -l select=1:ncpus={{ worker_cores | round | int }}:mem={{ worker_memory | pbs_format_bytes_ceil }}

wtbarnes · 2023-02-08T18:47:14Z

Unfortunately I don't have much time these days to invest in dask-jobqueue and am not using it in my work any longer so I'm going to close this PR. Furthermore, it was opened so long ago and has so many conflicts now that it isn't even clear to me if this is useful any longer.

If anyone would like to revive this, please feel free to reopen.

lesteve · 2023-02-09T05:38:33Z

Thanks a lot for this work which definitely looked useful and sorry I never managed to find the time to review it fully 😢

wtbarnes force-pushed the jinja-templates branch from 5330e1b to fcb2d58 Compare November 27, 2019 08:11

wtbarnes changed the title ~~WIP: Template job scripts with Jinja~~ Template job scripts with Jinja Nov 27, 2019

jacobtomlinson reviewed Nov 27, 2019

View reviewed changes

lesteve added this to the 0.8 milestone Dec 1, 2019

lesteve mentioned this pull request Dec 1, 2019

use str.format notation #318

Closed

guillaumeeb reviewed Feb 21, 2020

View reviewed changes

guillaumeeb mentioned this pull request May 22, 2020

Make submission script more human readable #431

Closed

wtbarnes force-pushed the jinja-templates branch from 934e403 to 7a3cd9e Compare May 27, 2020 18:58

lesteve mentioned this pull request Oct 1, 2020

Modification to run on Summit and support login / batch / compute architecture #467

Closed

lesteve force-pushed the master branch 2 times, most recently from 4d181fe to 26a0e70 Compare December 8, 2020 12:40

wtbarnes and others added 12 commits July 5, 2021 16:47

use template to generate condor job script

9f4d995

remove header test

9e571ee

apply black

7fb6f75

remove trailing space

918d173

remove old script template

1edb3cd

render LSF job header with Jinja

caad975

render OAR job header with Jinja

272565a

render SGE job header with Jinja

88f1bad

apply black

5939574

remove old templating code

97d64ce

fix formatting

29aa9bb

add jinja2 to requirements

ca8b697

wtbarnes force-pushed the jinja-templates branch from 9a0fa4b to ca8b697 Compare July 5, 2021 20:53

formatting

2fe7e30

Add quotes to LSF template and rename templates with Jinja2 file exte…

32e1d3f

…nsion

jacobtomlinson reviewed Jul 6, 2021

View reviewed changes

remove JOB_ID from templates

def8d48

guillaumeeb reviewed Sep 3, 2021

View reviewed changes

guillaumeeb mentioned this pull request Mar 7, 2022

Configure Dask workers to contact scheduler on a specific address #548

Closed

guillaumeeb removed this from the 0.8 milestone Aug 30, 2022

wtbarnes closed this Feb 8, 2023

		return "%dB" % n


		def pbs_format_resource_spec(resource_spec, worker_cores, worker_memory):

Uh oh!

Uh oh!

Conversation

wtbarnes commented Nov 24, 2019 • edited by lesteve Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guillaumeeb commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobtomlinson left a comment

Choose a reason for hiding this comment

Uh oh!

wtbarnes commented Nov 27, 2019

Uh oh!

lesteve commented Nov 28, 2019

Uh oh!

lesteve commented Nov 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Dec 1, 2019

Uh oh!

wtbarnes commented Dec 1, 2019

Uh oh!

wtbarnes commented Feb 20, 2020

Uh oh!

guillaumeeb commented Feb 21, 2020

Uh oh!

guillaumeeb left a comment

Choose a reason for hiding this comment

Uh oh!

wtbarnes commented Feb 21, 2020

Uh oh!

wtbarnes commented Feb 21, 2020

Uh oh!

guillaumeeb commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Mar 4, 2020

Uh oh!

lesteve commented Mar 4, 2020

Uh oh!

guillaumeeb commented May 22, 2020

Uh oh!

wtbarnes commented May 22, 2020

Uh oh!

jacobtomlinson commented May 27, 2020

Uh oh!

wtbarnes commented May 27, 2020

Uh oh!

lesteve commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guillaumeeb commented Jan 23, 2021

Uh oh!

wtbarnes commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobtomlinson left a comment • 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!

sdtaylor commented Jul 6, 2021

Uh oh!

sdtaylor commented Jul 6, 2021

Uh oh!

wtbarnes commented Jul 8, 2021

Uh oh!

guillaumeeb commented Jul 12, 2021

Uh oh!

wtbarnes commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdtaylor commented Jul 13, 2021

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

wtbarnes commented Nov 24, 2019 •

edited by lesteve

Loading

guillaumeeb commented Nov 27, 2019 •

edited

Loading

lesteve commented Nov 28, 2019 •

edited

Loading

guillaumeeb commented Feb 21, 2020 •

edited

Loading

lesteve commented Jun 1, 2020 •

edited

Loading

wtbarnes commented Jul 5, 2021 •

edited

Loading

jacobtomlinson left a comment •

edited

Loading

wtbarnes commented Jul 13, 2021 •

edited

Loading

wtbarnes Sep 10, 2021 •

edited

Loading