augur subsample: merging of external & default configs

[victorlin]

#103 works fine for the automated Nextstrain builds, but it requires unusual workarounds for external users with custom config. Example on NW-PaGe:

rsv/profiles/wadoh/configfile_wadoh.yaml

Lines 23 to 35 in c364eea

	# FIXME: this should be named 'contextual' but 'all-time' is necessary to override defaults.
	all-time:
	exclude: config/outliers_ppx.txt
	exclude_where:
	- qc.overallStatus=bad
	- qc.overallStatus=mediocre
	group_by:
	- year
	- country
	min_date: 1975-01-01
	max_sequences: 3000
	min_length: 10000
	query: genome_coverage>0.3 & missing_data<1000 & division != 'Washington'

The issue is that subsample.'genome/all-time'.samples.all-time is added by default, so any external config must accept that sample to be part of the final output, or at the least redefine it with custom config (which is what I did above).

This is a config merging issue that technically applies to WNV too, where augur subsample is already used. NW-PaGe/WNV's external config defines a Washington-focused build named wa. The custom name is an unintentional workaround around the issue – wa is not present in the default config, so it does not inherit any build-specific defaults. If NW-PaGe/WNV wants to re-use any of the existing build names from nextstrain/WNV, it would encounter the same config merging issue.

Using a different build name is not a valid workaround for rsv because the workflow is only meant to handle certain build names based on wildcards build_name=genome|G|F and resolution=all-time|6y|3y.

Possible solutions

Options 1-7 were my initial ideas but they aren't great. Option 8 seems to be best so far. Open to any other ideas.

1. Use the current workaround of re-defining existing samples.

   • [con] This wouldn't work if there are more default samples than custom samples to be defined.
   • [con] This merges the existing sample config in, which could be considered a feature but is likely to cause confusing behavior if not overridden.
   • [con] New or renamed default samples would break the workaround.

2. Update augur subsample to ignore "empty" samples.

   • This allows external config to "remove" default samples by overwriting them with null:

     samples:
       all-time: null
       wa: …
       contextual: …

   • [con] New or renamed default samples would break the workaround.

3. Trick Snakemake into removing subsample.genome/all-time.samples.all-time (or even better, all of subsample) for external configs.

   • I've tried various approaches, but haven't found one that works.

4. Move Nextstrain-specific defaults out of the default workflow config file.

   • This was discussed when reworking WNV config, and the decision was to keep them in defaults.

5. Adjust the workflow to allow custom build names.

   • This would allow for the same workaround that NW-PaGe/WNV uses.
   • I haven't tried yet, but my guess is it will complicate the workflow.

6. Separate augur subsample config from Snakemake config.

   • Being considered in Configuring augur subsample in Snakemake workflows public#27
   • This requires additional work to support resolving config paths to workflow analysis directory / defaults directory – necessary for nextstrain run support.

7. Ignore all defaults with Snakemake's --replace-workflow-config.

   • [con] There are some useful defaults. At least in this case, it's mainly subsample that doesn't need to be inherited.

8. Add a custom_subsample config param to override the default subsample config. (ref)

   • [con] Two different config params for the same rule.
   • There's precedent with inputs + additional_inputs.

9. Custom workflow config merging public#28

[joverlee521]

8. Add a custom_subsample config param to override the default subsample config

diff --git a/workflow/snakemake_rules/core.smk b/workflow/snakemake_rules/core.smk
index e1d1319f23..b4fcc4b80b 100644
--- a/workflow/snakemake_rules/core.smk
+++ b/workflow/snakemake_rules/core.smk
@@ -57,7 +57,7 @@
         sequences=build_dir + "/{a_or_b}/{build_name}/{resolution}/filtered.fasta",
     params:
         strain_id=config["strain_id_field"],
-        config_section = lambda w: ["subsample", f"{w.build_name}/{w.resolution}"]
+        config_section = lambda w: ["custom_subsample" if config.get("custom_subsample") else "subsample", f"{w.build_name}/{w.resolution}"]
     log:
         "logs/subsample_{a_or_b}_{build_name}_{resolution}.txt",
     threads: workflow.cores

