Add a script to QC the update-synonyms-sync target#10260
Conversation
This adds a script (`synonym_sync_qc`) that looks at `mondo-edit.obo` after `update-synonyms-sync` has been run (but before it has been committed). It runs two checks: 1. Are all unstaged changes to `mondo-edit.obo` confined to lines about synonyms? If not, that means that there are extraneous changes that shouldn't be happening with the synonym sync workflow. 2. Is the set of synonyms pre-sync the exact same set post-sync? And if so, what axioms (if any) have changed on those synonyms? (1) uses `git diff` as the signal for analysis, (2) uses `fastobo` to actually parse `mondo-edit.obo` before and after sync. Because `fastobo` is fast, this runs an analysis on `mondo-edit.obo` in 6 seconds. --- This script also writes a report formatted as markdown. The report details whether the checks passed, and then gives some details about how synonym axioms have changed. --- The synonym-sync workflow has been updated to run this script. I also took out the `make test` invocation in there, since it will be run on the PR automatically anyway.
|
here's an example of the report generated from synonym sync run on current Synonym sync QC report
Pass 1: non-synonym line changesNo non-synonym lines changed. Pass 2: synonym additions and deletionsNo synonyms were added or removed. Modifications by category
Provenance xref changes by source prefix
modifier_change (2)
- type=None
+ type=ABBREVIATION
- type=None
+ type=ABBREVIATIONprovenance_change (29)
- xrefs=['NCIT:C4376']
+ xrefs=['DOID:1074', 'NCIT:C4376']
- xrefs=['NCIT:C116347']
+ xrefs=['DOID:14159', 'NCIT:C116347']
- xrefs=['Orphanet:1671']
+ xrefs=[]
- xrefs=['ICD10CM:Q06.2', 'NCIT:C98913', 'OMIM:222500', 'Orphanet:573278', 'icd11.foundation:2070601288']
+ xrefs=['ICD10CM:Q06.2', 'NCIT:C98913', 'OMIM:222500', 'icd11.foundation:2070601288']
- xrefs=['GARD:0001851', 'Orphanet:573278']
+ xrefs=['GARD:0001851', 'Orphanet:573278', 'icd11.foundation:2070601288']
- xrefs=['Orphanet:1671']
+ xrefs=[]
- xrefs=['GARD:0001851', 'Orphanet:573278']
+ xrefs=['GARD:0001851']
- xrefs=[]
+ xrefs=['DOID:1572']
- xrefs=['DOID:8970', 'icd11.foundation:1098683540']
+ xrefs=['icd11.foundation:1098683540']
- xrefs=['OMIM:306000']
+ xrefs=['DOID:0111042', 'OMIM:306000']
- xrefs=['DOID:0111042', 'https://www.ncbi.nlm.nih.gov/books/NBK55061/']
+ xrefs=['https://www.ncbi.nlm.nih.gov/books/NBK55061/']
- xrefs=['OMIM:603457', 'https://orcid.org/0000-0001-9310-0163']
+ xrefs=['https://orcid.org/0000-0001-9310-0163']
- xrefs=['OMIM:603457', 'https://orcid.org/0000-0001-9310-0163']
+ xrefs=['https://orcid.org/0000-0001-9310-0163']
- xrefs=['OMIM:603457', 'https://orcid.org/0000-0001-9310-0163']
+ xrefs=['https://orcid.org/0000-0001-9310-0163']
- xrefs=['OMIM:608232']
+ xrefs=[]
- xrefs=['DOID:0110117', 'OMIM:614470', 'Orphanet:268114']
+ xrefs=['DOID:0110117', 'Orphanet:268114']
- xrefs=['DOID:0110117', 'OMIM:614470']
+ xrefs=['DOID:0110117']
- xrefs=['DOID:0111214', 'Orphanet:314485']
+ xrefs=[]
- xrefs=['DOID:0111214', 'Orphanet:314485']
+ xrefs=[]
- xrefs=['NORD:1914']
+ xrefs=['DOID:0080453', 'NORD:1914']
- xrefs=['OMIM:155100']
+ xrefs=[]
- xrefs=[]
+ xrefs=['OMIM:155100']
- xrefs=['https://orcid.org/0000-0001-9310-0163']
+ xrefs=['OMIM:155100', 'https://orcid.org/0000-0001-9310-0163']
- xrefs=[]
+ xrefs=['OMIM:155100']
- xrefs=[]
+ xrefs=['OMIM:155100']
- xrefs=['OMIMPS:610805', 'Orphanet:93545']
+ xrefs=['DOID:0080205', 'OMIMPS:610805', 'Orphanet:93545']
- xrefs=[]
+ xrefs=['DOID:0061138', 'OMIM:618174']
- xrefs=['Orphanet:495274']
+ xrefs=['DOID:0110160', 'Orphanet:495274']
- xrefs=['Orphanet:495274']
+ xrefs=['DOID:0110160', 'Orphanet:495274'] |
Put rdfs:label in the header, and the text of the synonym beneath, showing what type of synonym it is in the synced version.
|
I changed the report a little bit, to make it clear what the label is and what the synonym is: Synonym sync QC report
Pass 1: non-synonym line changesNo non-synonym lines changed. Pass 2: synonym additions and deletionsNo synonyms were added or removed. Modifications by category
Provenance xref changes by source prefix
modifier_change (2)
- type=None
+ type=ABBREVIATION
- type=None
+ type=ABBREVIATIONprovenance_change (29)
- xrefs=['NCIT:C4376']
+ xrefs=['DOID:1074', 'NCIT:C4376']
- xrefs=['NCIT:C116347']
+ xrefs=['DOID:14159', 'NCIT:C116347']
- xrefs=['Orphanet:1671']
+ xrefs=[]
- xrefs=['ICD10CM:Q06.2', 'NCIT:C98913', 'OMIM:222500', 'Orphanet:573278', 'icd11.foundation:2070601288']
+ xrefs=['ICD10CM:Q06.2', 'NCIT:C98913', 'OMIM:222500', 'icd11.foundation:2070601288']
- xrefs=['GARD:0001851', 'Orphanet:573278']
+ xrefs=['GARD:0001851', 'Orphanet:573278', 'icd11.foundation:2070601288']
- xrefs=['Orphanet:1671']
+ xrefs=[]
- xrefs=['GARD:0001851', 'Orphanet:573278']
+ xrefs=['GARD:0001851']
- xrefs=[]
+ xrefs=['DOID:1572']
- xrefs=['DOID:8970', 'icd11.foundation:1098683540']
+ xrefs=['icd11.foundation:1098683540']
- xrefs=['OMIM:306000']
+ xrefs=['DOID:0111042', 'OMIM:306000']
- xrefs=['DOID:0111042', 'https://www.ncbi.nlm.nih.gov/books/NBK55061/']
+ xrefs=['https://www.ncbi.nlm.nih.gov/books/NBK55061/']
- xrefs=['OMIM:603457', 'https://orcid.org/0000-0001-9310-0163']
+ xrefs=['https://orcid.org/0000-0001-9310-0163']
- xrefs=['OMIM:603457', 'https://orcid.org/0000-0001-9310-0163']
+ xrefs=['https://orcid.org/0000-0001-9310-0163']
- xrefs=['OMIM:603457', 'https://orcid.org/0000-0001-9310-0163']
+ xrefs=['https://orcid.org/0000-0001-9310-0163']
- xrefs=['OMIM:608232']
+ xrefs=[]
- xrefs=['DOID:0110117', 'OMIM:614470', 'Orphanet:268114']
+ xrefs=['DOID:0110117', 'Orphanet:268114']
- xrefs=['DOID:0110117', 'OMIM:614470']
+ xrefs=['DOID:0110117']
- xrefs=['DOID:0111214', 'Orphanet:314485']
+ xrefs=[]
- xrefs=['DOID:0111214', 'Orphanet:314485']
+ xrefs=[]
- xrefs=['NORD:1914']
+ xrefs=['DOID:0080453', 'NORD:1914']
- xrefs=['OMIM:155100']
+ xrefs=[]
- xrefs=[]
+ xrefs=['OMIM:155100']
- xrefs=['https://orcid.org/0000-0001-9310-0163']
+ xrefs=['OMIM:155100', 'https://orcid.org/0000-0001-9310-0163']
- xrefs=[]
+ xrefs=['OMIM:155100']
- xrefs=[]
+ xrefs=['OMIM:155100']
- xrefs=['OMIMPS:610805', 'Orphanet:93545']
+ xrefs=['DOID:0080205', 'OMIMPS:610805', 'Orphanet:93545']
- xrefs=[]
+ xrefs=['DOID:0061138', 'OMIM:618174']
- xrefs=['Orphanet:495274']
+ xrefs=['DOID:0110160', 'Orphanet:495274']
- xrefs=['Orphanet:495274']
+ xrefs=['DOID:0110160', 'Orphanet:495274'] |
matentzn
left a comment
There was a problem hiding this comment.
This is great! Looks amazing. Can we wrap the detailed lists in <details>...</details> and add some way to crop the list at say 30 entries per category in case the PR has MAAAANY changes (that would make the github PR field blow up)?
And then I am just happy to go ahead and try!
matentzn
left a comment
There was a problem hiding this comment.
And the <details> thing i mentioned earlier
| make GITHUB_ACTION=true IMP=false PAT=false update-synonyms-sync -B | ||
|
|
||
| - name: QC to ensure that nothing other than provenance has changed | ||
| run: cd src/ontology; make GITHUB_ACTION=true IMP=false PAT=false test -B |
There was a problem hiding this comment.
Make sure make test REALLY runs as you say - I dont think so without additional prodding (action triggering action)
There was a problem hiding this comment.
here's a PR from a previous run: #10194

