docs: revise best practices and contributing guide#293
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.
| - 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.
| - e.g. do not use `latest` tags for Docker images. | ||
| - This ensures reproducibility across time and environments. | ||
| - Check all assumptions made about workflow inputs before beginning long running executions. | ||
| - Common examples of assumptions that should be checked: valid `String` choice, mutually exclusive parameters, missing optional file for selected parameters, filename extensions |
There was a problem hiding this comment.
Valid String choice should be replaced with enum in new code. We should probably update this to note that parenthetically.
| - Common examples of assumptions that should be checked: valid `String` choice, mutually exclusive parameters, missing optional file for selected parameters, filename extensions | |
| - Common examples of assumptions that should be checked: valid `String` choice (post-WDL 1.3, an `enum` should be used in place of `String`s with a fixed set of valid options), mutually exclusive parameters, missing optional file for selected parameters, filename extensions |
There was a problem hiding this comment.
Related question, should this document be applicable to all versions of WDL (which may have slightly different best practices), or should we simplify things and assume any "best practice conforming" WDL is going to be in the latest revision of the specification (which for now is 1.3)?
This would be easier to write if we just assumed everything was 1.3, but I also don't have enough experience (nil) writing 1.3 to feel very confident staking my flag in this document if we go that direction 🤔
There was a problem hiding this comment.
I'm not ready to push it to 1.3, since we aren't using it and nothing else supports it. That said I think the document should be written to some form of latest (which, for now, I'd leave at 1.2). I think it's OK to note that something will change in the next version, if nothing else, as a reminder to ourselves to update it. We may want to consider having a section at the end along the lines of "pre-1.3"/"pre-1.2"/etc. that we move old best practices to when they no longer apply to the version we are targeting.
| - Use `after` clauses in workflows to ensure that all these assumptions are valid before beginning tasks with heavy computation. | ||
| - If the _contents_ of a `File` are not read or do not need to be localized for a task, try to coerce the `File` variable to a `Boolean` (with `defined()`) or a `String` (with `basename()`) to avoid unnecessary disk space usage and networking. | ||
| - All requirement values are overridable at runtime. However, tasks should have easily configurable memory and disk space allocations. | ||
| - TODO should this be here? |
There was a problem hiding this comment.
I'd leave it for now. If we're sharing this in the OpenWDL channels, I assume we'll get feedback if people disagree.
There was a problem hiding this comment.
makes sense. I will rephrase it a bit to be clearer in intention (and agnostic of our specific naming conventions)
| - TODO should this be here? | ||
| - 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 | ||
| - TODO should this be here? Will at least need to be rephrased. |
There was a problem hiding this comment.
I think we could delete this as a global best practice.
| - 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
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!