-
Notifications
You must be signed in to change notification settings - Fork 20
pathogens: Define workflows in top level key of nextstrain-pathogen.yaml
#472
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
Conversation
d117abe to
e701c24
Compare
Workflow changes to support running the workflows with the `nextstrain run` command. I specifically chose _not_ to declare compatibility for `nextstrain run` in the `nextstrain-pathogen.yaml` because the phylogenetic and nextclade workflows are incomplete in this guide. Once workflow specific compatibility is supported,¹ we can declare compatibility for the ingest workflow. ¹ <nextstrain/cli#472>
tsibley
left a 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.
I continue to +1 for the change in nextstrain-pathogen.yaml! My comments here are almost all around the way workflows are exposed in Nextstrain CLI. Happy to discuss my suggestions if you want to consider them. But also, if you are happy not to care about the details, I can make the changes as I see fit instead of going back and forth here.
Dropping based on feedback #472 (comment)
Declutter the version output for pathogen worklfows but still keeping a simple list of pathogen workflow names for discoverability. Based on feedback <#472 (comment)>
Depends on merge and release of nextstrain/cli#472
Depends on merge and release of nextstrain/cli#472
Depends on merge and release of nextstrain/cli#472
…run` compatibility
Workflows are now registered under a top level "workflows" key in the
nextstrain-pathogen.yaml file, and each workflow has its own
"compatibility" declarations. The only current "compatibility"
declaration remains "nextstrain run".
This allows for more meta information about each workflow to be
registered in one place not under the banner of compatibility.
The nextstrain-pathogen.yaml schema changes to a workflow-first
structure like:
workflows:
ingest:
compatibility:
nextstrain run: yes
phylogenetic:
compatibility:
nextstrain run: no
instead of a compatibility-first structure like:
compatibility:
nextstrain run:
ingest: yes
phylogenetic: no
as previously introduced¹ (but not yet released). As it was never
released, don't bother supporting the latter schema at all anymore.
Change first arose out of discussion started by @victorlin in a related
PR.² Subsequent discussion with @tsibley³ led to dropping support for
the unreleased schema.
¹ <#461>
² <#462 (comment)>
³ <#472 (comment)>
Co-authored-by: Thomas Sibley <tsibley@fredhutch.org>
List the workflow names on the same line as the pathogen name@version when there are any workflows and emit nothing when there aren't. This fits more in the style of the rest of `nextstrain version` and is much less cluttered (more information dense), particularly considering that we, in short order too, expect ~all of our pathogens (and all their future versions) to have the same 2–3 workflows listed (nextclade, ingest, phylogenetic). Prompted by feedback from @tsibley.¹ ¹ <#472 (comment)> Co-authored-by: Thomas Sibley <tsibley@fredhutch.org>
fad921c to
8e98a95
Compare
…run` compatibility
Workflows are now registered under a top level "workflows" key in the
nextstrain-pathogen.yaml file, and each workflow has its own
"compatibility" declarations. The only current "compatibility"
declaration remains "nextstrain run".
This allows for more meta information about each workflow to be
registered in one place not under the banner of compatibility.
The nextstrain-pathogen.yaml schema changes to a workflow-first
structure like:
workflows:
ingest:
compatibility:
nextstrain run: yes
phylogenetic:
compatibility:
nextstrain run: no
instead of a compatibility-first structure like:
compatibility:
nextstrain run:
ingest: yes
phylogenetic: no
as previously introduced¹ (but not yet released). As it was never
released, don't bother supporting the latter schema at all anymore.
Change first arose out of discussion started by @victorlin in a related
PR.² Subsequent discussion with @tsibley³ led to dropping support for
the unreleased schema.
¹ <#461>
² <#462 (comment)>
³ <#472 (comment)>
Co-authored-by: Thomas Sibley <tsibley@fredhutch.org>
List the workflow names on the same line as the pathogen name@version when there are any workflows and emit nothing when there aren't. This fits more in the style of the rest of `nextstrain version` and is much less cluttered (more information dense), particularly considering that we, in short order too, expect ~all of our pathogens (and all their future versions) to have the same 2–3 workflows listed (nextclade, ingest, phylogenetic). Prompted by feedback from @tsibley.¹ ¹ <#472 (comment)> Co-authored-by: Thomas Sibley <tsibley@fredhutch.org>
|
Repushed with tweaks to the code and commit messages. |
|
I'll merge once I get tests to pass… |
…run` compatibility
Workflows are now registered under a top level "workflows" key in the
nextstrain-pathogen.yaml file, and each workflow has its own
"compatibility" declarations. The only current "compatibility"
declaration remains "nextstrain run".
This allows for more meta information about each workflow to be
registered in one place not under the banner of compatibility.
The nextstrain-pathogen.yaml schema changes to a workflow-first
structure like:
workflows:
ingest:
compatibility:
nextstrain run: yes
phylogenetic:
compatibility:
nextstrain run: no
instead of a compatibility-first structure like:
compatibility:
nextstrain run:
ingest: yes
phylogenetic: no
as previously introduced¹ (but not yet released). As it was never
released, don't bother supporting the latter schema at all anymore.
Change first arose out of discussion started by @victorlin in a related
PR.² Subsequent discussion with @tsibley³ led to dropping support for
the unreleased schema.
¹ <#461>
² <#462 (comment)>
³ <#472 (comment)>
Co-authored-by: Thomas Sibley <tsibley@fredhutch.org>
List the workflow names on the same line as the pathogen name@version when there are any workflows and emit nothing when there aren't. This fits more in the style of the rest of `nextstrain version` and is much less cluttered (more information dense), particularly considering that we, in short order too, expect ~all of our pathogens (and all their future versions) to have the same 2–3 workflows listed (nextclade, ingest, phylogenetic). Prompted by feedback from @tsibley.¹ ¹ <#472 (comment)> Co-authored-by: Thomas Sibley <tsibley@fredhutch.org>
Helpful to ensure that their directories exist as expected to catch registration vs. reality mismatches.
8e98a95 to
20c99d4
Compare
Depends on merge and release of nextstrain/cli#472
Depends on merge and release of nextstrain/cli#472
Workflow changes to support running the workflows with the `nextstrain run` command. I specifically chose _not_ to declare compatibility for `nextstrain run` in the `nextstrain-pathogen.yaml` because the phylogenetic and nextclade workflows are incomplete in this guide. Once workflow specific compatibility is supported,¹ we can declare compatibility for the ingest workflow. ¹ <nextstrain/cli#472>
Description of proposed changes
Originally implemented as part of #462. Pulled out as a separate PR since other changes in the original PR are being discussed.
NOTE: This is changing the schema for the workflow registration originally added (but not yet released) in #461.
List workflows under a top level
workflowskey in thenextstrain-pathogen.yamlfile where each workflow has it's owncompatibilitykey:The top levelDropped support for top level compatibility declaration as discussed.compatibility['nextstrain run']boolean is still supported for backwards compatibility.Checklist
Post-merge/release