fix: slurm default_resources quoting #2043

dlaehnemann · 2023-01-02T12:35:35Z

to start with, I'll try to generate a failing test by including quoted runtime specification in test_slurm_complex()
the fix is to use shlex.quote() instead of repr() for quoting shell command strings (which should also be safer)

Some background on proper shell quoting style can be found in this stackoverflow answer on [How to escape single quotes within single quoted strings

Description

This fixes errors in slurm execution that are generated due to incorrect quote escaping with repr(). These can for example be:

WorkflowError:
SLURM job submission failed. The error message was /bin/sh: 1: Syntax error: "(" unexpected

or:

WorkflowError:
SLURM job submission failed. The error message was /bin/sh: -c: line 0: unexpected EOF while looking for matching `''
/bin/sh: -c: line 1: syntax error: unexpected end of file

QC

The PR contains a test case for the changes or the changes are already covered by an existing test case.
The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

…n in test_slurm_complex()

…epr()` to allow proper interpretation by the calling shell

dlaehnemann · 2023-01-02T16:24:48Z

It might make sense to check other uses of repr() to see if they should also be replaced by shlex.quote(). But I'm just glad to have fixed my current problem and currently don't have the bandwidth to go looking further.

johanneskoester

Using shlex seems like a good idea here. There is one additional issue though: runtime is supposed to be given as an integer, in minutes (see here). Actually, there is a check missing which ensures that it is an integer and provides a reasonable error message if it is not.

johanneskoester · 2023-01-04T21:09:10Z

Using shlex seems like a good idea here. There is one additional issue though: runtime is supposed to be given as an integer, in minutes (see here). Actually, there is a check missing which ensures that it is an integer and provides a reasonable error message if it is not.

I know, sbatch accepts anything from minutes over to more complex formats, but as always, Snakemake should ensure that workflow definitions are as portable as possible, and not slurm specific. For more convenient specifications, we will have PR #1861 soon.

dlaehnemann · 2023-01-05T11:27:03Z

So, is it enough to just change the test to some fake resource (non-runtime=) and wait for the other PR to also address the respective int (or whatever more complex) testing?

johanneskoester · 2023-01-09T09:17:21Z

Yes, but it must be one of the resources that the slurm executor translates into an sbatch argument. For example constraint.

…ap=` quoting checks

…to have any features configured

…est instance to include feature annotations for nodes

sonarcloud · 2023-01-10T13:09:16Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

dlaehnemann · 2023-01-10T15:26:38Z

Just to quickly document the long history of commits here: I tried to also include proper quoting and testing for it for constraint= annotations, but I could not get the slurm testing harness for snakemake to properly configure Features annotations for nodes.

* to start with, I'll try to generate a failing test by including quoted runtime specification in test_slurm_complex() * the fix is to use [`shlex.quote()`](https://docs.python.org/3/library/shlex.html#shlex.quote) instead of [`repr()`](https://docs.python.org/3/library/functions.html#repr) for quoting shell command strings (which should also be safer) Some background on proper shell quoting style can be found in this [stackoverflow answer on `[How to escape single quotes within single quoted strings`](https://stackoverflow.com/a/1250279) ### Description This fixes errors in slurm execution that are generated due to incorrect quote escaping with `repr()`. These can for example be: ```bash WorkflowError: SLURM job submission failed. The error message was /bin/sh: 1: Syntax error: "(" unexpected ``` or: ```bash WorkflowError: SLURM job submission failed. The error message was /bin/sh: -c: line 0: unexpected EOF while looking for matching `'' /bin/sh: -c: line 1: syntax error: unexpected end of file ``` ### QC  * [x] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [x] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). Co-authored-by: David Laehnemann <david.laehnemann@helmholtz-hzi.de>

🤖 I have created a release *beep* *boop* --- ## [7.20.0](v7.19.1...v7.20.0) (2023-01-18) ### Features * add tes token ([#1966](#1966)) ([59a8fa0](59a8fa0)) * Add token auth to GitLab/GitHub hosting providers ([#1761](#1761)) ([e03a3b4](e03a3b4)), closes [#1301](#1301) * allow for human friendly resource definitions (e.g. mem="5GB", runtime="1d") ([#1861](#1861)) ([24610ac](24610ac)) ### Bug Fixes * 🐛 - fix hyperlink ([#2046](#2046)) ([9519d31](9519d31)) * Catch missing error stream in Slurm executor ([#2063](#2063)) ([c21fc7e](c21fc7e)) * correctly parse empty values in config cli ([#2032](#2032)) ([1b0689d](1b0689d)) * Correctly parse UserDicts in executors ([#2016](#2016)) ([e3926fa](e3926fa)) * Fix handling of --jobs in no-exec state ([#2029](#2029)) ([e8e8222](e8e8222)) * make `--show-failed-logs` handle empty log files ([#2039](#2039)) ([683c6f2](683c6f2)), closes [#2023](#2023) * make python version check more robust ([#2058](#2058)) ([e685621](e685621)) * parsing error when last line is comment ([#2054](#2054)) ([a928dd4](a928dd4)) * prevent overriding of retries when set to 0 ([#2053](#2053)) ([a328f3e](a328f3e)) * propagate attempt count from group to subjobs ([#2052](#2052)) ([da3f1c0](da3f1c0)) * remove overflow from rulegraph div in report ([9a0aaa7](9a0aaa7)) * skip type checks of missing dir in touch mode ([#2051](#2051)) ([ae00c25](ae00c25)) * slurm default_resources quoting ([#2043](#2043)) ([47d3fc3](47d3fc3)) * Update list of python versions in classifiers ([#2020](#2020)) ([7a98100](7a98100)) * use short argument name for `--chdir` for compatibility with Slurm <=v17 ([#2040](#2040)) ([a9ed3ec](a9ed3ec)) ### Documentation * Fix typo in SLURM help text ([#2049](#2049)) ([79b7025](79b7025)) * mention XDG_CACHE_HOME ([#2057](#2057)) ([ec2ef45](ec2ef45)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

🤖 I have created a release *beep* *boop* --- ## [7.20.0](snakemake/snakemake@v7.19.1...v7.20.0) (2023-01-18) ### Features * add tes token ([snakemake#1966](snakemake#1966)) ([59a8fa0](snakemake@59a8fa0)) * Add token auth to GitLab/GitHub hosting providers ([snakemake#1761](snakemake#1761)) ([e03a3b4](snakemake@e03a3b4)), closes [snakemake#1301](snakemake#1301) * allow for human friendly resource definitions (e.g. mem="5GB", runtime="1d") ([snakemake#1861](snakemake#1861)) ([24610ac](snakemake@24610ac)) ### Bug Fixes * 🐛 - fix hyperlink ([snakemake#2046](snakemake#2046)) ([9519d31](snakemake@9519d31)) * Catch missing error stream in Slurm executor ([snakemake#2063](snakemake#2063)) ([c21fc7e](snakemake@c21fc7e)) * correctly parse empty values in config cli ([snakemake#2032](snakemake#2032)) ([1b0689d](snakemake@1b0689d)) * Correctly parse UserDicts in executors ([snakemake#2016](snakemake#2016)) ([e3926fa](snakemake@e3926fa)) * Fix handling of --jobs in no-exec state ([snakemake#2029](snakemake#2029)) ([e8e8222](snakemake@e8e8222)) * make `--show-failed-logs` handle empty log files ([snakemake#2039](snakemake#2039)) ([683c6f2](snakemake@683c6f2)), closes [snakemake#2023](snakemake#2023) * make python version check more robust ([snakemake#2058](snakemake#2058)) ([e685621](snakemake@e685621)) * parsing error when last line is comment ([snakemake#2054](snakemake#2054)) ([a928dd4](snakemake@a928dd4)) * prevent overriding of retries when set to 0 ([snakemake#2053](snakemake#2053)) ([a328f3e](snakemake@a328f3e)) * propagate attempt count from group to subjobs ([snakemake#2052](snakemake#2052)) ([da3f1c0](snakemake@da3f1c0)) * remove overflow from rulegraph div in report ([9a0aaa7](snakemake@9a0aaa7)) * skip type checks of missing dir in touch mode ([snakemake#2051](snakemake#2051)) ([ae00c25](snakemake@ae00c25)) * slurm default_resources quoting ([snakemake#2043](snakemake#2043)) ([47d3fc3](snakemake@47d3fc3)) * Update list of python versions in classifiers ([snakemake#2020](snakemake#2020)) ([7a98100](snakemake@7a98100)) * use short argument name for `--chdir` for compatibility with Slurm <=v17 ([snakemake#2040](snakemake#2040)) ([a9ed3ec](snakemake@a9ed3ec)) ### Documentation * Fix typo in SLURM help text ([snakemake#2049](snakemake#2049)) ([79b7025](snakemake@79b7025)) * mention XDG_CACHE_HOME ([snakemake#2057](snakemake#2057)) ([ec2ef45](snakemake@ec2ef45)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

### Description - Removes unnecessary setting of Snakefile in AzBatch executor. This was 1. not necessary, and 2. was causing a failure to resolve the Snakefile path when it was located at `workflow/Snakefile`. - Since default resources is now included in the command per #2395 , I needed to update the shell quoting so the command doesn't fail because of the parentheses in the default resources. Used the same solution as #2043 and it works nicely. - Updated the masking regex to be more generic for SAS token possibilities ### QC  * [x] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [x] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

try to generate failing test by including quoted runtime specificatio…

b93b161

…n in test_slurm_complex()

dlaehnemann requested a review from johanneskoester as a code owner January 2, 2023 12:35

fix: switch slurm --wrap= argument quoting to shlex.quote() from `r…

0261d33

…epr()` to allow proper interpretation by the calling shell

johanneskoester requested changes Jan 4, 2023

View reviewed changes

David Laehnemann added 22 commits January 9, 2023 10:53

adjust test case according PR #2043 discussion

f7b62ab

try fixing slurm constraint quoting

eab7db7

try reversing quoting of constraint, add disk_mb expression for `--wr…

0bc3a00

…ap=` quoting checks

try explicitly quoting constraints specs

0fc6c81

try with empty constraints, as the slurm test instance does not seem …

f246bf3

…to have any features configured

try configuring slurm nodes with features

5b6b2ef

request the feature from node-2 for the complex test

b001ff6

run sinfo after slurm setup to get an idea of what is created

6f4db08

try sinfo after slurm account adding

c8d6b62

further uncomment and set slurm_partitions

61ac0aa

simplify node specification under partition

60b40ac

try reverting to hosts: all from localhost

74c4774

try reverting further hosts stuff towards the galaxyproject state

aad673d

try specifying vm hosts on ansible-playbook command line

02b8098

fix ansible-playbook hosts spec

059a572

try host ips

1d90622

specify port for hosts

f891422

try hosts_inventory file to specify ports of hosts

1089ace

try fixing workflow by using just one with: statement

47d22e4

create hosts_inventory.txt in its own substep

7d7e73f

fix hosts list name and fix ports

a310b45

try guest port for hosts

3eeba5b

David Laehnemann and others added 3 commits January 10, 2023 13:52

revert Github Actions workflow to main branch state

bbca96d

remove constraint quoting and testing, as I could not get the slurm t…

b00c765

…est instance to include feature annotations for nodes

Merge branch 'main' into fix-slurm-resources-quoting

bf4998a

johanneskoester approved these changes Jan 10, 2023

View reviewed changes

johanneskoester merged commit 47d3fc3 into main Jan 10, 2023

johanneskoester deleted the fix-slurm-resources-quoting branch January 10, 2023 15:53

github-actions bot mentioned this pull request Jan 10, 2023

chore(main): release 7.20.0 #2018

Merged

jakevc mentioned this pull request Aug 5, 2023

fix: unnecessary set Snakefile in AzBatch executor #2397

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: slurm default_resources quoting #2043

fix: slurm default_resources quoting #2043

dlaehnemann commented Jan 2, 2023 •

edited

Loading

dlaehnemann commented Jan 2, 2023

johanneskoester left a comment

johanneskoester commented Jan 4, 2023

dlaehnemann commented Jan 5, 2023

johanneskoester commented Jan 9, 2023

sonarcloud bot commented Jan 10, 2023

dlaehnemann commented Jan 10, 2023

fix: slurm default_resources quoting #2043

fix: slurm default_resources quoting #2043

Conversation

dlaehnemann commented Jan 2, 2023 • edited Loading

Description

QC

dlaehnemann commented Jan 2, 2023

johanneskoester left a comment

Choose a reason for hiding this comment

johanneskoester commented Jan 4, 2023

dlaehnemann commented Jan 5, 2023

johanneskoester commented Jan 9, 2023

sonarcloud bot commented Jan 10, 2023

dlaehnemann commented Jan 10, 2023

dlaehnemann commented Jan 2, 2023 •

edited

Loading