From 77a8fa1c512bfdb49be7b97a654ea9284bab16f1 Mon Sep 17 00:00:00 2001 From: Christian Meesters Date: Thu, 4 Sep 2025 19:32:35 +0200 Subject: [PATCH 1/8] fix: first account testing with sacctmgr, then sshare --- snakemake_executor_plugin_slurm/__init__.py | 30 ++++++++++++--------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/snakemake_executor_plugin_slurm/__init__.py b/snakemake_executor_plugin_slurm/__init__.py index 40ca9b9..b1f9525 100644 --- a/snakemake_executor_plugin_slurm/__init__.py +++ b/snakemake_executor_plugin_slurm/__init__.py @@ -657,9 +657,13 @@ def get_account_arg(self, job: JobExecutorInterface): else raises an error - implicetly. """ if job.resources.get("slurm_account"): - # here, we check whether the given or guessed account is valid - # if not, a WorkflowError is raised - self.test_account(job.resources.slurm_account) + # split the account upon ',' and whitespace, to allow + # multiple accounts being given + accounts = re.split(r"[,\s]+", job.resources.slurm_account) + for account in accounts: + # here, we check whether the given or guessed account is valid + # if not, a WorkflowError is raised + self.test_account(account) return f" -A '{job.resources.slurm_account}'" else: if self._fallback_account_arg is None: @@ -719,32 +723,34 @@ def test_account(self, account): """ tests whether the given account is registered, raises an error, if not """ - cmd = "sshare -U --format Account%256 --noheader" + # first we need to test with sacctmgr because sshare might not + # work in a multicluster environment + cmd = f'sacctmgr -n -s list user "{os.environ["USER"]}" format=account%256' try: accounts = subprocess.check_output( cmd, shell=True, text=True, stderr=subprocess.PIPE ) except subprocess.CalledProcessError as e: - sshare_report = ( + sacctmgr_report = ( "Unable to test the validity of the given or guessed" - f" SLURM account '{account}' with sshare: {e.stderr}." + f" SLURM account '{account}' with sacctmgr: {e.stderr}." ) accounts = "" if not accounts.strip(): - cmd = f'sacctmgr -n -s list user "{os.environ["USER"]}" format=account%256' + cmd = "sshare -U --format Account%256 --noheader" try: accounts = subprocess.check_output( cmd, shell=True, text=True, stderr=subprocess.PIPE ) except subprocess.CalledProcessError as e: - sacctmgr_report = ( + sshare_report = ( "Unable to test the validity of the given or guessed " - f"SLURM account '{account}' with sacctmgr: {e.stderr}." + f"SLURM account '{account}' with sshare: {e.stderr}." ) raise WorkflowError( - f"The 'sshare' reported: '{sshare_report}' " - f"and likewise 'sacctmgr' reported: '{sacctmgr_report}'." + f"The 'sacctmgr' reported: '{sacctmgr_report}' " + f"and likewise 'sshare' reported: '{sshare_report}'." ) # The set() has been introduced during review to eliminate @@ -753,7 +759,7 @@ def test_account(self, account): if not accounts: self.logger.warning( - f"Both 'sshare' and 'sacctmgr' returned empty results for account " + f"Both 'sacctmgr' and 'sshare' returned empty results for account " f"'{account}'. Proceeding without account validation." ) return "" From e5759b4f4c70507174d236bf1045ac813f3b0acc Mon Sep 17 00:00:00 2001 From: Christian Meesters Date: Mon, 8 Sep 2025 22:56:47 +0200 Subject: [PATCH 2/8] fix: no empty reports --- snakemake_executor_plugin_slurm/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/snakemake_executor_plugin_slurm/__init__.py b/snakemake_executor_plugin_slurm/__init__.py index b1f9525..4741a22 100644 --- a/snakemake_executor_plugin_slurm/__init__.py +++ b/snakemake_executor_plugin_slurm/__init__.py @@ -726,6 +726,7 @@ def test_account(self, account): # first we need to test with sacctmgr because sshare might not # work in a multicluster environment cmd = f'sacctmgr -n -s list user "{os.environ["USER"]}" format=account%256' + sacctmgr_report = sshare_report = "" try: accounts = subprocess.check_output( cmd, shell=True, text=True, stderr=subprocess.PIPE From f8a8cf3220da0fb4eac466bce963027de2ce123a Mon Sep 17 00:00:00 2001 From: Christian Meesters Date: Mon, 15 Sep 2025 12:14:07 +0000 Subject: [PATCH 3/8] Update snakemake_executor_plugin_slurm/__init__.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- snakemake_executor_plugin_slurm/__init__.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/snakemake_executor_plugin_slurm/__init__.py b/snakemake_executor_plugin_slurm/__init__.py index 4741a22..cf5a77c 100644 --- a/snakemake_executor_plugin_slurm/__init__.py +++ b/snakemake_executor_plugin_slurm/__init__.py @@ -659,12 +659,18 @@ def get_account_arg(self, job: JobExecutorInterface): if job.resources.get("slurm_account"): # split the account upon ',' and whitespace, to allow # multiple accounts being given - accounts = re.split(r"[,\s]+", job.resources.slurm_account) + accounts = [a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a] for account in accounts: # here, we check whether the given or guessed account is valid # if not, a WorkflowError is raised self.test_account(account) - return f" -A '{job.resources.slurm_account}'" + # sbatch accepts a single --account; use the first valid token + selected = accounts[0] + if len(accounts) > 1: + self.logger.info( + f"Multiple accounts provided ({accounts}); submitting with '{selected}'." + ) + return f" -A {shlex.quote(selected)}" else: if self._fallback_account_arg is None: self.logger.warning("No SLURM account given, trying to guess.") From f313166f1f43551a45a356b40304f8104f954046 Mon Sep 17 00:00:00 2001 From: meesters Date: Mon, 15 Sep 2025 14:22:04 +0200 Subject: [PATCH 4/8] fix: formatting --- snakemake_executor_plugin_slurm/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/snakemake_executor_plugin_slurm/__init__.py b/snakemake_executor_plugin_slurm/__init__.py index 2deeb8f..94b1ce4 100644 --- a/snakemake_executor_plugin_slurm/__init__.py +++ b/snakemake_executor_plugin_slurm/__init__.py @@ -670,7 +670,9 @@ def get_account_arg(self, job: JobExecutorInterface): if job.resources.get("slurm_account"): # split the account upon ',' and whitespace, to allow # multiple accounts being given - accounts = [a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a] + accounts = [ + a for a in re.split(r"[,\s]+", job.resources.slurm_account) if a + ] for account in accounts: # here, we check whether the given or guessed account is valid # if not, a WorkflowError is raised From 3ef1239e5f6818222c9cac7a761d554350ed2c2d Mon Sep 17 00:00:00 2001 From: meesters Date: Mon, 15 Sep 2025 15:10:38 +0200 Subject: [PATCH 5/8] feat: trying multiple accounts (experimental, not documented --- snakemake_executor_plugin_slurm/__init__.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/snakemake_executor_plugin_slurm/__init__.py b/snakemake_executor_plugin_slurm/__init__.py index 94b1ce4..2a5cd3f 100644 --- a/snakemake_executor_plugin_slurm/__init__.py +++ b/snakemake_executor_plugin_slurm/__init__.py @@ -677,13 +677,10 @@ def get_account_arg(self, job: JobExecutorInterface): # here, we check whether the given or guessed account is valid # if not, a WorkflowError is raised self.test_account(account) - # sbatch accepts a single --account; use the first valid token - selected = accounts[0] - if len(accounts) > 1: - self.logger.info( - f"Multiple accounts provided ({accounts}); submitting with '{selected}'." - ) - return f" -A {shlex.quote(selected)}" + # sbatch only allows one account per submission + # yield one after the other, if multiple were given + for account in accounts: + yield f" -A {shlex.quote(account)}" else: if self._fallback_account_arg is None: self.logger.warning("No SLURM account given, trying to guess.") From fc44cea3daf49c096af648ce4bd3253e1e351164 Mon Sep 17 00:00:00 2001 From: meesters Date: Mon, 15 Sep 2025 15:24:44 +0200 Subject: [PATCH 6/8] fix: account and partition quoting --- snakemake_executor_plugin_slurm/__init__.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/snakemake_executor_plugin_slurm/__init__.py b/snakemake_executor_plugin_slurm/__init__.py index 2a5cd3f..76fa9b3 100644 --- a/snakemake_executor_plugin_slurm/__init__.py +++ b/snakemake_executor_plugin_slurm/__init__.py @@ -679,6 +679,8 @@ def get_account_arg(self, job: JobExecutorInterface): self.test_account(account) # sbatch only allows one account per submission # yield one after the other, if multiple were given + # we have to quote the account, because it might + # contain build-in shell commands - see issue #354 for account in accounts: yield f" -A {shlex.quote(account)}" else: @@ -688,7 +690,7 @@ def get_account_arg(self, job: JobExecutorInterface): if account: self.logger.warning(f"Guessed SLURM account: {account}") self.test_account(f"{account}") - self._fallback_account_arg = f" -A {account}" + self._fallback_account_arg = f" -A {shlex.quote(account)}" else: self.logger.warning( "Unable to guess SLURM account. Trying to proceed without." @@ -711,7 +713,9 @@ def get_partition_arg(self, job: JobExecutorInterface): self._fallback_partition = self.get_default_partition(job) partition = self._fallback_partition if partition: - return f" -p {partition}" + # we have to quote the partition, because it might + # contain build-in shell commands + return f" -p {shlex.quote(partition)}" else: return "" From 92f1bb927fa8e2a1dde816f1eac45d472b1fa5dd Mon Sep 17 00:00:00 2001 From: meesters Date: Wed, 17 Sep 2025 13:24:23 +0200 Subject: [PATCH 7/8] fix: considering generator approach --- snakemake_executor_plugin_slurm/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/snakemake_executor_plugin_slurm/__init__.py b/snakemake_executor_plugin_slurm/__init__.py index 76fa9b3..43e4a29 100644 --- a/snakemake_executor_plugin_slurm/__init__.py +++ b/snakemake_executor_plugin_slurm/__init__.py @@ -307,7 +307,7 @@ def run_job(self, job: JobExecutorInterface): "run_uuid": self.run_uuid, "slurm_logfile": slurm_logfile, "comment_str": comment_str, - "account": self.get_account_arg(job), + "account": next(self.get_account_arg(job)), "partition": self.get_partition_arg(job), "workdir": self.workflow.workdir_init, } From 1979309e7022e80c4bccea5363c6ddc8f6dbec9c Mon Sep 17 00:00:00 2001 From: meesters Date: Wed, 17 Sep 2025 14:55:20 +0200 Subject: [PATCH 8/8] fix: generator approach --- snakemake_executor_plugin_slurm/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/snakemake_executor_plugin_slurm/__init__.py b/snakemake_executor_plugin_slurm/__init__.py index 43e4a29..64b5b4e 100644 --- a/snakemake_executor_plugin_slurm/__init__.py +++ b/snakemake_executor_plugin_slurm/__init__.py @@ -698,7 +698,7 @@ def get_account_arg(self, job: JobExecutorInterface): self._fallback_account_arg = ( "" # no account specific args for sbatch ) - return self._fallback_account_arg + yield self._fallback_account_arg def get_partition_arg(self, job: JobExecutorInterface): """