-
Notifications
You must be signed in to change notification settings - Fork 1
config: Set AUGUR_SEARCH_PATHS #68
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: main
Are you sure you want to change the base?
Conversation
snakemake/config.smk
Outdated
| os.getcwd(), | ||
| os.path.join(workflow.basedir, "defaults"), | ||
| ] | ||
| os.environ["AUGUR_SEARCH_PATHS"] = ":".join(search_paths) |
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.
For workflows which may want to customise this, e.g. add in workflow.basedir a workflow could do something like the following?
import: shared/config.smk
os.environ["AUGUR_SEARCH_PATHS"] = os.environ["AUGUR_SEARCH_PATHS"] + ":" + workflow.basedir
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.
Two ways to customize:
- Override defaults by setting
AUGUR_SEARCH_PATHSbefore the import - Extend defaults by prepending or appending after the import (e.g. your snippet)
snakemake/config.smk
Outdated
| # applicable for a static env var. | ||
| search_paths = [ | ||
| os.getcwd(), | ||
| os.path.join(workflow.basedir, "defaults"), |
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.
This won't exist for lots of workflows. I'm not sure what's best -- exclude it (and expect the workflow to update the env variable) or fall back to using workflow.basedir by default?
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.
Another direction is to wrap this entire if/else block in a function set_search_paths and expect all workflows to call it. An extra step for workflows, but given the diversity of workflows we have I think potentially a better one.
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.
We can add workflow.basedir as a search path:
search_paths = [
# User analysis directory
os.getcwd(),
# Workflow defaults folder
os.path.join(workflow.basedir, "defaults"),
# Workflow root (contains Snakefile)
workflow.basedir,
]We can also go further and include the pathogen repo root to cover avian-flu:
search_paths = [ … ]
repo_root = Path(workflow.basedir) / ".."
if (repo_root / "nextstrain-pathogen.yaml").is_file():
search_paths.append(repo_root)These defaults and their order should cover most if not all repos. Outlier repos can customize per #68 (comment).
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.
+1 for adding workflow root and repo root to the default search paths set here.
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.
Done, also updated to use pathlib:
Lines 26 to 42 in cd3def6
| search_paths = [ | |
| # User analysis directory | |
| Path.cwd(), | |
| # Workflow defaults folder | |
| Path(workflow.basedir) / "defaults", | |
| # Workflow root (contains Snakefile) | |
| Path(workflow.basedir), | |
| ] | |
| repo_root = Path(workflow.basedir) / ".." | |
| if (repo_root / "nextstrain-pathogen.yaml").is_file(): | |
| search_paths.extend([ | |
| # Pathogen repo root | |
| repo_root, | |
| ]) |
76e7390 to
973f8e4
Compare
| # Note that this differs from the search paths used in | ||
| # resolve_config_path(). | ||
| # This is the preferred default moving forwards, and the plan is to | ||
| # eventually update resolve_config_path() to use AUGUR_SEARCH_PATHS. |
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.
Here's a draft of what that could look like: nextstrain/avian-flu@dadbde0...290e6a7.
I don't think it should block this PR, but that's what I have in mind. We could discuss at the next dev chat.
To be used by various Augur commands and pathogen workflows, starting with augur subsample in the measles repo.
973f8e4 to
cd3def6
Compare
Description of proposed changes
To be used by various Augur commands and pathogen workflows, starting with augur subsample in the measles repo.
Related issue(s)
Used in nextstrain/measles#90
Checklist
Checks passN/AIf adding a script, add an entry for it in the README.N/A