Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Black breaks comment formatting for slurm/pbs cluster directives. #1024

Closed
asford opened this issue Sep 17, 2019 · 11 comments
Closed

Black breaks comment formatting for slurm/pbs cluster directives. #1024

asford opened this issue Sep 17, 2019 · 11 comments
Labels
F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs.

Comments

@asford
Copy link

asford commented Sep 17, 2019

Slurm and PBS use block comments of the form #SBATCH or #PBS immediately following the #! header to provide default configuration parameters for a batch scheduler. Unfortunately, these tools don't support a pep8 compatible # SBATCH or # PBS syntax.

Applying black to a script with #SBATCH and #PBS directives silently breaks these tools, in that:

#!/usr/bin/env python
#SBATCH --exclusive
[...]

is reformatted as:

#!/usr/bin/env python
# SBATCH --exclusive
[...]

causing the --exclusive directive to be ignored by SLURM.

As the directive must follow the shebang and must not have a space between # and SBATCH. As such a # fmt: off escape hatche isn't a viable solution and (interestingly) don't prevent block comment reformatting in black's current implementation.

@asford
Copy link
Author

asford commented Sep 17, 2019

#960 attempted to address this issue with a special-case for these directives, but it seems clear in #1014 that a purely special-case solution isn't practical in the long term.

@JelleZijlstra @edreamleo would an escape hatch option --skip-comment-normalization akin to --skip-string-normalization be a viable solution? Taking an opinion here seems hard given the potential complexity of multiple semantically overloaded comment pragma. It seems like providing a bail-out option is the most blackish solution?

@zsol
Copy link
Collaborator

zsol commented Sep 17, 2019

I think that's the best solution I've seen so far, even though it would make Black produce code that's technically not PEP-8 compliant. But maybe it's the pragmatic choice.

@asford asford changed the title Black beaks comment formatting for slurm/pbs cluster directives. Black breaks comment formatting for slurm/pbs cluster directives. Sep 17, 2019
@asford
Copy link
Author

asford commented Sep 17, 2019

Yes, for clarity for incoming readers, the explicit ask in the referenced PRs is for a form on non-pep8 compliant output for semantically overloaded comments.

@thomas-albrecht
Copy link

I came across the very same problem and would very much appreciate a pragmatic solution.
Is there any progress on this issue?

@edreamleo
Copy link

Similarly, Leo's sentinel comments are not pep-8.

Leo now does allow whitespace between the '#" and '@', but that creates other problems, including massive diffs after blackening files.

In short, a pragmatic solution would be very much appreciated. It would be good if black could be told not to munge some (or all) comments. Thanks.

@JelleZijlstra JelleZijlstra added the F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. label May 30, 2021
@SomeoneSerge
Copy link

I think the pragmatic solution would be to make fmt: off work for comments, cf. #1245

@JelleZijlstra
Copy link
Collaborator

I do agree that # fmt: off should stop Black from adding spaces after #, and I'd accept a patch fixing that bug.

However, according to the OP that's not an option for these #SBATCH comments, because they must be right after the shebang line.

@SomeoneSerge
Copy link

However, according to the OP that's not an option for these #SBATCH comments, because they must be right after the shebang line.

I've just tried running a job with fmt: off between shebang and the SLURM directives and it worked. Idk yet if it's documented

@SomeoneSerge
Copy link

sbatch submits a batch script to Slurm. The batch script may be given to sbatch through a file name on the command line, or if no file name is specified, sbatch will read in a script from standard input. The batch script may contain options preceded with "#SBATCH" before any executable commands in the script. sbatch will stop processing further #SBATCH directives once the first non-comment non-whitespace line has been reached in the script.
...
https://slurm.schedmd.com/sbatch.html

Emphasis added

@tautomer
Copy link

tautomer commented Oct 29, 2022

However, according to the OP that's not an option for these #SBATCH comments, because they must be right after the shebang line.

I've just tried running a job with fmt: off between shebang and the SLURM directives and it worked. Idk yet if it's documented

I can confirm this is the behavior I observe as well.
As long as the fmt on/off derivative works on pure comment, this issue should be gone.
As of now, we can ada a dummy line of code at the end of SLURM derivatives before# fmt: on, like

#!/usr/bin/env python3
# fmt: off
#SBATCH --time=2-00:00:00
#SBATCH --nodes=1
#SBATCH --ntasks=40
#SBATCH --mail-type=all
#SBATCH -J test
#SBATCH --qos=long
#SBATCH -o test.log
# test
from time import sleep

sleep(10)
print(123)
# fmt: on

It's ugly, but at least works.

@JelleZijlstra
Copy link
Collaborator

Let's make a decision in #3668.

@JelleZijlstra JelleZijlstra closed this as not planned Won't fix, can't repro, duplicate, stale Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs.
Projects
None yet
Development

No branches or pull requests

7 participants