-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating ELK 0.43 to 0.5 #999
Conversation
@jamesaoverton and I discussed this and we will move this to a future version of ROBOT, maybe ROBOT 2.0 :P |
@matentzn @jamesaoverton I think it's a high priority for us to get ELK 0.5 into ROBOT. I started to look at publishing a version in one of our own orgs, so we wouldn't need to use the snapshot repo (which I think would prevent ROBOT from being published to Maven central). But I found this one already on Maven central: https://search.maven.org/artifact/au.csiro/elk-owlapi4/0.5.0/jar Can we just use that for the time being? |
I would suggest we do this asap:
Unless someone figures out how to have elk 0.4 and 0.5 co-exist, I am a bit worried who will deal with the 1 billion support issues coming from unsats revealed by 0.5. |
I never tried to use two versions of the same dependency in a Java project before. Some quick research makes it look like black magic to me, but if someone else has experience then let us know. I'm OK with creating a ROBOT branch and beta release |
Maybe I am overthinking this. @jamesaoverton and I have decided to make a quick experiment with OBO Dashboard and see what will happen if we simply swap in ELK 0.5. Basically, we will compare unsats with ELK 0.4.3 and make a decision based on that. |
Sorry I forgot it again @jamesaoverton. Was it somehow possible to run an action and obtain a ROBOT jar from a branch using a URL (this makes it easy to inject a DEV version of ROBOT into ODK)? |
Ok, assigned this to @anitacaron, but it will have to wait until mid February until after the OntoSummit Dashboard session is finished: OBOFoundry/OBO-Dashboard#82 |
Kinda. Our GitHub Actions do create a JAR file that you can download with a browser, BUT the link uses some JavaScript so you don't get a URL that you can just I'll try to make a pre-release here in the repo. |
Here is a pre-release with an attached JAR: https://github.com/ontodev/robot/releases/tag/v1.9.1-elk-0.5.0 I didn't know how to test that it actually includes ELK 0.5.0, so please confirm that before running hours of tests. |
@jamesaoverton, checking in on this PR. Editors are switching over to Protégé 5.6.1 which has ELK 0.5.0. Based on the thread above, it is not clear what the next step is to update ROBOT to 0.5.0 and who needs to do it. Is there something specific someone needs to do to move this forward so Protégé and ROBOT can be aligned? Thanks for your attention. |
@bvarner-ebi thanks for pushing, this is on me. I will soon create a ODK dev release with ELK 0.5 for testing |
ELK 0.5 did not recognise this ontology (in a test) as incoherent, because it reverted to a wrong branch in the unsatisfiability testing framework in ROBOT due to an incomplete implementation (liveontologies/elk-reasoner#66).
Seems that ELK 0.43 was able to for some reason. Replicated this in Protege as well. EDIT: this is now dealt with: liveontologies/elk-reasoner#66 |
robot-core/src/test/java/org/obolibrary/robot/ReasonerHelperTest.java
Outdated
Show resolved
Hide resolved
.getPrecomputableInferenceTypes() | ||
.contains(InferenceType.OBJECT_PROPERTY_HIERARCHY)) { | ||
if (reasoner.getPrecomputableInferenceTypes().contains(InferenceType.OBJECT_PROPERTY_HIERARCHY) | ||
&& !reasoner.getClass().getName().equals("org.semanticweb.elk.owlapi.ElkReasoner")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary because of liveontologies/elk-reasoner#66 (comment)
Basically, ELK 0.5 (as opposed to 0.43) claims to implement OBJECT_PROPERTY_HIERARCHY
, but it does not recognise unsatisfiable object properties (or at least, not the one we are looking for in the test for incoherentRBox).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @matentzn for tracking down the cause and for this solution! I'm glad that it didn't require reverting the faster inference when using Hermit - that's been a huge boon in my workflow.
Given my new awesome diff system, I determined these cases with difference in unsats: Differences in the number of unsatisfiable classes comparing Elk 0.4.3 and Elk 0.5
cc @FernandaFarinelli @JCGiron @zachll @DanBerrios @mjy @jonrkarr @pbuttigieg I hereby officially make my recommendation that this should be merged - its thoroughly tested, and ready for action. The differences in unsats are only revealing problems that are already there. |
I don't have a good way to test this, so I'm taking @matentzn at his word. |
Resolves #380 #660
docs/
have been added/updated (NOT NECESSARY)mvn verify
says all tests passmvn site
says all JavaDocs correctCHANGELOG.md
has been updatedThis pull request migrates ELK to the 0.5 version. I have tested it in various ODK pipelines, and bundled it with the latest ODK:dev image.