Repository: facebookincubator/submitit

Issue:
Slurm job name with non alphanumeric symbols sanitized to underscores
Submitit is not able to start a job with a name containing non alphanumeric symbols e.g _project-subproject:test1_. If user pass such name it is changed to _project_subproject_test1_.

It is possible to configure a slurm cluster to force an user to add non alphanumeric symbols in a job name. Big clusters require such features for better allocation of resources to different projects.

## Reproduction

Create python program to run jobs with colon and run it:
```
user@cluster-head-node: ./colon_job_executor.py --job_name project-subproject:test1
```
Expected result
The job _project-subproject:test1_ is executed at cluster.

Actual results:

The executed job name is _project_subproject_test1_

```
2021-10-21 04:26:02,340 INFO The job project-subproject:test1 executed
sbatch: error: Please set a name for this job, formatted like this:
sbatch: error:  project-<subproject>:*
sbatch: error: Batch job submission failed: Access/permission denied
subprocess.CalledProcessError: Command '['sbatch', './file_job.sh']' returned non-zero exit status 1.
```
## Root cause

The __make_sbatch_string_ function in _slurm.py_ applies utils.sanitize to job_name without _only_alphanum_ argument set to false.

https://github.com/facebookincubator/submitit/blob/main/submitit/slurm/slurm.py#L463

```
    parameters["signal"] = f"USR1@{signal_delay_s}"
    if job_name:
        parameters["job_name"] = utils.sanitize(job_name) #Line 463 - sanitize replaces "-" and ":" with "_"
    if comment:
        parameters["comment"] = utils.sanitize(comment, only_alphanum=False)
    if num_gpus is not None:
        warnings.warn(
            '"num_gpus" is deprecated, please use "gpus_per_node" instead (overwritting with num_gpus)'
        )
        parameters["gpus_per_node"] = parameters.pop("num_gpus", 0)
```

## Solution

The solution can be changing line 463:

```
    if job_name:
        parameters["job_name"] = utils.sanitize(job_name, only_alphanum=False) 
    if comment:
```



Discussion:

Patch:
diff --git a/submitit/core/utils.py b/submitit/core/utils.py
index f639852..36a3491 100644
--- a/submitit/core/utils.py
+++ b/submitit/core/utils.py
@@ -9,7 +9,6 @@ import io
 import itertools
 import os
 import pickle
-import re
 import select
 import shutil
 import subprocess
@@ -61,6 +60,9 @@ class JobPaths:
 
     @property
     def submission_file(self) -> Path:
+        if self.job_id and "_" in self.job_id:
+            # We only have one submission file per job array
+            return self._format_id(self.folder / "%A_submission.sh")
         return self._format_id(self.folder / "%j_submission.sh")
 
     @property
@@ -218,18 +220,6 @@ def copy_par_file(par_file: Union[str, Path], folder: Union[str, Path]) -> Path:
     return dst_name
 
 
-def sanitize(s: str, only_alphanum: bool = True, in_quotes: bool = True) -> str:
-    """Sanitize the string"""
-    if only_alphanum:
-        # Replace all consecutive non-alphanum character by _
-        return re.sub(r"[\W_]+", "_", s)
-    if in_quotes:
-        # Escape double quotes in the original string and put it between double quotes
-        s = s.replace('"', '\\"')
-        s = f'"{s}"'
-    return s
-
-
 def pickle_load(filename: Union[str, Path]) -> Any:
     # this is used by cloudpickle as well
     with open(filename, "rb") as ifile:
