Do we not want these errors to stop the workflow? As these lines are written, the script can fail in many different ways and still return a 0 exit code that allows the workflow to run.

Related to troubleshooting errors, it's easier to track error messages when they are written to sys.stderr like this:

except FileNotFoundError as e:
    print(f"Error reading metadata file: {e}", file=sys.stderr)
except Exception as e:
    print(f"An unexpected error occurred: {e}", file=sys.stderr)

This error handling is something I'm not very familiar with. I intensionally set this up to fail and proceed with workflow. If the remote URL can't be fetched, then

metadata[args.metadata_clade_attribute] = math.nan

still results in a file that's just

    "Scotland/CLIMB-CM7YFURA/2024": {
      "mlr_lineage_fitness": NaN
    },

I'd prefer an empty coloring to workflow that crashes if S3 can't be reached.

I updated to use sys.stderr with 6f5d767.

FWIW, NaN is not a valid JSON value. Python's json module will read/write it by default, but it violates the JSON spec and will cause other parsers to error.

It "works" here because this node data file is only being read by augur export in this case which then suppresses the NaN value as a side-effect of other filtering:

https://github.com/nextstrain/augur/blob/73f1fa94e03775340843a04d63ce97af1dee372f/augur/export_v2.py#L812

but not intentionally because it considers NaN or related values Infinity/-Infinity. (In fact, putting the latter in a node data JSON will cause an error in Augur because it loads the value into a float (against JSON spec) but fails to expect/handle it.)

We really shouldn't be emitting NaN (or Infinity/-Infinity) in JSON outputs.

How do you recommend labeling these instead? "?"? "NaN"? I don't think it's appropriate to just leave these missing entries out of the branch JSON.

You could set them to Python's None (JSON's null) if you want an explicit missing/unknown value. But I'd wonder why not just leave them out? What's the benefit to including all missing values?

I'd prefer an empty coloring to workflow that crashes if S3 can't be reached.

Is the idea that in the face of an error you want the coloring to be present, even if there's no data for it, rather than the coloring to be absent?

Yes. And this is what happens in the current Auspice JSON. It's lacking any entry for mlr_lineage_fitness for Philippines/PH-PGC-52649/2021. This should be due to how augur export is working. However, regarding #1169 (comment), I wonder if this is an issue with write_json that should be documented?

I'm assuming this has to do with something in augur.utils.write_json?

Sigh. Looking at (and quickly testing) write_json(), I don't think it is converting None → NaN, but Pandas may be. It's the sort of thing Pandas does in some cases, e.g.

>>> pd.Series([1, None, 2])
0    1.0
1    NaN
2    2.0
dtype: float64

though I don't know if it applies here without digging into the specifics of this data frame and the series dtypes that were sniffed, and on top of that it may be Pandas-version dependent.

(This sort of behaviour is why I loathe Pandas. Behaviours which are convenient for interactive usage, like in a notebook, are often very hard to reliably reason about for repeated and unattended usage.)

It felt cleaner and more inspectable for the script to explicitly write out each entry and flag the strains that have missing data.

I'm talking about the

else:
    metadata[args.metadata_clade_attribute] = math.nan

case where all the strains having missing data, though. (Am I being thick and missing something?)

I was thinking that this URL fetch would potentially fail transiently when the workflow is run. I didn't want a single failure in one of the 72 Snakemake endpoints to blow up the entire workflow on S3. I'm okay with this (presumably rare) situation resulting in a live dataset that is just completely gray for the MLR fitness coloring.

This felt like the most graceful way for the script to fail.

Ok. The downside is that failure is silent. Our workflows tend to use loud noisy failures, so this is a departure. That's fine, to be clear, but notable. When introducing non-fatal failures that can be ignored/moved past, it's operationally useful to have a way to still surface those as prominent warnings and/or put the monitoring in a "degraded" status. (Historically we've used Slack for the former and don't have the infra in place for the latter.)

In any case, this feels like the weeds for this PR, and I didn't intend to wade into actual code review here. Please do not consider anything I'm saying as a blocker.

Ok. The downside is that failure is silent. Our workflows tend to use loud noisy failures, so this is a departure. That's fine, to be clear, but notable. When introducing non-fatal failures that can be ignored/moved past, it's operationally useful to have a way to still surface those as prominent warnings and/or put the monitoring in a "degraded" status.

This is me not knowing how to use Snakemake. Is there a way for the rule mlr_lineage_fitness to emit an error, still produce the mlr_lineage_fitness.json file and then for the dataset to be produced?

Or I don't understand how to "still surface those as prominent warnings" that is different than

except Exception as e:
	print(f"Error fetching the JSON file: {e}", file=sys.stderr)
	return None

Okay. f088d49 fixes the NaN vs null issue. I didn't realize that this was how Pandas worked. Thanks for the pointer.

@tsibley: Please let me know if you have a specific change you'd recommend for the error / warning issue. I'm confused on what you're currently advocating. Otherwise, I'll plan to merge.

I'm going to go ahead with merging this now. @tsibley: if you have recommended changes I'm happy to make these on a subsequent branch. I'll leave this thread open.