yuch7 / cwlexec

A new open source tool to run CWL workflows on LSF
Other
36 stars 8 forks source link

add the -n option to model.conf files #48

Closed drkennetz closed 5 years ago

drkennetz commented 5 years ago

I have tried to setup an LSF.conf for a workflow that looks like this:

{
   "queue": "standard",
   "steps": {
       "step1": {
             "rerunnable": false,
             "res_req: "rusage[mem=20000]",
             "num_processors": 4
        }
   }
}

And it fails, so I stepped into your code here: https://github.com/IBMSpectrumComputing/cwlexec/blob/master/src/main/java/com/ibm/spectrumcomputing/cwl/model/conf/FlowExecConf.java

and found that you have nothing that handles the -n option for LSF which would distribute to multiple processors.

I'm tagging as a feature enhancement because without this, you can't make a job distributable without hard-coding it in the cwl source, which we don't want backend users to have to do.

It seems like you could do it with adding:

    private int processors
    ...

    public int getProcessors(){
        return processors
    }
    /**
     * Sets the LSF resource requirement (-n) option
     *
     * @param processors
     *                     The LSF num_processors requirement option
     */
    public void setProcessors(int processors) {
          this.processors. = processors;
    }

To the files FlowExecConf.java and StepExecConf.java.

I don't know if there is anything else that needs changing but this seems like a great feature to add.

skeeey commented 5 years ago

@drkennetz you also need to add the code to add the processor argument to bsub command, the related code can be found from https://github.com/IBMSpectrumComputing/cwlexec/blob/master/src/main/java/com/ibm/spectrumcomputing/cwl/exec/service/CWLLSFCommandServiceImpl.java#L137

drkennetz commented 5 years ago

@skeeey I can fork this and create a PR after I make changes and test it. Is this acceptable to you?

drkennetz commented 5 years ago

PR submitted. Thanks!

mr-c commented 5 years ago

Another option, which is more portable, is specifying override requirements using the cwltool:overrides extension in v1.0 or the native CWL v1.1 feature cwl:requirements to the input object

mr-c commented 5 years ago

(neither of which I've tested with CWLEXEC)

drkennetz commented 5 years ago

Me neither! And this was a decent fix that was easy enough for me to make. Why do you say that would make it more portable?

mr-c commented 5 years ago

Your fix looks useful, and is similar to a feature toil-cwl-runner has. I just want to make people aware of other options as well.

drkennetz commented 5 years ago

It might be worth updating the docs to include "processors" in the bsub options support. Just whenever you get some time.

skeeey commented 5 years ago

Thanks @drkennetz :-), I have updated the document