Adding support for Snakemake v7 --slurm-sidecar #85
Conversation
There was a problem hiding this comment.
Thanks for this PR @holtgrewe! I'll try to find some time this week to have a look at it.
|
@holtgrewe @percyfal Just pinging to check if you had time to work on the PR. A lot of slurm users will be grateful for this PR. |
|
It's on my list... |
|
Note to self: need to get rid of the HTTP log messages on the console. |
Also done. |
|
@holtgrewe so I finally got some time to have a look at the PR. I have had some issues getting the tests to work with the local CI setup, for some of which I could identify the causes, for some it's more the case of the CI environment itself. For one thing, I believe the REST API is not activated in the slurm container, so for now I may just turn them off for the sidecar case. Hopefully that gets resolved though. On a more general note, the profile should be backwards compatible (snakemake <v7), which I think currently is not the case (correct me if I'm wrong)? For clarification on this, see the comments on the code. |
|
|
||
|
|
||
| def register_with_sidecar(jobid): | ||
| sidecar_vars = json.loads(SIDECAR_VARS) |
There was a problem hiding this comment.
Add
if SIDECAR_VARS is None:
return
to handle cases where we're not using sidecar (or some other way to handle this case; not calling register_with_sidecar at all?)
There was a problem hiding this comment.
In this case we would also need to adjust the config.yaml to not set cluster-sidecar, else snakemake will complain about the unknown option.
There was a problem hiding this comment.
Either that, or throw an error notifying the user that snakemake >=7 is needed. It's always possible to modify the config.yaml afterwards, commenting out relevant options. Still, it would be more user-friendly to make the sidecar config an option.
| else: | ||
| time.sleep(1) | ||
|
|
||
| return res[jobid] or "" |
There was a problem hiding this comment.
Dedent the return statement one level, otherwise get_status_direct returns None
But there I was wrong. Updating the snakemake docker image does indeed make use of the sidecar function resulting in functional interaction with the slurm controller in the slurm CI image. Great! |
Currently the sidecar does not use the |
OK was not at the end yet. |
|
@holtgrewe in case you missed it I made a PR to your PR indicating the necessary changes to make the CI run. If you want I can add them myself after merging. What remains to be addressed is how to handle backwards compatibility, but we could do it post merging. |
|
@holtgrewe I added the CI changes and merged your PR in #92. I also added a cookiecutter option to use sidecar (default) or not, thereby providing backwards compatibility. Give me a holler if you notice anything out of order. And thank you for your efforts! |
|
@percyfal thanks - sorry for being neither responsive nor active... |
Note that this requires at least Snakemake 7.x with the patch applied in snakemake/snakemake#1440 (hopefully as 7.0.2 or 7.1).