---

This is an issue we'll continue to run into with merging of dicts in a Snakemake config even outside of augur subsample. I think in the long run we'd want our own way to merge configs outside of Snakemake, but that's a larger project...

[victorlin]

Thanks, that's a good one! Added to the list. It's not great to have two different keys for the same thing, but there's precedent with inputs vs. additional_inputs (in that case it's the opposite problem – list-type keys are overridden entirely).

[tsibley]

I think in the long run we'd want our own way to merge configs outside of Snakemake, but that's a larger project...

This. Doing merging/expanding outside the workflow or taking more control of it inside the workflow.

It's not great to have two different keys for the same thing, but there's precedent with inputs vs. additional_inputs (in that case it's the opposite problem – list-type keys are overridden entirely).

This case of the opposite problem points to the fundamental issue: no single automatic merge routine is going to serve every need equally well. Ways to address this:

• What we're doing now: pick a single consistent merge approach that's Good Enough and apply it globally, even though it causes us problems like this one and others.

• Customize the merge approach used depending on the config context, e.g. when merging subsampling configs, it may be more useful more of the time to override all sample definitions for a build. This is better for the "typical" use cases, but worse for the "unexpected" cases and harder to explain/document to boot.

• Support explicitly denoting in the overlaying config what should happen at each merge level, with some default behaviour based on either approach above. This is more complicated, but provides an escape hatch when the default merging behaviour hurts more than it helps.

I do still think that CUE can help us out here a lot… I may noodle with it.

[victorlin]

I brought this up with Dan from WA DOH in nextstrain/measles#70 (comment) and we settled on using option 8 for measles. This is what he said:

I think it just needs to be made obvious in the documentation that this yaml section needs to be called "custom subsample", so everything remains accessible to the epi who wants to tweak things for their own needs. I can make that apparent in the README.md where we specify how to adapt the NW-PaGe build for use by other states via custom config file.

Maybe this is another +1 for defining a config schema, which could be a basis for documentation that tells users to use custom_subsample instead of subsample.

[jameshadfield]

Noting that this isn't augur subsample specific at all in my eyes, which I think is what Tom was getting at. For instance, take the following config and imagine the user wants to change the behaviour such that the warning banner goes away:

# config structure of RSV
export:
  "<build>":
    warning: "markdown string"

# similar structure used in different repos
export:
  warning:
    "<build>": "markdown string"

They can't do this without knowing the contents of the default config and having some way to nullify the existing values. This will require good documentation. Conversation in this issue seems to be leaning towards implementing custom_<rule> config keys for everything, but that's got sizeable downsides such as having to implement and maintain the remaining default keys/values in the users config. And I don't see how they could do that without good docs either!

[tsibley]

Conversation in this issue seems to be leaning towards implementing custom_<rule> config keys for everything

I think we definitely shouldn't do that!

[victorlin]

custom_subsample is what we have for measles right now, but I am exploring alternatives. Mainly these:

(6) Separate augur subsample config from Snakemake config.

• measles prototype – maintain separate config files per augur subsample invocation (only 2 so it's manageable): nextstrain/measles@43cb2f5
• rsv prototype – maintain a templated config with a script that generates separate config files per augur subsample invocation (this is more scalable but needs extra work for nextstrain run compatibility): f615375

(9) Merge workflow config outside of Snakemake

• measles prototype: nextstrain/measles@ad89480

Out of these, I'm leaning towards the custom script for config merging – open to feedback / more ideas.

[jameshadfield]

Is there something special about subsample cf. other keys? I don't see it differently to the export example I used above. If we do want to special-case it, you could explore removing the wildcard key for subsample like so:

# default config
subsample_scheme:
  all-time: all-time # default

subsample:
  all-time:
    samples:
      exclude: config/outliers_ppx.txt 
      ...
  
# custom config:
subsample_scheme:
  all-time: custom

subsample:
  custom:
    samples:
      min_date: 1975-01-01
      ...