All equivalence axioms / logical defs must map to a pattern OR be explicitly approved

[cmungall]

We need a check

1. IF there is a logical def (equiv axiom between NC and CE)
2. AND that does not map to a DP
3. AND it does not have an annotation on it to the effect of "we know this is a wildcard def"
4. THEN throw an error

@nicolevasilevsky do we have this in mondo? can we port over?

[matentzn]

Its hard to do 2 on the fly.. We check these in Mondo after the fact, not at PR time.

Addressing this issue in a computational way is currently not implemented anywhere afaik

[github-actions]

This issue has not seen any activity in the past 6 months; it will be closed automatically in one year from now if no action is taken.

[matentzn]

@cmungall can we use the owl-patterniser on Uberon? Would you be able to support @rays22 to do that (because we want to go that direction anyways not only for Uberon, but in general)?

[github-actions]

This issue has not seen any activity in the past 6 months; it will be closed automatically one year from now if no action is taken.

[matentzn]

@anitacaron mention in tech team? Documentation of this could be universally useful..

[github-actions]

This issue has not seen any activity in the past 6 months; it will be closed automatically one year from now if no action is taken.

[matentzn]

Mybe aplit the issue:

1. If the diff touches an intersections_of, require a cjm / davidos review (do this in github action)
2. The pattern based pattern based validation requires grant money.

[github-actions]

This issue has not seen any activity in the past 6 months; it will be closed automatically one year from now if no action is taken.

[matentzn]

@gouttegd here is what I was thinking (roughly)

https://github.com/obophenotype/test-actions/blob/main/.github/workflows/reviewers.yml

However, the last block is bad from a workflow perspective. I would like to post a failing review, but I cant do that in the name of whoever is the responsible person - so I posted one in the name of github actions.

Example PR: obophenotype/test-actions#1

This means, whoever, that GitHub actions will request changes, and someone needs to remember to dismiss that review when they are satisfied. Are we happy with this overhead? A slightly less intrusive way of handling the SOP would be to simply post a comment with a checkbox Reminding folks to NOT MERGE until the reviews of the main reviewer are in (and use this opportunity to even explain WHY the review is needed?)

[gouttegd]

First, the way you detect changes to logical axioms won’t work. You can’t just check for the presence of intersection_of: tags in the edit file -- this would find all the intersection_of: tags that are present in the edit file (including those that have been there for ages), while what you want is to find the tags that have been added, removed, or modified by the PR.

So what you need to do is to

(1) Look at the diff between the uberon-edit.obo file from the master branch and the uberon-edit.obo file from the PR’s head revision, instead of looking at the uberon-edit.obo file from the PR’s head.

(2) Look for ^(-|\+)intersection_of: lines. You must specifically look for lines starting with a - or a + to avoid detecting the lines that are in the diff just because they are in the context window of the diff, as in this example:

--- a/src/ontology/uberon-edit.obo
+++ b/src/ontology/uberon-edit.obo
@@ -61071,7 +61071,6 @@ xref: FMA:72595
 xref: HBA:265504914
 xref: neuronames:756 {source="BIRNLEX:2638"}
 xref: UMLS:C0262207 {source="BIRNLEX:2638"}
-xref: ZFA:0000518
 intersection_of: UBERON:0035011 ! central gray substance
 intersection_of: part_of UBERON:0001896 ! medulla oblongata
 relationship: part_of UBERON:0001896 {source="FMA"} ! medulla oblongata

Second, regarding what to do when a PR is found to be changing an equivalence axiom:

My concern with your approach is not the fact that the automatic review will have to be dismissed, it’s more that, even after the PR will have been reviewed and approved by a senior editor, it will still appear as having failed one of the CI checks. This does not prevent merging because Uberon (AFAIK) does not enforce that all CI checks must pass before a PR can be merged, but I think it’s a bad idea to get people used to the idea that a failed CI check is not something to worry about.

Why not simply post the failing review, but then allow the CI check to terminate successfully? The failing review in itself is enough to prevent merging, there is no need to end the check on a failure.

[matentzn]

Thanks for feedback.. I forgot to change the grep goal, sorry. Better?

https://github.com/obophenotype/test-actions/blob/main/.github/workflows/reviewers.yml

I tested the diff.txt logic locally now, but weirdly the condition seems not to work in the action itself, see

https://github.com/obophenotype/test-actions/actions/runs/11816392103/job/32919571534

[gouttegd]

I tested the diff.txt logic locally now, but weirdly the condition seems not to work in the action itself, see

When you ask for git diff uberon-edit.obo from the tip of the PR branch, you won’t see anything, and that’s normal. By default, git diff shows the change in the working copy compared to the HEAD revision. Here (in the context of the GitHub Actions workflow), the working copy has been freshly checked out, so it does not contain any change.

You must compare the tip of the PR branch with the “base” -- the latest commit that is common to the two branches:

git diff master... -- src/ontology/uberon-edit.obo

[gouttegd]

Actually it would be better to compare with base.ref rather than hard-coding master. github.base_ref is set to point to the base of whichever branch the PR is intended to be merged into (most of the time it would be master, but not always):

git diff ${{ github.base_ref }}... -- src/ontology/uberon-edit.obo

[matentzn]

Works now. If you check one last time, I will make a PR on Uberon with the action for further discussion!

[gouttegd]

Looks good to me.

I just wonder if it would be possible to:

(1) re-trigger the workflow every time the “review status” of the PR is changed (i.e., when someone approves the PR);
(2) check whether the PR has been approved by a senior editor;
(3) and if so, automatically dismiss the previous “change requested“ review.

But that’s a refinement that can be left for later (assuming it is possible). Better to check with the Uberon editors what they think of the basic idea before making it more complicated. :)

[matentzn]

Draft here: #3425

[gouttegd]

It just occurred to me that another improvement to the action could be to use ROBOT to get the diff, rather than git diff. It would be slightly more complicated (we’d need to check out both versions to compare in two directories side by side, so that we can call robot diff on the two versions), but the benefit is that it would work regardless of the edit format. Instead of looking for lines containing intersection_of: (which can only work if the edit file is in the OBO format), we would look for lines containing EquivalentClasses.

It wouldn’t change anything for Uberon, but it would make the action easily “transferable” to other ontologies that use OFN as their edit format (such as CL).

[matentzn]

The added level of generality would come at additional processing cost; right now the action virtually needs no memory, finishes in 15 seconds doesn't need ODK - I would first test this version in Uberon, and maybe refine the strategy later if a need arises?