Test for pbs files on slurm #166

aalitaiga · 2017-08-09T15:02:56Z

No description provided.

bouthilx · 2017-08-15T15:17:55Z

smartdispatch/tests/pbs_slurm_test.py

+
+#PBS -N arrayJob
+#PBS -o arrayJob_%A_%a.out
+#PBS -t 1-2


We can ignore arrays as long as SmartDispatch is not using them.

bouthilx · 2017-08-15T15:20:10Z

smartdispatch/tests/pbs_slurm_test.py

+        )
+
+#
+# pbs_string2 = """\


Why a second PBS template?

bouthilx · 2017-08-15T15:24:26Z

smartdispatch/tests/pbs_slurm_test.py

+            text_file.write(command)
+        process = Popen("sbatch test.pbs", stdout=PIPE, stderr=PIPE, shell=True)
+        stdout, stderr = process.communicate()
+        assert_true("Submitted batch job" in stdout)


You can define the function as a Method of TestSlurm and use self.assertIn rather than assert_true

bouthilx · 2017-08-15T15:28:38Z

smartdispatch/tests/pbs_slurm_test.py

+        stdout, stderr = process.communicate()
+        job_params = [c.strip() for c in stdout.split("\n")[1:] if c != '']
+        # import ipdb; ipdb.set_trace()
+        assert_true(all(p == param for p in job_params))


Again, this would give a more detailed error message self.assertSequenceEqual(job_params, (param for _ in job_params))

bouthilx · 2017-08-15T15:33:48Z

smartdispatch/tests/pbs_slurm_test.py

+
+    def tearDown(self):
+        process = Popen("rm *.out test.pbs", stdout=PIPE, stderr=PIPE, shell=True)
+        stdout, stderr = process.communicate()


I think it'd be better to use glob instead of Popen.

for file_name in (glob.glob('*.out') + ["test.pbs"]): os.remove(file_name)

bouthilx · 2017-09-01T20:56:19Z

smartdispatch/tests/pbs_slurm_test.py

+#SBATCH --job-name=arrayJob
+#SBATCH --output=arrayJob_%A_%a.out
+#SBATCH --time=01:00:00
+#SBATCH --gres=gpu


-l naccelerators=x will give gpu:x with squeue -o gres, so you should use --gres=gpu:x in sbatch file too. You should not test for 1 only, give it a random integer too.

bouthilx · 2017-09-01T20:57:28Z

smartdispatch/tests/pbs_slurm_test.py

+#SBATCH --output=arrayJob_%A_%a.out
+#SBATCH --time=01:00:00
+#SBATCH --gres=gpu
+#SBATCH --constraint=gpu6gb


gpu:x and constraint should be formatable in sbatch string since we want to try different values.

bouthilx · 2017-09-21T15:32:42Z

smartdispatch/tests/pbs_slurm_test.py

