-
Notifications
You must be signed in to change notification settings - Fork 23
(WIP) phylogenetic: Support nextstrain run
#327
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| configfile: os.path.join(workflow.basedir, "../defaults/mpxv/config.yaml") | ||
|
|
||
|
|
||
| if os.path.exists("config.yaml"): | ||
|
|
||
| configfile: "config.yaml" | ||
|
|
||
|
|
||
| include: "../rules/main.smk" | ||
|
|
||
|
|
||
| rule _all: | ||
| input: | ||
| rules.all.input, |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| configfile: os.path.join(workflow.basedir, "../defaults/clade-i/config.yaml") | ||
|
|
||
|
|
||
| if os.path.exists("config.yaml"): | ||
|
|
||
| configfile: "config.yaml" | ||
|
|
||
|
|
||
| include: "../rules/main.smk" | ||
|
|
||
|
|
||
| rule _all: | ||
| input: | ||
| rules.all.input, |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| configfile: os.path.join(workflow.basedir, "../defaults/hmpxv1/config.yaml") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's cleaner and easier to understand if we co-locate the $ tree phylogenetic/clade-iib
clade-iib
├── Snakefile
└── config.yamlThis raises the question of where to store clade-iib specific config files: color_scheme: "color_schemes.tsv"
auspice_config: "clade-iib/auspice_config.json"which isn't very ergonomic from an external users point of view. I think something like the following is cleaner: color_scheme: "color_schemes.tsv"
auspice_config: "auspice_config.json"$ tree
phylogenetic
├── defaults
│ └── color_schemes.tsv
├── clade-iib
│ ├── Snakefile
│ ├── config.yaml
│ ├── defaults # maybe use a defaults subdir?
│ │ └── auspice_config.json
│ └── auspice_config.json # or maybe just do this?Where you have a path resolution of (1) analysis dir, (2) clade-iib, (3) phylogenetic.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, totally see your point about the config not being ergonomic for external users here. On the other hand, as a maintainer, it's no longer clear from color_scheme: "color_schemes.tsv"
auspice_config: "auspice_config.json"which files need to be edited to change the config...I am also wary of the splintering of config files, which is making me reconsider the separate Snakefile approach. See proposal in nextstrain/cli#454
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for exploring ways to get different configfile "entrypoints" into Even with the |
||
|
|
||
|
|
||
| if os.path.exists("config.yaml"): | ||
|
|
||
| configfile: "config.yaml" | ||
|
|
||
|
|
||
| include: "../rules/main.smk" | ||
|
|
||
|
|
||
| rule _all: | ||
| input: | ||
| rules.all.input, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| configfile: os.path.join(workflow.basedir, "../defaults/hmpxv1_big/config.yaml") | ||
|
|
||
|
|
||
| if os.path.exists("config.yaml"): | ||
|
|
||
| configfile: "config.yaml" | ||
|
|
||
|
|
||
| include: "../rules/main.smk" | ||
|
|
||
|
|
||
| rule _all: | ||
| input: | ||
| rules.all.input, |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,23 @@ from textwrap import dedent, indent | |
| from typing import Union | ||
|
|
||
|
|
||
| include: "../../shared/vendored/snakemake/config.smk" | ||
|
|
||
|
|
||
| def phylo_resolve_config_path(path: str) -> Callable[[Wildcards], str]: | ||
| """ | ||
| Wrapper around the shared `resolve_config_path` to force the default directory | ||
| to be `phylogenetic/defaults`. This is necessary because the entry point for | ||
| each build is nested within phylogenetic (e.g. phylogenetic/clade-i/Snakefile). | ||
| """ | ||
| PHYLO_DEFAULTS_DIR = os.path.normpath(os.path.join(workflow.current_basedir, "../defaults")) | ||
| # Strip the `defaults/` prefix to be backwards compatible with older configs | ||
| # This is necessary in this wrapper because we are providing a custom defaults dir | ||
| # which skips the handling of the defaults/ prefix within resolve_config_path. | ||
| path = path.removeprefix("defaults/") | ||
| return resolve_config_path(path, PHYLO_DEFAULTS_DIR) | ||
|
|
||
|
|
||
|
Comment on lines
+10
to
+26
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still wish we got rid of all this magic around Secondly I still think include: "../../shared/vendored/snakemake/config.smk"
_shared_resolve_config_path = resolve_config_path
def resolve_config_path(path: str) -> Callable[[Wildcards], str]:
return _shared_resolve_config_path(...)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is only necessary because of the nested Snakefiles. If we keep the main |
||
| def as_list(config_param: Union[list,str]) -> list: | ||
| if isinstance(config_param, list): | ||
| return config_param | ||
|
|
||
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.
If we're going to rename hmpxv1 to clade-iib (which seems fine 🤷 I don't have much context here) then we should go the whole way and rename everything -
defaults/hmpxv1etc. (Perhaps you plan to do this -- it's a draft PR after all -- if so this can just serve as a reminder)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.
Users in office hours and myself have often been confused by the mapping between the config files and the named builds, so I thought the workflow should at least match the build name.
If there's not push back from others here, I'll rename everything.