Repository: HECBioSim/Longbow

Issue:
PBS and NAMD SMP
On ARCHER, NAMD jobs with SMP have intermittent difficulties with the ncpus and/or mpiprocs directive to "-l". This should be investigated, further. It appears to stem from the corespernode needing to be set at 1 which also sets ncpus to 1.There are several possibilities here:

1. A return to defaulting to not issuing ncpus or mpiprocs with the "-l" command and allowing users to specify if they are necessary or not as whether these are needed or not varies machine to machine. 

2. Another consideration is to see if there is actually something that is missing with these NAMD jobs that would let Longbow correctly assign ncpus ie -d to aprun.

3. Another solution is to have corespernode and mpiprocs as seperate parameters such that mpiprocs defaults to corespernode unless mpiprocs states otherwise. This should provide a better way to calculate ncpus from corespernode and mpiprocs (most users won't need to worry about it, just those using NAMD or those that wish to undersubscribe nodes). 

Discussion:

Patch:
diff --git a/longbow/configuration.py b/longbow/configuration.py
index bd12c87..2426df1 100644
--- a/longbow/configuration.py
+++ b/longbow/configuration.py
@@ -93,29 +93,30 @@ JOBTEMPLATE = {
     "host": "",
     "localworkdir": "",
     "lsf-cluster": "",
-    "modules": "",
     "maxtime": "24:00",
     "memory": "",
+    "modules": "",
+    "mpiprocs": "",
     "polling-frequency": "300",
     "port": "22",
     "queue": "",
     "recoveryfile": "",
     "remoteworkdir": "",
-    "resource": "",
     "replicates": "1",
     "replicate-naming": "rep",
+    "resource": "",
     "scheduler": "",
     "scripts": "",
+    "sge-peflag": "mpi",
+    "sge-peoverride": "false",
     "slurm-gres": "",
     "staging-frequency": "300",
     "stdout": "",
     "stderr": "",
-    "sge-peflag": "mpi",
-    "sge-peoverride": "false",
     "subfile": "",
-    "user": "",
     "upload-exclude": "",
-    "upload-include": ""
+    "upload-include": "",
+    "user": ""
 }
 
 
@@ -505,6 +506,7 @@ def _processconfigsresource(parameters, jobdata, hostsections):
     # Initialise
     jobs = {}
 
+
     # Process resource/s for job/s.
     for job in jobdata:
 
diff --git a/longbow/entrypoints.py b/longbow/entrypoints.py
index 140d583..8fc1bf0 100644
--- a/longbow/entrypoints.py
+++ b/longbow/entrypoints.py
@@ -415,8 +415,7 @@ def recovery(jobs, recoveryfile):
 
         _, _, jobparams = configuration.loadconfigs(jobfile)
 
-        # Copy to jobs so when exceptions are raised the structure is
-        # available.
+        # Copy to jobs so when exceptions are raised the structure is available.
         for param in jobparams:
 
             jobs[param] = jobparams[param]
@@ -454,8 +453,7 @@ def update(jobs, updatefile):
 
         _, _, jobparams = configuration.loadconfigs(jobfile)
 
-        # Copy to jobs so when exceptions are raised the structure is
-        # available.
+        # Copy to jobs so when exceptions are raised the structure is available.
         for param in jobparams:
 
             jobs[param] = jobparams[param]
diff --git a/longbow/schedulers/pbs.py b/longbow/schedulers/pbs.py
index 4449614..daaffca 100644
--- a/longbow/schedulers/pbs.py
+++ b/longbow/schedulers/pbs.py
@@ -108,30 +108,23 @@ def prepare(job):
             jobfile.write("#PBS " + job["accountflag"] + " " +
                           job["account"] + "\n")
 
+    processes = job["cores"]
     cpn = job["corespernode"]
+    mpiprocs = job["mpiprocs"]
 
-    cores = job["cores"]
+    # If not undersubscribing then.
+    if mpiprocs == "" and processes < cpn:
 
-    # Load levelling override. In cases where # of cores is less than
-    # corespernode, user is likely to be undersubscribing.
-    if int(cores) < int(cpn):
+        mpiprocs = processes
 
-        cpn = cores
+    elif mpiprocs == "":
 
-    # Calculate the number of nodes.
-    nodes = float(cores) / float(cpn)
-
-    # Makes sure nodes is rounded up to the next highest integer.
-    nodes = str(int(math.ceil(nodes)))
+        mpiprocs = cpn
 
-    # Number of cpus per node (most machines will charge for all available cpus
-    # per node whether you are using them or not)
-    ncpus = cpn
-
-    # Number of mpi processes per node.
-    mpiprocs = cpn
+    # Calculate the number of nodes.
+    nodes = str(int(math.ceil(float(processes) / float(mpiprocs))))
 
-    tmp = "select=" + nodes + ":ncpus=" + ncpus + ":mpiprocs=" + mpiprocs
+    tmp = "select=" + nodes + ":ncpus=" + cpn + ":mpiprocs=" + mpiprocs
 
     # If user has specified memory append the flag (not all machines support
     # this).
@@ -203,7 +196,7 @@ def prepare(job):
     # CRAY's use aprun which has slightly different requirements to mpirun.
     if mpirun == "aprun":
 
-        mpirun = mpirun + " -n " + cores + " -N " + mpiprocs
+        mpirun = mpirun + " -n " + processes + " -N " + mpiprocs
 
     # Single jobs only need one run command.
     if int(job["replicates"]) == 1:
diff --git a/longbow/scheduling.py b/longbow/scheduling.py
index 4153bff..385af73 100644
--- a/longbow/scheduling.py
+++ b/longbow/scheduling.py
@@ -329,8 +329,7 @@ def prepare(jobs):
                 LOG.info("For job '%s' user has supplied their own job submit "
                          "script - skipping creation.", item)
 
-                job["upload-include"] = (job["upload-include"] + ", " +
-                                         job["subfile"])
+                job["upload-include"] = job["upload-include"] + ", " + job["subfile"]
 
         except AttributeError:
 


Test patch:
diff --git a/tests/standards/pbs_submitfiles/case11.txt b/tests/standards/pbs_submitfiles/case11.txt
new file mode 100644
index 0000000..d8c2843
--- /dev/null
+++ b/tests/standards/pbs_submitfiles/case11.txt
@@ -0,0 +1,13 @@
+#!/bin/bash --login
+#PBS -N testjob
+#PBS -q debug
+#PBS -l select=2:ncpus=24:mpiprocs=1
+#PBS -l walltime=24:00:00
+
+export PBS_O_WORKDIR=$(readlink -f $PBS_O_WORKDIR)
+cd $PBS_O_WORKDIR
+export OMP_NUM_THREADS=1
+
+module load namd
+
+aprun -n 2 -N 1 namd2 +ppn 23 +pemap 1-23 +commap 0 bench.in > bench.log
diff --git a/tests/standards/pbs_submitfiles/case3.txt b/tests/standards/pbs_submitfiles/case3.txt
index 686c7e8..fdac9e4 100644
--- a/tests/standards/pbs_submitfiles/case3.txt
+++ b/tests/standards/pbs_submitfiles/case3.txt
@@ -1,7 +1,7 @@
 #!/bin/bash --login
 #PBS -N testjob
 #PBS -q debug
-#PBS -l select=1:ncpus=16:mpiprocs=16
+#PBS -l select=1:ncpus=24:mpiprocs=16
 #PBS -l walltime=24:00:00
 
 export PBS_O_WORKDIR=$(readlink -f $PBS_O_WORKDIR)
diff --git a/tests/unit/configuration/test_processconfigsresource.py b/tests/unit/configuration/test_processconfigsresource.py
index db0f1fc..cda1424 100644
--- a/tests/unit/configuration/test_processconfigsresource.py
+++ b/tests/unit/configuration/test_processconfigsresource.py
@@ -119,6 +119,7 @@ def test_processconfigsresource1():
             "modules": "",
             "maxtime": "24:00",
             "memory": "",
+            "mpiprocs": "",
             "nochecks": False,
             "scripts": "",
             "slurm-gres": "",
@@ -232,6 +233,7 @@ def test_processconfigsresource2():
             "modules": "",
             "maxtime": "24:00",
             "memory": "",
+            "mpiprocs": "",
             "nochecks": False,
             "scripts": "",
             "slurm-gres": "",
@@ -345,6 +347,7 @@ def test_processconfigsresource3():
             "modules": "",
             "maxtime": "24:00",
             "memory": "",
+            "mpiprocs": "",
             "nochecks": False,
             "scripts": "",
             "slurm-gres": "",
@@ -457,6 +460,7 @@ def test_processconfigsresource4():
             "modules": "",
             "maxtime": "24:00",
             "memory": "",
+            "mpiprocs": "",
             "nochecks": False,
             "scripts": "",
             "slurm-gres": "",
diff --git a/tests/unit/schedulers_pbs/test_pbs_prepare.py b/tests/unit/schedulers_pbs/test_pbs_prepare.py
index b8b04f9..c3e390a 100644
--- a/tests/unit/schedulers_pbs/test_pbs_prepare.py
+++ b/tests/unit/schedulers_pbs/test_pbs_prepare.py
@@ -58,6 +58,7 @@ def test_prepare_case1():
         "maxtime": "24:00",
         "memory": "",
         "modules": "amber",
+        "mpiprocs": "",
         "queue": "debug",
         "replicates": "1",
         "stdout": "",
@@ -96,6 +97,7 @@ def test_prepare_case2():
         "maxtime": "24:00",
         "memory": "",
         "modules": "amber",
+        "mpiprocs": "",
         "queue": "debug",
         "replicates": "5",
         "stdout": "",
@@ -132,6 +134,7 @@ def test_prepare_case3():
         "maxtime": "24:00",
         "memory": "",
         "modules": "amber",
+        "mpiprocs": "",
         "queue": "debug",
         "replicates": "1",
         "stdout": "",
@@ -169,6 +172,7 @@ def test_prepare_case4():
         "maxtime": "24:00",
         "memory": "",
         "modules": "amber",
+        "mpiprocs": "",
         "queue": "debug",
         "replicates": "1",
         "stdout": "",
@@ -206,6 +210,7 @@ def test_prepare_case5():
         "maxtime": "24:00",
         "memory": "",
         "modules": "amber",
+        "mpiprocs": "",
         "queue": "debug",
         "replicates": "1",
         "stdout": "",
@@ -243,6 +248,7 @@ def test_prepare_case6():
         "maxtime": "24:00",
         "memory": "",
         "modules": "amber",
+        "mpiprocs": "",
         "queue": "debug",
         "replicates": "1",
         "stdout": "",
@@ -280,6 +286,7 @@ def test_prepare_case7():
         "maxtime": "24:00",
         "memory": "",
         "modules": "amber",
+        "mpiprocs": "",
         "queue": "debug",
         "replicates": "1",
         "stdout": "",
@@ -317,6 +324,7 @@ def test_prepare_case8():
         "maxtime": "24:00",
         "memory": "",
         "modules": "amber",
+        "mpiprocs": "",
         "queue": "debug",
         "replicates": "1",
         "stdout": "",
@@ -354,6 +362,7 @@ def test_prepare_case9():
         "maxtime": "24:00",
         "memory": "8",
         "modules": "amber",
+        "mpiprocs": "",
         "queue": "debug",
         "replicates": "1",
         "stdout": "",
@@ -390,6 +399,7 @@ def test_prepare_case10():
         "maxtime": "24:00",
         "memory": "",
         "modules": "amber",
+        "mpiprocs": "",
         "queue": "debug",
         "replicates": "1",
         "stdout": "test.log",
@@ -406,3 +416,41 @@ def test_prepare_case10():
         os.path.join(
             os.getcwd(),
             "tests/standards/pbs_submitfiles/case10.txt"), "rb").read()
+
+
+def test_prepare_case11():
+
+    """
+    Test SMP type
+    """
+
+    job = {
+        "account": "",
+        "cluster": "cluster1",
+        "cores": "2",
+        "corespernode": "24",
+        "executableargs": ("namd2 +ppn 23 +pemap 1-23 +commap 0 " +
+                           "bench.in > bench.log"),
+        "handler": "aprun",
+        "email-address": "",
+        "email-flags": "",
+        "jobname": "testjob",
+        "localworkdir": "/tmp",
+        "maxtime": "24:00",
+        "memory": "",
+        "modules": "namd",
+        "mpiprocs": "1",
+        "queue": "debug",
+        "replicates": "1",
+        "stdout": "",
+        "stderr": "",
+        "scripts": "",
+        "upload-include": "file1, file2"
+    }
+
+    prepare(job)
+
+    assert open("/tmp/submit.pbs", "rb").read() == open(
+        os.path.join(
+            os.getcwd(),
+            "tests/standards/pbs_submitfiles/case11.txt"), "rb").read()