diff --git a/submitit/slurm/slurm.py b/submitit/slurm/slurm.py
index d503b6e..ae46da2 100644
--- a/submitit/slurm/slurm.py
+++ b/submitit/slurm/slurm.py
@@ -460,10 +460,6 @@ def _make_sbatch_string(
     parameters = {k: v for k, v in locals().items() if v is not None and k not in nonslurm}
     # rename and reformat parameters
     parameters["signal"] = f"USR1@{signal_delay_s}"
-    if job_name:
-        parameters["job_name"] = utils.sanitize(job_name)
-    if comment:
-        parameters["comment"] = utils.sanitize(comment, only_alphanum=False)
     if num_gpus is not None:
         warnings.warn(
             '"num_gpus" is deprecated, please use "gpus_per_node" instead (overwritting with num_gpus)'
@@ -473,8 +469,8 @@ def _make_sbatch_string(
         warnings.warn('"cpus_per_gpu" requires to set "gpus_per_task" to work (and not "gpus_per_node")')
     # add necessary parameters
     paths = utils.JobPaths(folder=folder)
-    stdout = shlex.quote(str(paths.stdout))
-    stderr = shlex.quote(str(paths.stderr))
+    stdout = str(paths.stdout)
+    stderr = str(paths.stderr)
     # Job arrays will write files in the form  <ARRAY_ID>_<ARRAY_TASK_ID>_<TASK_ID>
     if map_count is not None:
         assert isinstance(map_count, int) and map_count
@@ -489,26 +485,25 @@ def _make_sbatch_string(
         parameters.update(additional_parameters)
     # now create
     lines = ["#!/bin/bash", "", "# Parameters"]
-    # pylint: disable=consider-using-f-string
-    lines += [
-        "#SBATCH --{}{}".format(k.replace("_", "-"), "" if parameters[k] is True else f"={parameters[k]}")
-        for k in sorted(parameters)
-    ]
+    for k in sorted(parameters):
+        lines.append(_as_sbatch_flag(k, parameters[k]))
     # environment setup:
     if setup is not None:
         lines += ["", "# setup"] + setup
     # commandline (this will run the function and args specified in the file provided as argument)
     # We pass --output and --error here, because the SBATCH command doesn't work as expected with a filename pattern
-    stderr_flag = "" if stderr_to_stdout else f"--error {stderr}"
+    stderr_flags = [] if stderr_to_stdout else ["--error", stderr]
     if srun_args is None:
-        srun_command = "srun"
-    else:
-        srun_command = f"srun {' '.join(srun_args)}"
+        srun_args = []
+
+    srun_cmd = _shlex_join(["srun", "--unbuffered", "--output", stdout, *stderr_flags, *srun_args])
     lines += [
         "",
         "# command",
         "export SUBMITIT_EXECUTOR=slurm",
-        f"{srun_command} --output {stdout} {stderr_flag} --unbuffered {command}\n",
+        # The input "command" is supposed to be a valid shell command
+        " ".join((srun_cmd, command)),
+        "",
     ]
     return "\n".join(lines)
 
@@ -517,3 +512,17 @@ def _convert_mem(mem_gb: float) -> str:
     if mem_gb == int(mem_gb):
         return f"{int(mem_gb)}GB"
     return f"{int(mem_gb * 1024)}MB"
+
+
+def _as_sbatch_flag(key: str, value: tp.Any) -> str:
+    key = key.replace("_", "-")
+    if value is True:
+        return f"#SBATCH --{key}"
+
+    value = shlex.quote(str(value))
+    return f"#SBATCH --{key}={value}"
+
+
+def _shlex_join(split_command: tp.List[str]) -> str:
+    """Same as shlex.join, but that was only added in Python 3.8"""
+    return " ".join(shlex.quote(arg) for arg in split_command)


Test patch:
diff --git a/submitit/core/test_utils.py b/submitit/core/test_utils.py
index 42909b5..c5799d9 100644
--- a/submitit/core/test_utils.py
+++ b/submitit/core/test_utils.py
@@ -66,14 +66,6 @@ def test_slurmpaths_id_independent() -> None:
     assert output.name == "truc"
 
 
-def test_sanitize() -> None:
-    assert utils.sanitize("AlreadySanitized") == "AlreadySanitized"
-    assert utils.sanitize("Name with space") == "Name_with_space"
-    assert utils.sanitize("Name with space", only_alphanum=False) == '"Name with space"'
-    assert utils.sanitize("Name with    many    spaces") == "Name_with_many_spaces"
-    assert utils.sanitize(" Non alph@^ Nüm%") == "_Non_alph_Nüm_"
-
-
 def test_archive_dev_folders(tmp_path: Path) -> None:
     utils.archive_dev_folders([Path(__file__).parent], outfile=tmp_path.with_suffix(".tar.gz"))
     shutil.unpack_archive(str(tmp_path.with_suffix(".tar.gz")), extract_dir=tmp_path)
diff --git a/submitit/slurm/_sbatch_test_record.txt b/submitit/slurm/_sbatch_test_record.txt
index 75995d8..42f2cf3 100644
--- a/submitit/slurm/_sbatch_test_record.txt
+++ b/submitit/slurm/_sbatch_test_record.txt
@@ -15,4 +15,4 @@
 
 # command
 export SUBMITIT_EXECUTOR=slurm
-srun -vv --cpu-bind none --output /tmp/%j_%t_log.out --error /tmp/%j_%t_log.err --unbuffered blublu
+srun --unbuffered --output /tmp/%j_%t_log.out --error /tmp/%j_%t_log.err -vv --cpu-bind none blublu bar
diff --git a/submitit/slurm/test_slurm.py b/submitit/slurm/test_slurm.py
index 52f9b98..9b2a203 100644
--- a/submitit/slurm/test_slurm.py
+++ b/submitit/slurm/test_slurm.py
@@ -236,12 +236,12 @@ def test_checkpoint_and_exit(tmp_path: Path) -> None:
 
 def test_make_sbatch_string() -> None:
     string = slurm._make_sbatch_string(
-        command="blublu",
+        command="blublu bar",
         folder="/tmp",
         partition="learnfair",
         exclusive=True,
         additional_parameters=dict(blublu=12),
-        srun_args=["-vv", "--cpu-bind none"],
+        srun_args=["-vv", "--cpu-bind", "none"],
     )
     assert "partition" in string
     assert "--command" not in string