Snakemake-Profiles / lsf

Snakemake profile for running jobs on an LSF cluster
MIT License
36 stars 22 forks source link

runtime resource added #38

Closed NBMueller closed 3 years ago

NBMueller commented 3 years ago

Allows the usage of "time" / "runtime" / "walltime" as a snakemake resource, equivalent to "mem_mb".

NBMueller commented 3 years ago

Hi @mbhall88, what exact kind of test do you want me to add? Cheers

mbhall88 commented 3 years ago

Have a look at https://github.com/Snakemake-Profiles/lsf/blob/57302c69c9eff456b5a5d727a7ac30b2b684845d/tests/test_lsf_submit.py#L553-L567 for a "template".
Effectively, it would be good to have a test for each of your runtime keywords. i.e. if they're in the jobscript, check they appear in the submit command the Submitter produces.

kpj commented 3 years ago

Great PR! Will this automatically compute the right runtime for group jobs?

mbhall88 commented 3 years ago

Thanks @NBMueller - that test is great. Are you finished with this now? (i.e. can I merge this?)

@kpj sorry, I don't know if I understand your question properly.

kpj commented 3 years ago

@mbhall88 Apologies, I'll try to be more precise.

In Snakemake, you can create group jobs using the group keyword. Resource requirements for the group job (such as threads or resources.mem_mb) will be determined by looking at the requirements of individual contained jobs ("The resource usage of a group job is determined by sorting involved jobs topologically, summing resource usage per level and taking the maximum over all levels."). I was wondering how this would work for the new resources.runtime.

NBMueller commented 3 years ago

@mbhall88 Yes, should be good to go now. Thanks for merging! @kpj Any idea if this is handled by profile, by snakemake itself, or by the LSF? Couldn't find anything inside the profile for handling group resources, therefore I assume you have a 50/50 chance that it's working: if the LSF takes care, it should work, if it's something embedded into snakemake, it does not. Cheers

mbhall88 commented 3 years ago

@kpj I was not aware of this. And I certainly don't handle this with respect to resources. I just use the resources defined on the rule. The only group-specific behaviour is the naming of the job.

I never use group jobs so I wonder if you might be able to run some tests and see whether the profile does what you would expect?

kpj commented 3 years ago

I gave it a try and I think it is necessary to define the runtime as an integer (e.g. number of seconds) instead of a string. Otherwise Snakemake crashes with

Traceback (most recent call last):
  File "/snakemake/__init__.py", line 694, in snakemake
    success = workflow.execute(
  File "/snakemake/workflow.py", line 1017, in execute
    success = scheduler.schedule()
  File "/snakemake/scheduler.py", line 469, in schedule
    run = self.job_selector(needrun)
  File "/snakemake/scheduler.py", line 775, in job_selector_greedy
    a = list(map(self.job_weight, jobs))  # resource usage of jobs
  File "/snakemake/scheduler.py", line 848, in job_weight
    res = job.resources
  File "/snakemake/jobs.py", line 1208, in resources
    sibling_resources[res] += value
TypeError: unsupported operand type(s) for +=: 'int' and 'str'

But after defining my rules like this:

localrules: all

rule all:
    input: 'bar'

rule foo:
    output: touch('foo')
    group: 'test'
    resources:
        mem_mb=200,
        time=60

rule bar:
    input: 'foo'
    output: touch('bar')
    group: 'test'
    resources:
        mem_mb=100,
        time=123

the job group was submitted with the correct resource requirements "resources": {"mem_mb": 200, "time": 123}.

kpj commented 3 years ago

Wonderful, thanks!

In analogy to mem_mb, it might make sense to call the resource time_min.

codecov-io commented 3 years ago

Codecov Report

Merging #38 (e73607a) into master (57302c6) will increase coverage by 0.14%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   88.52%   88.67%   +0.14%     
==========================================
  Files           6        6              
  Lines         462      468       +6     
  Branches       44       47       +3     
==========================================
+ Hits          409      415       +6     
  Misses         46       46              
  Partials        7        7              
Flag Coverage Δ
unittests 88.67% <100.00%> (+0.14%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
{{cookiecutter.profile_name}}/lsf_submit.py 88.46% <100.00%> (+0.30%) :arrow_up:
{{cookiecutter.profile_name}}/lsf_config.py 100.00% <0.00%> (ø)
NBMueller commented 3 years ago

What you wrote makes total sense, I think it's ready to get merged.