feat: preventing overwrites (#358)

From this PR onwards, overwriting flags already used internally with the
`slurm_extra` parameter. The plugin is feature rich and gets improved
continuously. Overwriting internally used flags (via CLI or resources)
already caused a number of issues. Now, we should have a reasonable
error message, instead.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Centralized SLURM extra validation to block forbidden SLURM options;
account/partition handling simplified to single-string arguments for
submissions.

* **Documentation**
* Updated slurm_extra example (removed QoS) and added a note advising
not to overwrite internal SLURM flags; recommend using profiles.

* **Tests**
* Added tests for slurm_extra validation, SBATCH formatting
expectations, and related error messages.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Author: cmeesters

docs/further.md

@@ -486,9 +486,11 @@ rule myrule:
     input: ...
     output: ...
     resources:
-        slurm_extra="--qos=long --mail-type=ALL --mail-user=<your email>"
+        slurm_extra="--mail-type=ALL --mail-user=<your email>"
 ```
 
+.. note:: We prevent you from overwriting all flags, which are used or configurable internally. This prevents conflicts and breaking job submission.
+
 Again, rather use a [profile](https://snakemake.readthedocs.io/en/latest/executing/cli.html#profiles) to specify such resources.
 
 ### Software Recommendations

snakemake_executor_plugin_slurm/__init__.py

@@ -18,6 +18,7 @@
 
 from snakemake_interface_executor_plugins.executors.base import SubmittedJobInfo
 from snakemake_interface_executor_plugins.executors.remote import RemoteExecutor
+
 from snakemake_interface_executor_plugins.settings import (
     ExecutorSettingsBase,
     CommonSettings,
@@ -34,6 +35,7 @@
 )
 from .efficiency_report import create_efficiency_report
 from .submit_string import get_submit_command
+from .validation import validate_slurm_extra
 
 
 @dataclass
@@ -682,6 +684,7 @@ def get_account_arg(self, job: JobExecutorInterface):
             # we have to quote the account, because it might
             # contain build-in shell commands - see issue #354
             for account in accounts:
+                self.test_account(account)
                 yield f" -A {shlex.quote(account)}"
         else:
             if self._fallback_account_arg is None:
@@ -820,13 +823,5 @@ def get_default_partition(self, job):
         return ""
 
     def check_slurm_extra(self, job):
-        jobname = re.compile(r"--job-name[=?|\s+]|-J\s?")
-        if re.search(jobname, job.resources.slurm_extra):
-            raise WorkflowError(
-                "The --job-name option is not allowed in the 'slurm_extra' parameter. "
-                "The job name is set by snakemake and must not be overwritten. "
-                "It is internally used to check the stati of the all submitted jobs "
-                "by this workflow."
-                "Please consult the documentation if you are unsure how to "
-                "query the status of your jobs."
-            )
+        """Validate that slurm_extra doesn't contain executor-managed options."""
+        validate_slurm_extra(job)

snakemake_executor_plugin_slurm/validation.py

@@ -0,0 +1,75 @@
+"""
+SLURM parameter validation functions for the Snakemake executor plugin.
+"""
+
+import re
+from snakemake_interface_common.exceptions import WorkflowError
+
+
+def get_forbidden_slurm_options():
+    """
+    Return a dictionary of forbidden SLURM options that the executor manages.
+
+    Returns:
+        dict: Mapping of regex patterns to human-readable option names
+    """
+    return {
+        # Job identification and output
+        r"--job-name[=\s]|-J\s?": "job name",
+        r"--output[=\s]|-o\s": "output file",
+        r"--error[=\s]|-e\s": "error file",
+        r"--parsable": "parsable output",
+        r"--export[=\s]": "environment export",
+        r"--comment[=\s]": "job comment",
+        r"--workdir[=\s]|-D\s": "working directory",
+        # Account and partition
+        r"--account[=\s]|-A\s": "account",
+        r"--partition[=\s]|-p\s": "partition",
+        # Memory options
+        r"--mem[=\s]": "memory",
+        r"--mem-per-cpu[=\s]": "memory per CPU",
+        # CPU and task options
+        r"--ntasks[=\s]|-n\s": "number of tasks",
+        r"--ntasks-per-gpu[=\s]": "tasks per GPU",
+        r"--cpus-per-task[=\s]|-c\s": "CPUs per task",
+        r"--cpus-per-gpu[=\s]": "CPUs per GPU",
+        # Time and resource constraints
+        r"--time[=\s]|-t\s": "runtime/time limit",
+        r"--constraint[=\s]|-C\s": "node constraints",
+        r"--qos[=\s]": "quality of service",
+        r"--nodes[=\s]|-N\s": "number of nodes",
+        r"--clusters[=\s]": "cluster specification",
+        # GPU options
+        r"--gres[=\s]": "generic resources (GRES)",
+        r"--gpus[=\s]": "GPU allocation",
+    }
+
+
+def validate_slurm_extra(job):
+    """
+    Validate that slurm_extra doesn't contain executor-managed options.
+
+    Args:
+        job: Snakemake job object with resources attribute
+
+    Raises:
+        WorkflowError: If forbidden SLURM options are found in slurm_extra
+    """
+    # skip testing if no slurm_extra is set
+    slurm_extra = getattr(job.resources, "slurm_extra", None)
+    if not slurm_extra:
+        return
+
+    forbidden_options = get_forbidden_slurm_options()
+
+    for pattern, option_name in forbidden_options.items():
+        if re.search(pattern, slurm_extra):
+            raise WorkflowError(
+                f"The --{option_name.replace(' ', '-')} option is not "
+                f"allowed in the 'slurm_extra' parameter. "
+                f"The {option_name} is set by the snakemake executor plugin "
+                f"and must not be overwritten. "
+                f"Please use the appropriate snakemake resource "
+                f"specification instead. "
+                f"Consult the documentation for proper resource configuration."
+            )

tests/tests.py

@@ -15,6 +15,7 @@
 )
 from snakemake_executor_plugin_slurm.utils import set_gres_string
 from snakemake_executor_plugin_slurm.submit_string import get_submit_command
+from snakemake_executor_plugin_slurm.validation import validate_slurm_extra
 from snakemake_interface_common.exceptions import WorkflowError
 import pandas as pd
 
@@ -785,3 +786,80 @@ def test_wildcard_slash_replacement(self):
 
     # Verify no slashes remain in the wildcard string
     assert "/" not in wildcard_str
+
+
+class TestSlurmExtraValidation:
+    """Test cases for the validate_slurm_extra function."""
+
+    @pytest.fixture
+    def mock_job(self):
+        """Create a mock job with configurable slurm_extra resource."""
+
+        def _create_job(**resources):
+            mock_resources = MagicMock()
+            # Configure get method to return values from resources dict
+            mock_resources.get.side_effect = lambda key, default=None: resources.get(
+                key, default
+            )
+            # Add direct attribute access for certain resources
+            for key, value in resources.items():
+                setattr(mock_resources, key, value)
+
+            mock_job = MagicMock()
+            mock_job.resources = mock_resources
+            mock_job.name = "test_job"
+            mock_job.wildcards = {}
+            mock_job.is_group.return_value = False
+            mock_job.jobid = 1
+            return mock_job
+
+        return _create_job
+
+    def test_valid_slurm_extra(self, mock_job):
+        """Test that validation passes with allowed SLURM options."""
+        job = mock_job(slurm_extra="--mail-type=END [email protected]")
+        # Should not raise any exception
+        validate_slurm_extra(job)
+
+    def test_forbidden_job_name_long_form(self, mock_job):
+        """Test that --job-name is rejected."""
+        job = mock_job(slurm_extra="--job-name=my-job --mail-type=END")
+        with pytest.raises(WorkflowError, match=r"job-name.*not allowed"):
+            validate_slurm_extra(job)
+
+    def test_forbidden_job_name_short_form(self, mock_job):
+        """Test that -J is rejected."""
+        job = mock_job(slurm_extra="-J my-job --mail-type=END")
+        with pytest.raises(WorkflowError, match=r"job-name.*not allowed"):
+            validate_slurm_extra(job)
+
+    def test_forbidden_account_long_form(self, mock_job):
+        """Test that --account is rejected."""
+        job = mock_job(slurm_extra="--account=myaccount --mail-type=END")
+        with pytest.raises(WorkflowError, match=r"account.*not allowed"):
+            validate_slurm_extra(job)
+
+    def test_forbidden_account_short_form(self, mock_job):
+        """Test that -A is rejected."""
+        job = mock_job(slurm_extra="-A myaccount --mail-type=END")
+        with pytest.raises(WorkflowError, match=r"account.*not allowed"):
+            validate_slurm_extra(job)
+
+    def test_forbidden_comment(self, mock_job):
+        """Test that --comment is rejected."""
+        job = mock_job(slurm_extra="--comment='my comment' --mail-type=END")
+        with pytest.raises(WorkflowError, match=r"job-comment.*not allowed"):
+            validate_slurm_extra(job)
+
+    def test_forbidden_gres(self, mock_job):
+        """Test that --gres is rejected."""
+        job = mock_job(slurm_extra="--gres=gpu:1 --mail-type=END")
+        with pytest.raises(WorkflowError, match=r"generic-resources.*not allowed"):
+            validate_slurm_extra(job)
+
+    def test_multiple_forbidden_options(self, mock_job):
+        """Test that the first forbidden option found is reported."""
+        job = mock_job(slurm_extra="--job-name=test --account=myaccount")
+        # Should raise error for job-name (first one encountered)
+        with pytest.raises(WorkflowError, match=r"job-name.*not allowed"):
+            validate_slurm_extra(job)