search

Snakemake-Profiles / slurm

Cookiecutter for snakemake slurm profile
MIT License
126 stars 44 forks source link

Adding support for Snakemake v7 --slurm-sidecar #85

Closed holtgrewe closed 2 years ago

holtgrewe commented 2 years ago

Note that this requires at least Snakemake 7.x with the patch applied in https://github.com/snakemake/snakemake/pull/1440 (hopefully as 7.0.2 or 7.1).

fgypas commented 2 years ago

@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.

holtgrewe commented 2 years ago

It's on my list...

holtgrewe commented 2 years ago

Note to self: need to get rid of the HTTP log messages on the console.

holtgrewe commented 2 years ago

Note to self: need to get rid of the HTTP log messages on the console.

Also done.

percyfal commented 2 years ago

@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.

percyfal commented 2 years ago

@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.

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!

holtgrewe commented 2 years ago

@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 https://github.com/giovtorres/docker-centos7-slurm/pull/42, so for now I may just turn them off for the sidecar case. Hopefully that gets resolved though.

Currently the sidecar does not use the slurmrestd. I think this would be a worthwhile additional feature and could be enabled by an environment variable (e.g., configuring the base URL of the REST API).

holtgrewe commented 2 years ago

Currently the sidecar does not use the slurmrestd. I think this would be a worthwhile additional feature and could be enabled by an environment variable (e.g., configuring the base URL of the REST API).

OK was not at the end yet.

percyfal commented 2 years ago

@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.

percyfal commented 2 years ago

@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!

holtgrewe commented 2 years ago

@percyfal thanks - sorry for being neither responsive nor active...