Repository: ExaWorks/psij-python (language: python)

Issue:
Not able to get ResourceSpecV1 working with PBS
Just checking to see if I am just doing something wrong or not but I am not able to get a PBS select string any different than `1:ncpus=1` no matter what resource spec I give to the job. Is this a known issue?


Discussion / hints:
I am not entirely sure what you mean, but I'm guessing that the complaint is that you are only getting one process started on one node. Could it be because of `launcher='single'`? May I suggest either the `multiple` launcher or `mpirun`?
So this is my quick example I was testing with. I had thought that this example would select 2 nodes. Am I doing something incorrect? I was working off of the example in the user guide, still am very new to using this utility.

```
from psij import Job, JobExecutor, JobAttributes, JobSpec, ResourceSpec, ResourceSpecV1
from psij.executors.batch.batch_scheduler_executor import BatchSchedulerExecutorConfig
from pathlib import Path
from datetime import timedelta
import logging

logging.basicConfig(level=logging.DEBUG)

working_directory = "/home/user/psi/test_runs/test"


ex = JobExecutor.get_instance(
    name="pbs",
    config=BatchSchedulerExecutorConfig(
        launcher_log_file=Path(working_directory) / "job.log",
        work_directory=Path(working_directory),
        initial_queue_polling_delay=2,
        queue_polling_error_threshold=2,
        keep_files=True
    )
)

job_attributes = JobAttributes(
    duration=timedelta(minutes=60),
    project_name="test_proj",
)

job_resources = ResourceSpecV1(
    node_count=2,
    processes_per_node=4,
)

job_spec = JobSpec(
    name="test_job",
    executable="/bin/date",
    attributes=job_attributes,
    resources=job_resources,
    stdout_path=Path(working_directory) / "job.out",
    stderr_path=Path(working_directory) / "job.err",
    launcher="multiple"
)

job = Job(job_spec)

ex.submit(job)
```
Well, this is embarrassing. The PBS job template says: `# TODO: job.spec.resources`. I will work on fixing this and hopefully have a fix and release early next week.
Aha! No worries, I just was unsure if I was making a newbie mistake. I appreciate it!

