docs: revise best practices and contributing guide#293
Conversation
a-frantz
changed the title
maybe something else should go there?
| - See `template/common-parameter-meta.txt` for common description strings. | ||
| - If applicable, use the same parameter name and help text as the underlying tool called by the task. |
There was a problem hiding this comment.
Not at all related to this PR, but I wonder if there is a way to tell Copilot about this file. It'd be great if its autocomplete suggestions would consider that file as a set of options when writing parameter_meta.
There was a problem hiding this comment.
I'm skimming this now, but there are instructions for doing something that sounds like this feature: https://docs.github.com/en/copilot/how-tos/configure-custom-instructions/add-repository-instructions?tool=vscode
| - Contributors can mix and match the available templates, copy and pasting subsections as appropriate. | ||
| - It is allowed to have one resource allocated dynamically, and another allocated statically in the same task. | ||
| - Multi-core tasks should *always* follow the conventions laid out in the `use_all_cores_task` example (see `template/task-examples.wdl`). | ||
| - This is catering to cloud users, who may be allocated a machine with more cores than are specified by the `ncpu` parameter. |
There was a problem hiding this comment.
We may want to revisit this paradigm in the future. With Planetary + AKS, the container only gets allocated the requested number of cores within the VM.
There was a problem hiding this comment.
Q you may not have the answer to: how does nproc work in those environments? I think that nproc on the cluster is "smart" and only returns what was allocated.
There was a problem hiding this comment.
It does. With Planetary + AKS, the container is allocated n CPUs, and it can only see that many. I'm not actually confident that the idea behind nproc is sound. As long as the task is running in a container, the Docker engine will have provided some number of cores to the container. Presumably, whatever engine is queueing the Docker container will provide the cpu argument to Docker as the number of cores. The use_all_cores idea is probably better implemented at the engine-level. If the engine allocates 1 VM per task, then it should use_all_cores. If it is capable of sharing VMs between tasks, then it shouldn't use_all_cores.
There was a problem hiding this comment.
the point of use_all_cores is so that you can tell called tools what level of parallelism should be used. Most bioinformatics tools will only spin up as many threads as you ask for (i.e. without specifying some kind of --thread [>1 value] everything will run in one thread regardless of how many cores the task/engine/backend/VM allocates.
There was a problem hiding this comment.
Yeah, I meant more that the engine, itself, should set use_all_cores if it isn't capable of scheduling multiple tasks to a node.
With our experience with LSF though, I'm curious how other HPC system function (e.g. Slurm). LSF does something so that the underlying task only sees the allocated cores. If the scheduling systems reliably did that, then we could just always use_all_cores.
There was a problem hiding this comment.
Yeah, I meant more that the engine, itself, should set use_all_cores if it isn't capable of scheduling multiple tasks to a node.
this stumped me for a minute. I don't like the idea of the engine messing around in the inputs section, but that wouldn't be needed. 1.3 exposes both hints and requirements to command blocks, which is a paradigm shift I haven't adjusted to yet. I can see something analagous to use_all_cores being a hint that can be accessed in the command in order to determine how nproc should (or should not be) called
best-practices.md
Outdated
| - The non-empty qualifier (`+`) of arrays and maps should be avoided. | ||
| - TODO this is really just our opinion after trying to use it and finding the resulting WDL ugly. I'm not sure it can really be called a "best practice". | ||
| - This is because the non-empty qualifier can be cumbersome to deal with in WDL source code. No newline at end of file |
There was a problem hiding this comment.
I don't actually recall what led to writing this. I'm not sure we should recommend skipping use of a WDL language feature though as a best practice. If anything, the spec should be improved around the feature to improve the user experience.
There was a problem hiding this comment.
We briefly had Array[*]+ on main before we had some issue with sprocket+miniwdl compatibility that made us go back and revert them all (and write this entry), although I now can't remember the specifics. It should be in our git history though, will just take some digging to find
There was a problem hiding this comment.
removed for now
If anything, the spec should be improved around the feature to improve the user experience.
this is true. I'm going to leave this thread unresolved, as we should go find that git history and share the pain points we ran into and file issues in OpenWDL for ways to improve the spec
Co-authored-by: Andrew Thrasher <adthrasher@gmail.com>
Co-authored-by: Andrew Thrasher <adthrasher@gmail.com>
2 participants
Describe the problem or feature in addition to a link to the issues.
closes #277
Before submitting this PR, please make sure:
scripts/ordocker/directories, please ensure any image versions have been incremented accordingly!