That QC is run from a PR event.
The QC in the sync workflow runs before the PR is created, on the exact same SHA, but it doesn't count as the required QC check. That makes it totally superfluous.
Yep, good idea. Should there possibly be some diagnostic about synonyms that have lost all provenance? |
For now just a number; maybe broken by "prefix that used to support it". But we have not yet decided how exactly SOP these cases. |
1. Allow calling the script on a specific ref, to get a summary of changes for commits in the past (not just unstaged changes) 2. Add an option to export a short summary, appropriate for a pull request body. 3. Add that summary as the PR body in the synonyms.yaml workflow, and attach the full report as an artifact that is hyperlinked. (To prevent a super long commit message).
You were completely right about this. And it led me to an interesting conclusion. We have been talking about the limitations of using the built-in mondo/.github/workflows/synonyms.yaml Lines 26 to 35 in 265623d
In my view, it would be good to stop relying on this, and remove instances of your PAT, if possible. I have changes coming into #10244 which will do just that. I'll wait on this one until we finish that. |
Please micromanage my reviews on slack on this - happy to follow your nose on this one! Whats the next step then? |
This adds a script (
synonym_sync_qc) that looks atmondo-edit.oboafterupdate-synonyms-synchas been run (but before it has been committed). It runs two checks:Are all unstaged changes to
mondo-edit.oboconfined to lines about synonyms? If not, that means that there are extraneous changes that shouldn't be happening with the synonym sync workflow.Is the set of synonyms pre-sync the exact same set post-sync? And if so, what axioms (if any) have changed on those synonyms?
(1) uses
git diffas the signal for analysis, (2) usesfastoboto actually parsemondo-edit.obobefore and after sync. Becausefastobois fast, this runs an analysis onmondo-edit.oboin 6 seconds.This script also writes a report formatted as markdown. The report details whether the checks passed, and then gives some details about how synonym axioms have changed.
The synonym-sync workflow has been updated to run this script. I also took out the
make testinvocation in there, since it will be run on the PR automatically anyway.