-
Notifications
You must be signed in to change notification settings - Fork 4
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
Accommodate Nextflow CPU/Memory in Nomad Job taskResources #27
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments for informing the reviewer.
void "submit a task with docker volume"(){ | ||
given: | ||
def config = new NomadConfig( | ||
client:[ | ||
address : "http://${mockWebServer.hostName}:${mockWebServer.port}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this test as we decided not to continue with docker volume
.
|
||
JobRegisterRequest jobRegisterRequest = new JobRegisterRequest(); | ||
jobRegisterRequest.setJob(job); | ||
JobRegisterResponse jobRegisterResponse = jobsApi.registerJob(jobRegisterRequest, config.jobOpts.region, config.jobOpts.namespace, null, null) | ||
jobRegisterResponse.evalID | ||
} | ||
|
||
TaskGroup createTaskGroup(String id, String name, String image, List<String> args, String workingDir, Map<String, String>env){ | ||
def task = createTask(id, image, args, workingDir, env) | ||
TaskGroup createTaskGroup(String id, String name, String image, List<String> args, String workingDir, Map<String, String>env, Resources resources){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't like to expose specific nomad objects outside of the service
I mean, Resources is something strong related with the Service but not with the Task. Maybe we can use some TaskResource class as parameter and let the Service do the conversion to a nomad Resource object
it sounds a little more complicated/elaborated but in this way we have separate both concepts (nextflow vs nomad)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jagedn , okay so I've tried to refactor the code keeping different layers separate. Let me know what you think about the current structure?
I do think that we can explore the idea regarding a specific "translation" class which covers all aspects of NomadJobDefinition
from a NextflowTask
, however I suggest that we still continue the current iterations and revisit this once the plugin has stabilised in its design - what do you think?
I have drafted an initial support for cores and memory.
NOTE: As Nomad interprets
cpus
inMegaHertz
different from the integer values inNextflow
, therefore, I have mappedNextflow CPU -> Nomad Cores
.Tested this with the
nf-core/fetchngs
pipelineResources