fix: don't error if node data file is empty

[corneliusroemer]

Resolves #1215

Partial revert of (over) eager validation introduced recently through PR #728

Description of proposed changes

In PR #728, extra node data validation was introduced. In particular, files without information for either nodes or branches caused erroring.

This is problematic for test scripts that may produce empty node data in test cases.

This PR removes the eager validation. In the future we could reintroduce it as a warning.
And possibly an error but with opt-out.

This type of node data json was previously errored on by augur export, it is now accepted again:

{
  "nodes": {},
  "rbd_level_details": {}
}

Related issue(s)

Fixes the ncov pathogen-CI issue: nextstrain/conda-base#27 (comment)

Testing

What steps should be taken to test the changes you've proposed?
If you added or changed behavior in the codebase, did you update the tests, or do you need help with this?

• ci: Test pathogen repo CI builds with the final packages conda-base#27 (comment) is fixed, export now accepts empty nodes dicts again

Checklist

• Add a message in CHANGES.md summarizing the changes in this PR that are end user focused. Keep headers and formatting consistent with the rest of the file.

[huddlej]

Thank you for checking into this, @corneliusroemer! Looking at the original commit that added the validation check, @jameshadfield's commit message says:

A side-effect of this work is that the requirement for node-data JSONs
to specify "nodes" has been relaxed (see [2] for an example); however
if neither "nodes" nor "branches" are defined then we raise a validation
error.

Based on this description, it seems the intention was the observed behavior where "nodes" and "branches" cannot both be empty. I agree though that it is possible and even common in some workflows to produce node data JSONs with no annotations.

If that is really the behavior we want as a default, then instead of removing the validation check, we might move the check into the conditional block below where the other validation logic lives. With some slight reorganization of that block, we could use the existing validation mode interface to allow this validation to be skipped.

[rneher]

I think this shouldn't be an error. I can think of scenarios where you annotate specific branches or tips with rare traits that are sometimes present, sometimes not. For example drug resistance mutations, insertions, etc. But I see how empty branches and nodes could also be flag that something went wrong. But if we don't require EVERY node or branch to be present, I don't think we should error when NONE are present.

Instead of whole-sale deletion of the check, or making it conditional, maybe we can just provide a warning like here:

augur/augur/util_support/node_data_file.py

Line 93 in 99f1711

print_err(f"WARNING: {msg}")

[corneliusroemer]

Given that this is a surprising breaking change that wasn't announced in the changelog, I'd suggest we merge this PR asap and make a bug fix release.

This doesn't prejudice how we may want to handle this in the future. I'd be fine with warning and/or making it part of export validation, but I don't think we need to agree on the path forward to make sure we don't break things in unexpected ways.

[rneher]

why not make it warning now? that would be the minimal change.

[codecov]

Codecov Report

Patch coverage: 66.66% and project coverage change: -0.01 ⚠️

Comparison is base (aade8fd) 68.81% compared to head (6f96100) 68.81%.

❗ Current head 6f96100 differs from pull request most recent head b745538. Consider uploading reports for the commit b745538 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1214      +/-   ##
==========================================
- Coverage   68.81%   68.81%   -0.01%     
==========================================
  Files          64       64              
  Lines        6936     6935       -1     
  Branches     1692     1692              
==========================================
- Hits         4773     4772       -1     
  Misses       1856     1856              
  Partials      307      307              

Impacted Files	Coverage Δ
augur/util_support/node_data_file.py	85.45% <66.66%> (-0.26%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[rneher]

would be good to get input form James, but this looks good to me (pending update of the PR description and changelog)

[corneliusroemer]

@rneher I've added a changelog entry, so should be good to go from my end

[jameshadfield]

A warning is absolutely fine. There may have been a bug in my implementation, I wanted to allow empty dictionaries, but error when both dictionaries were missing from the JSON.

[corneliusroemer]

It works in ncov pathogen CI now: https://github.com/nextstrain/augur/actions/runs/4985866782/jobs/8925984486

[jameshadfield]

Specifically, my PR should have had something like:

- if not self.nodes and not self.branches:
+ if self.attrs.get("branches") == None and self.attrs.get("nodes") == None:
     raise AugurError(
         f"{self.fname} did not contain either `nodes` or `branches`. Please check the formatting of this JSON!"
      )

But it's probably fine as-is -- we'll now get a warning, and maybe intermediates without nodes and without branches are normal behavior in some workflows.