Fix patch:
diff --git a/src/psij-descriptors/pbs_descriptor.py b/src/psij-descriptors/pbs_descriptor.py
new file mode 100644
index 00000000..d603f83c
--- /dev/null
+++ b/src/psij-descriptors/pbs_descriptor.py
@@ -0,0 +1,11 @@
+from distutils.version import StrictVersion
+
+from psij.descriptor import Descriptor
+
+
+__PSI_J_EXECUTORS__ = [Descriptor(name='pbs', nice_name='PBS Pro', aliases=['pbspro'],
+                                  version=StrictVersion('0.0.2'),
+                                  cls='psij.executors.batch.pbs.PBSJobExecutor'),
+                       Descriptor(name='pbs_classic', nice_name='PBS Classic', aliases=['torque'],
+                                  version=StrictVersion('0.0.2'),
+                                  cls='psij.executors.batch.pbs_classic.PBSClassicJobExecutor')]
diff --git a/src/psij-descriptors/pbspro_descriptor.py b/src/psij-descriptors/pbspro_descriptor.py
deleted file mode 100644
index 05f7a8df..00000000
--- a/src/psij-descriptors/pbspro_descriptor.py
+++ /dev/null
@@ -1,8 +0,0 @@
-from distutils.version import StrictVersion
-
-from psij.descriptor import Descriptor
-
-
-__PSI_J_EXECUTORS__ = [Descriptor(name='pbs', nice_name='PBS', aliases=['pbspro'],
-                                  version=StrictVersion('0.0.1'),
-                                  cls='psij.executors.batch.pbspro.PBSProJobExecutor')]
diff --git a/src/psij/executors/batch/pbs.py b/src/psij/executors/batch/pbs.py
new file mode 100644
index 00000000..55fec639
--- /dev/null
+++ b/src/psij/executors/batch/pbs.py
@@ -0,0 +1,27 @@
+from pathlib import Path
+from typing import Optional
+
+from psij.executors.batch.pbs_base import PBSExecutorConfig, GenericPBSJobExecutor
+from psij.executors.batch.script_generator import TemplatedScriptGenerator
+
+
+class PBSJobExecutor(GenericPBSJobExecutor):
+    """A :class:`~psij.JobExecutor` for PBS Pro and friends.
+
+    This executor uses resource specifications specific to PBS Pro
+    """
+
+    def __init__(self, url: Optional[str] = None, config: Optional[PBSExecutorConfig] = None):
+        """
+        Parameters
+        ----------
+        url
+            Not used, but required by the spec for automatic initialization.
+        config
+            An optional configuration for this executor.
+        """
+        if not config:
+            config = PBSExecutorConfig()
+        generator = TemplatedScriptGenerator(config, Path(__file__).parent / 'pbspro'
+                                             / 'pbspro.mustache')
+        super().__init__(generator, url=url, config=config)
diff --git a/src/psij/executors/batch/pbs/pbs_classic.mustache b/src/psij/executors/batch/pbs/pbs_classic.mustache
new file mode 100644
index 00000000..a531cfa6
--- /dev/null
+++ b/src/psij/executors/batch/pbs/pbs_classic.mustache
@@ -0,0 +1,71 @@
+#!/bin/bash
+
+
+{{#job.name}}
+#PBS -N="{{.}}"
+{{/job.name}}
+
+{{#job.spec.inherit_environment}}
+#PBS -V
+{{/job.spec.inherit_environment}}
+
+{{#job.spec.resources}}
+    {{#process_count}}
+#PBS -l nodes={{job.spec.resources.computed_node_count}}:ppn={{.}}{{#job.spec.resources.gpu_cores_per_process}}:gpus={{.}}{{/job.spec.resources.gpu_cores_per_process}}
+    {{/process_count}}
+    {{#exclusive_node_use}}
+#PBS -n
+    {{/exclusive_node_use}}
+{{/job.spec.resources}}
+
+{{#formatted_job_duration}}
+#PBS -l walltime={{.}}
+{{/formatted_job_duration}}
+
+{{#job.spec.attributes}}
+    {{#project_name}}
+#PBS -P {{.}}
+    {{/project_name}}
+    {{#queue_name}}
+#PBS -q {{.}}
+    {{/queue_name}}
+    {{!PBS uses specially named queues for reservations, so we send the job to the respective
+    queue when a reservation ID is specified.}}
+    {{#reservation_id}}
+#PBS -q {{.}}
+    {{/reservation_id}}
+{{/job.spec.attributes}}
+
+{{#custom_attributes}}
+    {{#pbs}}
+#PBS -{{key}} "{{value}}"
+    {{/pbs}}
+{{/custom_attributes}}
+
+
+{{!since we redirect the output manually, below, tell pbs not to do its own thing, since it
+only results in empty files that are not cleaned up}}
+#PBS -e /dev/null
+#PBS -o /dev/null
+
+{{#job.spec.inherit_environment}}
+#PBS -V
+{{/job.spec.inherit_environment}}
+{{#env}}
+#PBS -v {{name}}={{value}}
+{{/env}}
+
+PSIJ_NODEFILE="$PBS_NODEFILE"
+export PSIJ_NODEFILE
+
+
+{{#job.spec.directory}}
+cd "{{.}}"
+{{/job.spec.directory}}
+
+exec &>> "{{psij.script_dir}}/$PBS_JOBID.out"
+
+{{#psij.launch_command}}{{.}} {{/psij.launch_command}}
+
+{{!we redirect to a file tied to the native ID so that we can reach the file with attach().}}
+echo "$?" > "{{psij.script_dir}}/$PBS_JOBID.ec"
diff --git a/src/psij/executors/batch/pbspro/pbspro.mustache b/src/psij/executors/batch/pbs/pbspro.mustache
similarity index 79%
rename from src/psij/executors/batch/pbspro/pbspro.mustache
rename to src/psij/executors/batch/pbs/pbspro.mustache
index f0ed319e..001384cf 100644
--- a/src/psij/executors/batch/pbspro/pbspro.mustache
+++ b/src/psij/executors/batch/pbs/pbspro.mustache
@@ -1,7 +1,5 @@
 #!/bin/bash
 
-# TODO: job.spec.resources
-# TODO: job.spec.attributes
 
 {{#job.name}}
 #PBS -N="{{.}}"
@@ -11,6 +9,18 @@
 #PBS -V
 {{/job.spec.inherit_environment}}
 
+{{#job.spec.resources}}
+    {{#process_count}}
+#PBS -l select={{job.spec.resources.computed_node_count}}:ncpus={{.}}
+    {{/process_count}}
+    {{#exclusive_node_use}}
+#PBS -l place=scatter:exclhost
+    {{/exclusive_node_use}}
+    {{^exclusive_node_use}}
+#PBS -l place=scatter:shared
+    {{/exclusive_node_use}}
+{{/job.spec.resources}}
+
 {{#formatted_job_duration}}
 #PBS -l walltime={{.}}
 {{/formatted_job_duration}}
diff --git a/src/psij/executors/batch/pbspro.py b/src/psij/executors/batch/pbs_base.py
similarity index 91%
rename from src/psij/executors/batch/pbspro.py
rename to src/psij/executors/batch/pbs_base.py
index 260b9142..5c5f5d1c 100644
--- a/src/psij/executors/batch/pbspro.py
+++ b/src/psij/executors/batch/pbs_base.py
@@ -11,7 +11,7 @@
 
 _QSTAT_COMMAND = 'qstat'
 
-# This table maps PBS Pro state codes to the corresponding PSI/J
+# This table maps PBS state codes to the corresponding PSI/J
 # JobState.
 # See https://www.altair.com/pdfs/pbsworks/PBSReferenceGuide2021.1.pdf
 # page 361, section 8.1 "Job States"
@@ -39,14 +39,14 @@
 }
 
 
-class PBSProExecutorConfig(BatchSchedulerExecutorConfig):
-    """A configuration class for the PBS executor."""
+class PBSExecutorConfig(BatchSchedulerExecutorConfig):
+    """A generic configuration class for PBS-type executors."""
 
     pass
 
 
-class PBSProJobExecutor(BatchSchedulerExecutor):
-    """A :class:`~psij.JobExecutor` for PBS.
+class GenericPBSJobExecutor(BatchSchedulerExecutor):
+    """A generic :class:`~psij.JobExecutor` for PBS-type schedulers.
 
     PBS, originally developed by NASA, is one of the oldest resource managers still in use.
     A number of variations are available: `PBS Pro <https://www.altair.com/pbs-professional/>`_,
@@ -60,7 +60,8 @@ class PBSProJobExecutor(BatchSchedulerExecutor):
     Creates a batch script with #PBS directives when submitting a job.
     """
 
-    def __init__(self, url: Optional[str] = None, config: Optional[PBSProExecutorConfig] = None):
+    def __init__(self, generator: TemplatedScriptGenerator, url: Optional[str] = None,
+                 config: Optional[PBSExecutorConfig] = None) -> None:
         """
         Parameters
         ----------
@@ -69,11 +70,8 @@ def __init__(self, url: Optional[str] = None, config: Optional[PBSProExecutorCon
         config
             An optional configuration for this executor.
         """
-        if not config:
-            config = PBSProExecutorConfig()
         super().__init__(url=url, config=config)
-        self.generator = TemplatedScriptGenerator(config, Path(__file__).parent / 'pbspro'
-                                                  / 'pbspro.mustache')
+        self.generator = generator
 
     # Submit methods
 
diff --git a/src/psij/executors/batch/pbs_classic.py b/src/psij/executors/batch/pbs_classic.py
new file mode 100644
index 00000000..baa0f1f1
--- /dev/null
+++ b/src/psij/executors/batch/pbs_classic.py
@@ -0,0 +1,29 @@
+from pathlib import Path
+from typing import Optional
+
+from psij.executors.batch.pbs_base import PBSExecutorConfig, GenericPBSJobExecutor
+from psij.executors.batch.script_generator import TemplatedScriptGenerator
+
+
+class PBSClassicJobExecutor(GenericPBSJobExecutor):
+    """A :class:`~psij.JobExecutor` for classic PBS systems.
+
+    This executor uses resource specifications specific to Open PBS. Specifically,
+    this executor uses the `-l nodes=n:ppn=m` way of specifying nodes, which
+    differs from the scheme used by PBS Pro.
+    """
+
+    def __init__(self, url: Optional[str] = None, config: Optional[PBSExecutorConfig] = None):
+        """
+        Parameters
+        ----------
+        url
+            Not used, but required by the spec for automatic initialization.
+        config
+            An optional configuration for this executor.
+        """
+        if not config:
+            config = PBSExecutorConfig()
+        generator = TemplatedScriptGenerator(config, Path(__file__).parent / 'pbs'
+                                             / 'pbs_classic.mustache')
+        super().__init__(generator, url=url, config=config)


Test patch:
diff --git a/tests/test_executor.py b/tests/test_executor.py
index bb9e4560..7a545d3d 100644
--- a/tests/test_executor.py
+++ b/tests/test_executor.py
@@ -194,7 +194,7 @@ def _check_str_attrs(ex: BatchSchedulerExecutor, job: Job, names: List[str],
                 pytest.fail('Script generation failed for %s' % name)
 
 
-_PREFIX_TR = {'pbspro': 'pbs'}
+_PREFIX_TR = {'pbspro': 'pbs', 'pbs_classic': 'pbs'}
 
 
 def _get_attr_prefix(exec_name: str) -> str: