feat: add duplicate-key QC gate to qcModelChecks#1056
Merged
Conversation
A repeated key inside a metabolite/reaction/gene !!omap block (e.g. two name fields on one reaction, as happened with MAR01801) is tolerated by RAVEN's YAML reader/writer but makes cobra.io.load_yaml_model raise a bare AssertionError with no location, failing the QC and MEMOTE jobs opaquely. Add check_no_duplicate_keys, a PyYAML event scan that runs before the cobra load and fails the build with a clear message naming the entry, key and line numbers. Covers both duplicate fields and duplicate stoichiometry metabolites. Install pyyaml in the model-qc checks job.
Model quality report
Structural checksDuplicate keys (model unloadable) and no growth block the merge; the other rows are non-blocking.
Model QC reports
MACAW and mass/charge balance
Gene essentiality (Hart 2015)
❌ = a count rose vs the target branch (regression) · Results for commit ec1e12d. Full workflow run · this comment is edited on subsequent runs |
…and MEMOTE detail (#1056) - buildReport.py: render the MACAW and mass/charge balance findings (dead-end, duplicate, and mass- and charge-imbalanced reaction counts) as a status table with the value, the change versus the target branch and an icon, matching the Model QC checks table, instead of embedding the prose summary. A result set not committed yet shows an hourglass. - Fetch macaw_results.csv and balance_results.csv from the target branch in both QC workflows so the new table can show a delta. - README test-results list: give each result set its own line, updated by its own workflow (model QC checks, MEMOTE, MACAW and mass/charge balance, gene essentiality). The template carried two "(QC)" lines that the qc-tests sed both rewrote, and had no MEMOTE or MACAW entry; the Model QC checks and memote jobs now each update their own line. - memoteSnapshot.py: add a per-test detailed-score table grouped by section, in addition to the section summary.
3 tasks
#1056) The adversarial review surfaced two gaps: the checks that block a merge (duplicate/empty/inconsistent reactions, growth) left their findings only in the workflow log, and the pull-request comment covered only the soft report checks, so it could read all-green while a required check was red. This reworks the pipeline so every failing check has a committed detail file and the comment shows a clear problem-or-not verdict with a delta vs the target branch. - Consolidate the structural checks into code/test/qcModelChecks.py: gates (duplicate !!omap keys, reactions with no metabolites, model vs annotation-table consistency incl. deprecated ids and the spontaneous column, growth) and reports (metabolite completeness, reaction sanity, exact-duplicate reactions, unused entities), each writing a detailed CSV. On a growth failure it writes the blocking biomass precursors. - buildReport.py: a one-line verdict (merge blocked / regressions / running / clean), a Build gates table, a Model QC reports table and the MACAW/balance table, each value linked to its CSV with a delta and an icon. Each result set is stamped with the head commit (qc_checks/memote/macaw.sha) so a set that has not re-run for the current commit shows as pending, not stale. - The Model QC checks job commits the detail and posts the comment even when a gate fails, then fails the build, so the failure is visible in both. - Remove the now-duplicated memoteTest.py and sanityCheck.py; trim yaml-validation to the YAML lint. Guard the MACAW step so a crash no longer skips the balance report and comment; balanceTest.py records reactions it cannot check instead of silently dropping them. Branch protection should require the "Model QC checks / checks" job (which now owns the structural gates) in place of "yaml-validation".
The 7 formula-less metabolites (MAM10001-3 generic class sinks; MAM10012-15 *_pool_biomass) intrinsically have no molecular formula, so flagging them as "missing formula" is noise. Exclude them via an explicit documented set - not the MAM10xxx id range, which also holds real metabolites (bile acids, CoAs). Missing-formula count 7 -> 0.
- Link finding counts to their CSV with an absolute repo blob URL on the PR branch, so the links resolve to the file instead of a PR-relative path. - Icon rule: red cross when a count rose vs the target branch (a regression this PR introduced), warning sign when a count is non-zero but did not rise (a pre-existing, non-blocking finding), check mark when zero. Growth stays pass/fail; the MEMOTE score warns only when it drops. - A result set is treated as current only when its commit stamp matches the PR head; an absent or older stamp now shows the rows as running (hourglass) instead of a previous run's numbers. - Empty reactions and model/annotation inconsistencies are non-blocking reports (a rising count still flags as a regression); only duplicate keys (model unloadable) and no growth block the merge.
edkerk
added a commit
that referenced
this pull request
Jul 14, 2026
Reconcile the CI migration with the QC consolidation (#1056) and combined QC workflow (#1059) merged into develop meanwhile: - gene-essentiality.yml: keep this branch's pull_request guard and updated comment wording, but post via github-script (develop dropped the deprecated comment-pr action and the commentGeneEssential.md template). - yaml-validation.yml: accept its removal; model-file-checks.yml now does the YAML lint. - model-file-checks.yml: drop the model/annotation consistency step - sanityCheck.py was removed on develop and its check is now in qcModelChecks.py (Model QC workflow). - README.md: use develop's model QC / MEMOTE / MACAW result mentions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Main improvements in this PR:
Add a duplicate-key QC gate so the class of bug behind the MAR01801 breakage in #1055 is caught with a clear message instead of an opaque cobra crash.
Each metabolite, reaction and gene is written as a YAML ordered map (
!!omap). A repeated key within one - twonamefields on a reaction, or the same metabolite twice in a reaction's stoichiometry - is written and re-read happily by RAVEN, but makescobra.io.load_yaml_modelraise a bareAssertionErrorwith no location (which is how the recentchecksandyaml-validationfailures surfaced).check_no_duplicate_keystocode/test/qcModelChecks.py: a PyYAML event scan that runs before the cobra load and fails the build with a clear message naming the entry, key and line numbers. Covers both duplicate fields and duplicate stoichiometry metabolites.pyyamlin the model-qc checks job.Validated locally: the clean model passes (the gate returns nothing, growth 124.9 ok); an injected duplicate
namefield and an injected duplicate stoichiometry metabolite are both reported with entry id and line numbers.No model changes.
I hereby confirm that I have:
data/deprecatedIdentifiers/.developas target branch, and will be resolved with a squash-merge.mainas target branch, and will be resolved with a merge commit.