Allofus - Adapt targene for use on the All of Us Researcher Workbench #174
Conversation
…/.nextflow/config
There was a problem hiding this comment.
Thank you Breeshey.
On the two currently non-ticked boxes:
- did you manage to run the pipeline entirely?
- What is the format of the input data, does it require an additional model to the "custom"? It might be good to explicitely state in the docs even if it can use this cohort mode.
| @@ -0,0 +1,23 @@ | |||
| process { | |||
| memory = { 6.GB * task.attempt } | |||
There was a problem hiding this comment.
On all memory/cpu requirements, why not simply include base.config? It would be a good opportunity to homogeneise these configurations.
There was a problem hiding this comment.
Similarly, max_retries is only 3 on eddie
There was a problem hiding this comment.
The only thing is the resource allocation needs to be slightly different on the AOU RW, given that they have couple memory-CPU constraints. Which is why these had to be changed here when you run on the AOU RW. I can update the max_retries, but there are additional error codes that need to be added when running on google life sciences API.
There was a problem hiding this comment.
Right I remember, what is the exact coupling, is it a factor? If this ie the case we could probably let the user chose memory only and infer cpu based on it, e.g. cpu = memory / 4 or something similar?
There was a problem hiding this comment.
It seems to be about 6 GB per 1 cpu. So we would have to make the memory a function of the number of CPUs.
For example:
cpu = 1 * task.attempt
memory = 6.GB * cpu
Or something like this?
There was a problem hiding this comment.
Additionally - When CPUs requested is > 2, you must request an even number of cores. Ie. 4, 6, 8 etc. Another odd feature that I found when running the end-to-end test.
There was a problem hiding this comment.
I guess if you wanted the user to choose memory, you could just do the inverse operation. But CPUs would have to be an integer, so something like:
cpu = ceiling(memory/6)
Unsure exactly how this can be implemented at the nextflow configuration level. Would probably have to use the Math() operators.
There was a problem hiding this comment.
It seems to be about 6 GB per 1 cpu. So we would have to make the memory a function of the number of CPUs.
For example:
cpu = 1 * task.attempt memory = 6.GB * cpu
Or something like this?
Then the multithreaded label does not respect this factor does it? I see 8GB for 2 cpus, should it not be 12GB? Any chance you can this info from all of us support team?
There was a problem hiding this comment.
I guess if you wanted the user to choose memory, you could just do the inverse operation. But CPUs would have to be an integer, so something like:
cpu = ceiling(memory/6)
Unsure exactly how this can be implemented at the nextflow configuration level. Would probably have to use the Math() operators.
fair enough!
containers/flashpca/Dockerfile
Outdated
There was a problem hiding this comment.
have you successfully built this image? The problem is that at the moment I would be the only one able to push it to the current docker tag since it is not built within the CI process.
There was a problem hiding this comment.
Yes i've built it and pushed it to my Dockerhub. It is here https://hub.docker.com/r/roskamsh/flashpca
Can we not just keep it as a stale file as it only needs to be done once and not updated again?
There was a problem hiding this comment.
I've already built it so no need to re-build. It is pointed at in the conf/allofus_container.config. I can update container.config to point at the new docker container.
There was a problem hiding this comment.
As you say the file would be unused so that is not ideal. Ideal the repo should be kept as minimal as possible with only useful code. Probably best to use a container for which there is already a build process within the organisation. That's also one less download at runtime, all we need is add a label to the process that misses one.
There was a problem hiding this comment.
In the long run I am happy to have some docker files within the pipeline repo as long as they can be built within the CI process.
There was a problem hiding this comment.
I just thought it would make sense to have the Dockerfile stored here so that people knew how it was built and we had a record of it. Where else would this file be stored then?
There was a problem hiding this comment.
it is definitely the right place for it but as part of another branch --> PR if this is not addressed fully here.
Currently I haven't run it with AOU data yet. That is the plan for this week! Then I should be able to add additional changes (if required) for a new COHORT mode. Once this is complete, I will update the docs. At this point, I have managed to run an end-to-end test using test.config (which is basically just https://github.com/TARGENE/targene-pipeline/blob/main/test/configs/custom_cohort_flat.config). |
| } | ||
|
|
||
| // Set google appropriate error strategy | ||
| errorStrategy = {task.exitStatus in [143,137,104,134,139,14] ? 'retry' : 'finish'} |
There was a problem hiding this comment.
Are the error codes the same as Eddie? Just curious. And is there a place where we can view the docs for the platforms errors?
test/directoryresources:
https://support.researchallofus.org/hc/en-us/articles/21179878475028-Using-Docker-Images-on-the-Workbench
https://workbench.researchallofus.org/workspaces/aou-rw-5b81a011/howtousenextflowintheresearcherworkbenchv7/data