+        output_array = output_array or param_array
+        for param, output in zip(param_array, output_array):
+            com = pbs_string.format(
+                string.format(command.format(param))


Just for clarity:
command -> command_template
com -> param_command

bouthilx · 2017-09-21T16:48:06Z

smartdispatch/job_generator.py

+
+    def _add_cluster_specific_rules(self):
+        for pbs in self.pbs_list:
+            node_resource = pbs.resources.pop('nodes')


It doesn't seem wise to pop ['nodes'] if there is other information than gpus and ppn. I would rather substitute "gpus=[0-9]+:" with "". However we would need to check if resources['nodes'] becomes empty afterwards and remove it if so. It is very likely to happen, but it is necessary if we want to keep other informations when present.

If go from "nodes=1:ppn=2:gpus=2" to "nodes=1" is it okay or I should remove nodes completely in this case?

bouthilx · 2017-09-21T16:49:32Z

smartdispatch/pbs.py

+        Parameters
+        ----------
+        **options : dict
+            each key is the name of a SBATCH option (see `Options`)


There is no Options section in this docstring

bouthilx · 2017-09-21T16:50:31Z

smartdispatch/pbs.py

+        """
+
+        for option_name, option_value in options.items():
+            self.sbatch_options[option_name] = option_value


We would need to add back the "-"/"--" here if we drop it in add_sbatch_flags.

if len(option_name) == 1: "-" + option_name else: "--" + option_name

Isn't there a duplicate at line 81? You can change it to option_name = "-" + option_name and leave line 81 like it is.

bouthilx · 2017-09-21T16:54:20Z

smartdispatch/job_generator.py

+
+        for flag in flags:
+            split = flag.find('=')
+            options[flag[:split]] = flag[split+1:]


It would be more consistent with the PBS options treatment if we drop the "-"/"--" in front of the option names. I would also raise a ValueError if neither are present.

bouthilx · 2017-09-21T17:00:27Z

smartdispatch/tests/pbs_slurm_test.py

@@ -39,6 +39,13 @@
 nvidia-smi
 """

+# Checking which cluster is running the tests first
+process = Popen("sacctmgr list cluster", stdout=PIPE, stderr=PIPE, shell=True)


would subprocess.check_output be simpler to use? It's used widely in this repo.

Also I would write functions for qstat and slurm cluster detections. Something like

def get_qstat_cluster_name(): """ ... Some doc ... Raises ------ OSError The cluster may not have qstat installed. """ output = Popen(["qstat", "-B"], stdout=PIPE).communicate()[0] return output.split('\n')[2].split(' ')[0] def get_slurm_cluster_name(): """ ... Some doc ... Raises ------ OSError The cluster may not have slurm installed. """ # ... Some code ...

Those would be used in detect_cluster()

bouthilx · 2017-09-21T17:10:03Z

smartdispatch/utils.py

@@ -115,6 +115,7 @@ def detect_cluster():
        output = Popen(["qstat", "-B"], stdout=PIPE).communicate()[0]
    except OSError:
        # If qstat is not available we assume that the cluster is unknown.
+        # TODO: handle MILA + CEDAR + GRAHAM


You can insert the function get_slurm_cluster_name() here for reference, but let it still return None. We don't want to add support for specific clusters now. That will be another ticket.

bouthilx · 2017-09-21T17:23:26Z

@aalitaiga You should rename pbs_slurm_test.py otherwise Travis will keep reporting failures. We could add --ignore-files= in .travis.yml but we don't want those tests to run locally neither. They are only usefull if we want to tests new settings on the slurm configuration, so they should only be ran alone. A name like verify_slurms_pbs_wrapper.py would be self-explanatory.

bouthilx · 2017-09-29T18:20:56Z

smartdispatch/job_generator.py

@@ -78,7 +78,12 @@ def add_sbatch_flags(self, flags):

        for flag in flags:
            split = flag.find('=')
-            options[flag[:split]] = flag[split+1:]
+            if flag.startswith('--'):
+                options[flag[2:split]] = flag[split+1:]


This would be simpler.

if not flag.startswith("-"): raise ValueError("Invalid SBATCH flag ({})".format(flag)) options[flag[:split].lstrip("-")] = flag[split+1:]

bouthilx · 2017-09-29T18:23:57Z

smartdispatch/job_generator.py

+            gpus = re.match(".*gpus=([0-9]+)", pbs.resources['nodes']).group(1)
+            ppn = re.match(".*ppn=([0-9]+)", pbs.resources['nodes']).group(1)
+            pbs.resources['nodes'] = re.sub("ppn=[0-9]+", "", pbs.resources['nodes'])
+            pbs.resources['nodes'] = re.sub(":gpus=[0-9]+", "", pbs.resources['nodes'])


You don't look for empty pbs.resources['nodes'] because there is always nodes=1 remaining? That would be fine in my opinion, I just want to be sure to understand for which reason you do not check.

Yes it is the reason

bouthilx · 2017-09-29T18:26:54Z

smartdispatch/utils.py

@@ -115,7 +115,7 @@ def detect_cluster():
        output = Popen(["qstat", "-B"], stdout=PIPE).communicate()[0]
    except OSError:
        # If qstat is not available we assume that the cluster is unknown.
-        # TODO: handle MILA + CEDAR + GRAHAM
+        cluster_name = get_slurm_cluster_name()


Please leave the TODO there. Otherwise, it is not obvious why it still returns None.

bouthilx · 2017-09-29T18:27:20Z

smartdispatch/utils.py

@@ -131,6 +131,11 @@ def detect_cluster():
        cluster_name = "hades"
    return cluster_name

+def get_slurm_cluster_name():


We need a unit test for this

There's no unittest for 'detect_cluster' should I make one for 'get_cluster_name'?

There should be one for each functions.

bouthilx · 2017-09-29T18:30:39Z

smartdispatch/job_generator.py

@@ -73,6 +73,21 @@ def add_pbs_flags(self, flags):
            pbs.add_resources(**resources)
            pbs.add_options(**options)

+    def add_sbatch_flags(self, flags):


We need a unit test for add_sbatch_flags

bouthilx · 2017-09-29T18:31:33Z

smartdispatch/job_generator.py

+
+class SlurmJobGenerator(JobGenerator):
+
+    def _add_cluster_specific_rules(self):


We need a unit test for this

bouthilx · 2017-09-29T18:31:53Z

scripts/smart-dispatch

@@ -206,6 +209,7 @@ def parse_arguments():

    parser.add_argument('-p', '--pool', type=int, help="Number of workers that will be consuming commands. Default: Nb commands")
    parser.add_argument('--pbsFlags', type=str, help='ADVANCED USAGE: Allow to pass a space seperated list of PBS flags. Ex:--pbsFlags="-lfeature=k80 -t0-4"')
+    parser.add_argument('--sbatchFlags', type=str, help='ADVANCED USAGE: Allow to pass a space seperated list of SBATCH flags. Ex:--sbatchFlags="-qos=high --output=file.out"')


We need a unit test for this.

bouthilx · 2017-09-29T18:38:34Z

We need unit-tests for

add_sbatch_flags
add_sbatch_options
~~command line option~~
job generator pbs file replacements (pnn, gpus, etc)

Make sure to test that what should pass, pass and what should fail fails. We need to try many different things to test the command line options. For instance, we need to try adding bad options or adding options already included by job generator. We need to make sure to test every cracks as this seems to me as an error-prone setup (replacing options for a framework by options a wrapper of another framework will recognize as what we want it to be for the latter framework 😓).

bouthilx · 2017-10-07T00:22:15Z

smartdispatch/job_generator.py

-            elif flag.startswith('-'):
-                options[flag[1:split]] = flag[split+1:]
+                if split == -1:
+                    raise ValueError("Invalid SBATCH flag ({})".format(flag))


We know it's invalid probably because it should be a single - or the = is missing. The error message should give a hint about it.

bouthilx · 2017-10-07T00:23:35Z

smartdispatch/job_generator.py

+                    raise ValueError("Invalid SBATCH flag ({})".format(flag))
+                options[flag[:split].lstrip("-")] = flag[split+1:]
+            elif flag.startswith('-') and split == -1:
+                options[flag[1:2]] = flag[2:]


simply flag[1]? 😛

bouthilx · 2017-10-07T00:29:57Z

smartdispatch/tests/test_job_generator.py

+    def test_add_sbatch_flag_invalid(self):
+        invalid_flags = ["--qos high", "gpu", "-lfeature=k80"]
+        for flag in invalid_flags:
+            assert_raises(ValueError, self._test_add_sbatch_flags, "--qos high")


you pass the same flag each time.

bouthilx · 2017-10-07T00:33:55Z

smartdispatch/tests/test_job_generator.py

+        self.pbs = job_generator.pbs_list
+
+    def test_ppn_ncpus(self):
+        assert_true("ppn" not in str(self.pbs[0]))


Don't forget to make a dummy mock of self._add_cluster_specific_rules() to test that otherwise ppn was indeed present in the PBS and ncpus was not.

coveralls · 2017-10-11T21:55:47Z

Coverage increased (+0.8%) to 95.346% when pulling a02e32b on aalitaiga:adrien_slurm into 9133e15 on SMART-Lab:master.

coveralls · 2017-10-12T01:08:05Z

Coverage increased (+0.8%) to 95.346% when pulling 39107f1 on aalitaiga:adrien_slurm into 9133e15 on SMART-Lab:master.

…andle slurm clusters

Why: For each option, add_sbatch_option would add the option in both the form --[OPTION_NAME] and [OPTION_NAME].

It will need many conversions, not only on resources, so better make it clean.

Slurm has no queues, so PBS option -q is invalid and non-convertible.

$PBS_JOBID was used to set the stdout/err of the job as well as in the commands. Replace them with $SLURM_JOB_ID. Also, workers were accessing os.environ[PBS_JOBID] so we added a second fetch on SLURM_JOB_ID in case os.environ[PBS_JOBID] gave undefined.

Slurm cannot be passed environment variables defined locally on command-line like PBS_FILENAME is. To bypass this, we add a definition in the prolog, making PBS_FILENAME available to all commands and epilog. NOTE: We leave PBS_FILENAME definition in command-line too such that any user using $PBS_FILENAME inside a custom pbsFlag can still do so.

PBS options -V is not converted properly to SBATCH --export ALL. We remove it and replace it with --export=ALL is the sbatch options.

Slurm does not have a equivalent environment variable set like PBS_WALLTIME. To avoid confusion, all variables PBS_WALLTIME are renamed to SBATCH_TIMELIMIT (the environment variable one would use to set --time with sbatch). As SBATCH_TIMELIMIT is not set automatically, we add it to the prolog to make it available to all commands and epilog. NOTE: PBS_WALLTIME is set in seconds, but we only have HH:MM:SS-like strings at the time of building the PBS file. We needed to add a walltime_to_seconds helper function to convert HH:MM:SS like strings into seconds, so that SBATCH_TIMELIMIT is set with seconds like PBS_WALLTIME.

It is possible to query the system to see if some commands are available using distutils.spawn.find_executable(command_name). Clusters where more than one launcher are available will still get launchers selected based on string matching. For instance, get_launcher("helios") would always return msub no matter what is available on the system.

JobGenerators are selected by job_generator_factory based on the cluster's name. We use a more flexible, duck typing approach for Slurm clusters. If cluster name is not known, or not any of the if-case clauses in the factory, then we look at which launchers are available in the system. If it is sbatch, then a SlurmJobGenerator is built, a JobGenerator otherwise.

The command `sacctmgr` fails on some computers (mila01 namely), but the current behavior gives the impression sbatch is simply not available. Printing the stderr makes it more obvious that sbatch should be available, but something is broken behind sacctmgr. It only appears when using -vv options nevertheless.

Adding a script to do automatic verifications to assert validity of the current code. The verifications are not automatic unit-tests, they need automatically checks that the process executed successfully, but the administrator still needs to verify manually, reading the logs, that the requested resources were provided. Verifications can easily be combined, building on top of each others, from complex ones to simpler ones. Here is a list of all the verification currently implemented for slurm clusters: 1. very_simple_task (1 CPU) 2. verify_simple_task_with_one_gpu (1 CPU 1 GPU) 3. verify_simple_task_with_many_gpus (1 CPU X GPU) 4. verify_many_task (X CPU) 5. verify_many_task_with_many_cores (XY CPU) 6. verify_many_task_with_one_gpu (X CPU X GPU) 7. verify_many_task_with_many_gpus (X CPU Y GPU) 8. verify_simple_task_with_autoresume_unneeded (1 CPU) 9. verify_simple_task_with_autoresume_needed (1 CPU) 10. verify_many_task_with_autoresume_needed (X CPU)

My initial though was that get_launcher should raise an error when no launcher is found on the system since there cannot be any job launcher. I realized that this would break the --doNotLaunch option that users may want to use on system with no launcher, just to create the files.

The tests were failing because the account was not specified.

The tests were failing because the account was not specified

There was a missing parentheses which was causing a bad conversion of "DD:HH:MM:SS" to seconds. The unit-test was also missing the same parentheses. I added a unit-test to make sure such error could not occur again.

coveralls · 2017-10-18T01:03:13Z

Coverage increased (+1.2%) to 95.738% when pulling 33c048b on aalitaiga:adrien_slurm into 9133e15 on SMART-Lab:master.

aalitaiga force-pushed the adrien_slurm branch 2 times, most recently from 355547e to 6f31489 Compare August 11, 2017 14:40

bouthilx reviewed Aug 29, 2017

View reviewed changes

bouthilx reviewed Sep 1, 2017

View reviewed changes

bouthilx reviewed Sep 21, 2017

View reviewed changes

bouthilx reviewed Sep 29, 2017

View reviewed changes

bouthilx reviewed Oct 7, 2017

View reviewed changes

This was referenced Oct 10, 2017

Add Cedar support #168

Open

Add Graham support #169

Open

Add MILA support #170

Open

aalitaiga force-pushed the adrien_slurm branch 3 times, most recently from b42e8bd to a02e32b Compare October 11, 2017 21:51

aalitaiga force-pushed the adrien_slurm branch 2 times, most recently from 5bd3c86 to 39107f1 Compare October 12, 2017 01:06

aalitaiga and others added 28 commits October 17, 2017 20:55

Fixed naccelerators issue

eb4d473

Updated tests to skip on Graham and Cedar smartdispatch modified to h…

c2f2de6

…andle slurm clusters

Cleaned code with PR feedback

dcf1504

Updated tests

23e8a01

Updated tests using mock

93a4a3a

Refactor detect_cluster tests

6952922

Small changes in TestSlurmQueue

60610b0

Fix add_sbatch_option bug

39fa04e

Why: For each option, add_sbatch_option would add the option in both the form --[OPTION_NAME] and [OPTION_NAME].

Refactor SlurmJobGenerator

255920c

It will need many conversions, not only on resources, so better make it clean.

Remove queue name for Slurm clusters

ce370bc

Slurm has no queues, so PBS option -q is invalid and non-convertible.

Fix env var export option for Slurm

22cfb38

PBS options -V is not converted properly to SBATCH --export ALL. We remove it and replace it with --export=ALL is the sbatch options.

Add sbatch to command-line launcher options

0b05a94

Updated documentation for slurm clusters

8565d69

Add verification script for cedar

1112ec9

Add verification script for graham

8502e22

Add verification script for mila

5baabe1

Updated README

eb91544

Set properly account in verify_graham

14c5819

The tests were failing because the account was not specified.

Set properly account in verify_cedar

fde46db

The tests were failing because the account was not specified

Fix walltime_to_seconds convertion

33c048b

There was a missing parentheses which was causing a bad conversion of "DD:HH:MM:SS" to seconds. The unit-test was also missing the same parentheses. I added a unit-test to make sure such error could not occur again.

bouthilx force-pushed the adrien_slurm branch from 1dea0d8 to 33c048b Compare October 18, 2017 01:01


		class SlurmJobGenerator(JobGenerator):

		def _add_cluster_specific_rules(self):

Test for pbs files on slurm #166

Are you sure you want to change the base?

Test for pbs files on slurm #166

Conversation

aalitaiga commented Aug 9, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bouthilx Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

bouthilx commented Sep 21, 2017

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

bouthilx commented Sep 29, 2017 • edited Loading

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

coveralls commented Oct 11, 2017

coveralls commented Oct 12, 2017

coveralls commented Oct 18, 2017

bouthilx Sep 21, 2017 •

edited

Loading

bouthilx commented Sep 29, 2017 •

edited

Loading