From 578de4e5b86b55cefb663761588bab9e9c6f6855 Mon Sep 17 00:00:00 2001 From: Vin Vomero <36705133+vinvomero@users.noreply.github.com> Date: Mon, 31 Aug 2026 22:34:45 -0400 Subject: [PATCH] fix: guard missing/unresolvable taxonomy in annotate/train + deterministic species selection annotate() and train() passed taxonomy straight into choose_best_busco_species / choose_best_augustus_species, so a False taxonomy (JGI lookup and training-data both unavailable) crashed, and a non-matching taxonomy fell through best_taxonomy's random.choice() tie-break -- the "randomly picks a wrong Augustus species" path behind #60 (whose log shows a valid taxonomy resolving to `augustus --species=elephant_shark`, which then crashes the augustus binary). This is the un-adopted sibling of the merged predict() fix (#94): - add augustus_species_from_taxonomy(), the Augustus analog of busco_lineage_from_taxonomy(); it delegates to choose_best_augustus_species with the full taxonomy (Augustus scores all levels, exact=False) so working inputs are byte-identical, guards falsy/no-overlap taxonomy with a deterministic default, and always returns a valid augustus_species key. - route the busco sites in annotate()/train() through busco_lineage_from_taxonomy(..., default=None) with the same warn+default pattern predict() already uses. - make best_taxonomy's tie-break deterministic (sorted(best_matches)[0] instead of random.choice) so species/lineage selection is reproducible. Extends tests/unit/test_utilities_taxonomy.py with Augustus sibling coverage (falsy/empty -> default, byte-identical delegation on working inputs, deterministic tie-break, default=None sentinel). Addresses #60 (refs #32). Co-Authored-By: Claude Opus 4.8 --- funannotate2/annotate.py | 14 ++- funannotate2/train.py | 33 ++++-- funannotate2/utilities.py | 59 ++++++++++- tests/unit/test_utilities_taxonomy.py | 142 +++++++++++++++++++++++++- 4 files changed, 235 insertions(+), 13 deletions(-) diff --git a/funannotate2/annotate.py b/funannotate2/annotate.py index e5ee0d9..5106fc4 100755 --- a/funannotate2/annotate.py +++ b/funannotate2/annotate.py @@ -37,8 +37,8 @@ swissprot_blast, ) from .utilities import ( + busco_lineage_from_taxonomy, checkfile, - choose_best_busco_species, create_directories, create_tmpdir, ensure_busco_lineage, @@ -361,8 +361,16 @@ def annotate(args): busco_species = args.busco_lineage logger.info(f"Using user-specified BUSCO lineage: {busco_species}") else: - # choose best busco species - busco_species = choose_best_busco_species(taxonomy) + # choose best BUSCO species; guard a missing/unresolvable taxonomy + # (the #93/#94 fix landed in predict(), applied here too) + busco_species = busco_lineage_from_taxonomy(taxonomy, default=None) + if busco_species is None: + busco_species = "fungi" + logger.warning( + f"Could not resolve a BUSCO lineage for species '{args.species}' " + f"(taxonomy unavailable or unrecognized); defaulting to '{busco_species}'. " + "BUSCO completeness scoring will be unreliable for a non-fungal genome." + ) # Ensure the BUSCO lineage is available under FUNANNOTATE2_DB, # downloading it if needed. In dockerized usage the lineage from a # previous train/predict run is gone when annotate starts in a fresh diff --git a/funannotate2/train.py b/funannotate2/train.py index fe2e125..88a8d6b 100755 --- a/funannotate2/train.py +++ b/funannotate2/train.py @@ -20,9 +20,9 @@ from .interlap import InterLap from .log import finishLogging, startLogging, system_info from .utilities import ( + augustus_species_from_taxonomy, + busco_lineage_from_taxonomy, checkfile, - choose_best_augustus_species, - choose_best_busco_species, create_directories, ensure_busco_lineage, lookup_taxonomy, @@ -123,9 +123,20 @@ def train(args): aug_species = args.augustus_species logger.info(f"Using user-specified Augustus species: {aug_species}") else: - # choose best augustus species based on taxonomy - aug_species = choose_best_augustus_species(taxonomy) - logger.info(f"Choosing best augustus species based on taxonomy: {aug_species}") + # choose best Augustus species; guard a missing/unresolvable taxonomy + # (the #93/#94 taxonomy-fallback fix, applied to the Augustus path -- #60) + aug_species = augustus_species_from_taxonomy(taxonomy, default=None) + if aug_species is None: + aug_species = "aspergillus_fumigatus" + logger.warning( + f"Could not resolve an Augustus species for '{args.species}' " + f"(taxonomy unavailable or unrecognized); defaulting to '{aug_species}'. " + "Ab initio gene predictions will be unreliable for a non-fungal genome." + ) + else: + logger.info( + f"Choosing best Augustus species based on taxonomy: {aug_species}" + ) # validate and set busco lineage if args.busco_lineage: @@ -138,8 +149,16 @@ def train(args): busco_species = args.busco_lineage logger.info(f"Using user-specified BUSCO lineage: {busco_species}") else: - # choose best busco species - busco_species = choose_best_busco_species(taxonomy) + # choose best BUSCO species; guard a missing/unresolvable taxonomy + # (the #93/#94 fix landed in predict(), applied here too) + busco_species = busco_lineage_from_taxonomy(taxonomy, default=None) + if busco_species is None: + busco_species = "fungi" + logger.warning( + f"Could not resolve a BUSCO lineage for species '{args.species}' " + f"(taxonomy unavailable or unrecognized); defaulting to '{busco_species}'. " + "BUSCO completeness scoring will be unreliable for a non-fungal genome." + ) # Ensure the BUSCO lineage exists under FUNANNOTATE2_DB (downloading # it if necessary) and capture its on-disk path for buscolite + the # params.json output below. diff --git a/funannotate2/utilities.py b/funannotate2/utilities.py index b4a9d54..1eed8c6 100755 --- a/funannotate2/utilities.py +++ b/funannotate2/utilities.py @@ -2,7 +2,6 @@ import multiprocessing import os import queue -import random import re import shutil import signal @@ -544,6 +543,56 @@ def _overlaps(level): return lineage if lineage in busco_taxonomy else default +def augustus_species_from_taxonomy(taxonomy, default="aspergillus_fumigatus"): + """ + Resolve a valid Augustus species from a taxonomy dict, deterministically. + + The Augustus sibling of `busco_lineage_from_taxonomy`. `annotate` and `train` + get taxonomy from `lookup_taxonomy` (which returns False when the JGI lookup + fails) and fall back to the training-data taxonomy (also possibly False). + Passing such a value straight into `choose_best_augustus_species` either + crashed (`best_taxonomy` iterates the query) or, for a non-empty but unmatched + taxonomy, returned a random species via `best_taxonomy`'s tie-break. This + helper guards both cases and always returns a species present in + `augustus_species`. + + Unlike `busco_lineage_from_taxonomy` (which uses `exact=True` and a reduced + superkingdom/kingdom query), Augustus resolution scores across all taxonomic + levels, so the FULL taxonomy is delegated to `choose_best_augustus_species` to + keep working inputs byte-identical; the superkingdom/kingdom overlap check is + used only to decide whether to trust the matcher or return the default. + + Parameters: + - taxonomy (dict or None): a taxonomy dict, or a falsy value when unavailable. + - default (str): species to fall back to; must be a valid augustus_species key. + + Returns: + - str: an Augustus species guaranteed to be present in augustus_species. + """ + if not isinstance(taxonomy, dict) or not taxonomy: + return default + + # best_taxonomy falls back to a tie-break when nothing matches, and that pick + # is always itself a valid augustus_species key -- so a plain "result not in + # augustus_species" check can never catch a spurious match. Only trust the + # matcher when the taxonomy overlaps the reference on superkingdom or kingdom; + # otherwise return the default deterministically. + def _overlaps(level): + value = taxonomy.get(level) + if not isinstance(value, str): + return False + value = value.lower() + return any( + isinstance(ref.get(level), str) and ref[level].lower() == value + for ref in augustus_species.values() + ) + + if not (_overlaps("superkingdom") or _overlaps("kingdom")): + return default + species = choose_best_augustus_species(taxonomy) + return species if species in augustus_species else default + + def best_taxonomy(query, reference, exact=False): """ Find the best matching taxonomy in a reference dictionary based on a query taxonomy. @@ -632,7 +681,13 @@ def similarity_score(query, ref): best_matches = [name] elif score == highest_score: best_matches.append(name) - return random.choice(best_matches) if best_matches else None + # Deterministic tie-break (was random.choice): when several references share the + # top score, pick the case-insensitively-first key so species/lineage selection + # is reproducible run-to-run. `key=str.lower` avoids a lexical bias toward the + # few capitalized reference keys (e.g. an ASCII sort would hand every tie to + # "Xiphophorus_maculatus"). A non-deterministic pick here is the mechanism behind + # issue #60 (a random -- sometimes unrunnable -- Augustus species being chosen). + return sorted(best_matches, key=str.lower)[0] if best_matches else None def which_path(file_name): diff --git a/tests/unit/test_utilities_taxonomy.py b/tests/unit/test_utilities_taxonomy.py index d553550..07bb129 100644 --- a/tests/unit/test_utilities_taxonomy.py +++ b/tests/unit/test_utilities_taxonomy.py @@ -6,10 +6,12 @@ import funannotate2.utilities from funannotate2.utilities import ( + augustus_species_from_taxonomy, busco_lineage_from_taxonomy, + choose_best_augustus_species, choose_best_busco_species, ) -from funannotate2.config import busco_taxonomy +from funannotate2.config import augustus_species, busco_taxonomy @patch("funannotate2.utilities.pretty_taxonomy") @@ -467,3 +469,141 @@ def test_non_string_values_do_not_crash(self): {"superkingdom": "", "kingdom": ""}, ]: assert busco_lineage_from_taxonomy(tax) in busco_taxonomy + + +class TestAugustusSpeciesFromTaxonomy: + """Tests for augustus_species_from_taxonomy. + + The Augustus sibling of busco_lineage_from_taxonomy (#93/#94): `annotate` and + `train` fed taxonomy straight into `choose_best_augustus_species`, which + crashed on a `False` taxonomy and, for a non-empty taxonomy, resolved through + `best_taxonomy`'s random tie-break -- the "randomly picks a wrong Augustus + species" path behind issue #60. This helper guards both cases and always + returns a species present in `augustus_species`. Unlike the busco path + (`exact=True`, reduced query), Augustus scores across all levels, so the helper + delegates to `choose_best_augustus_species` with the FULL taxonomy to keep + working inputs byte-identical. + """ + + def test_false_returns_default_without_crashing(self): + # #60 sibling: a `False` taxonomy previously reached best_taxonomy and + # raised `TypeError: argument of type 'bool' is not iterable`. + assert augustus_species_from_taxonomy(False) == "aspergillus_fumigatus" + + def test_none_returns_default(self): + assert augustus_species_from_taxonomy(None) == "aspergillus_fumigatus" + + def test_empty_dict_returns_default(self): + assert augustus_species_from_taxonomy({}) == "aspergillus_fumigatus" + + def test_overlap_delegates_to_choose_best_augustus_species(self): + # On a working (overlapping) input the helper must return EXACTLY what + # choose_best_augustus_species returns -- byte-identical selection (R1), + # proving the helper did not silently coarsen the query the way the busco + # path does. Apis mellifera resolves uniquely to 'honeybee1' (no tie). + tax = { + "superkingdom": "Eukaryota", + "kingdom": "Metazoa", + "phylum": "Arthropoda", + "class": "Insecta", + "order": "Hymenoptera", + "family": "Apidae", + "genus": "Apis", + "species": "Apis mellifera", + } + assert augustus_species_from_taxonomy(tax) == choose_best_augustus_species(tax) + assert augustus_species_from_taxonomy(tax) in augustus_species + + def test_overlapping_tie_is_deterministic(self): + # {"superkingdom": "Eukaryota"} alone ties every eukaryotic Augustus + # species at score 1, so best_taxonomy's tie-break decides the pick -- it + # must be deterministic, not random.choice() (the #60 lever). + results = { + augustus_species_from_taxonomy({"superkingdom": "Eukaryota"}) + for _ in range(50) + } + assert len(results) == 1, f"non-deterministic Augustus species: {results}" + assert results.pop() in augustus_species + + def test_choose_best_augustus_species_tie_is_deterministic(self): + # Direct lock on the shared best_taxonomy tie-break, independent of the + # helper: the same tie-inducing query must resolve stably across runs. + results = { + choose_best_augustus_species({"superkingdom": "Eukaryota"}) + for _ in range(50) + } + assert len(results) == 1, f"non-deterministic tie-break: {results}" + + def test_non_matching_dict_is_deterministic_default(self): + # No overlap with any Augustus species (all are Eukaryota) -> deterministic + # default, never a random pick. + results = { + augustus_species_from_taxonomy( + {"superkingdom": "Bacteria", "kingdom": "Nonexistent"} + ) + for _ in range(50) + } + assert results == {"aspergillus_fumigatus"} + + def test_default_none_signals_unresolved(self): + # annotate()/train() pass default=None to detect when to warn + fall back. + assert augustus_species_from_taxonomy(False, default=None) is None + assert ( + augustus_species_from_taxonomy( + {"superkingdom": "Bacteria", "kingdom": "Nonexistent"}, default=None + ) + is None + ) + assert ( + augustus_species_from_taxonomy( + {"superkingdom": "Eukaryota", "kingdom": "Metazoa"}, default=None + ) + in augustus_species + ) + + def test_default_is_overridable(self): + assert ( + augustus_species_from_taxonomy( + False, default="saccharomyces_cerevisiae_S288C" + ) + == "saccharomyces_cerevisiae_S288C" + ) + + def test_result_is_always_a_valid_species(self): + for tax in [ + False, + None, + {}, + {"superkingdom": "Eukaryota", "kingdom": "Metazoa"}, + {"superkingdom": "Eukaryota"}, + {"superkingdom": "Bacteria", "kingdom": "Nonexistent"}, + ]: + assert augustus_species_from_taxonomy(tax) in augustus_species + + def test_non_string_values_do_not_crash(self): + for tax in [ + {"superkingdom": 123, "kingdom": ["Metazoa"]}, + {"superkingdom": {"x": 1}, "kingdom": None}, + {"superkingdom": "", "kingdom": ""}, + ]: + assert augustus_species_from_taxonomy(tax) in augustus_species + + def test_deep_only_taxonomy_hits_the_gate_and_defaults(self): + # A taxonomy overlapping augustus_species only BELOW kingdom (no + # superkingdom/kingdom) is rejected by the overlap gate and returns the + # deterministic default, not a lower-confidence deeper-level guess. Real + # lookups always populate superkingdom, so this only affects malformed + # partial taxonomies -- the deliberate safe behavior. + tax = {"phylum": "Arthropoda", "class": "Insecta", "genus": "Apis"} + assert augustus_species_from_taxonomy(tax) == "aspergillus_fumigatus" + + def test_tie_break_is_case_insensitive(self): + # {"superkingdom": "Eukaryota"} ties every eukaryotic species; the pick + # must not be biased toward the capitalized reference keys by an ASCII sort + # (e.g. "Xiphophorus_maculatus"). Case-insensitive ordering keeps it stable + # and unbiased. + result = augustus_species_from_taxonomy({"superkingdom": "Eukaryota"}) + assert result in augustus_species + assert result == result.lower(), ( + f"tie resolved to a capitalized oddball key: {result}" + )