-
Notifications
You must be signed in to change notification settings - Fork 34
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
Upgrade to OWL API 4.5.29. #327
Conversation
I don't think we have CI checks enabled here, but the remaining tests do all pass for me locally. |
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.
I mean, I trust your own review much more. It looks ok - you skip a lot of tests, but I am assuming you dont do so without a good reason.
As I am anyways advocating for fading owltools out, I am fine with these changes which I eyeballed.
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.
Assuming these different are accounted for!
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.
My not too deep analysis made me think it is related to the new output of a label for oboInOwl:id
, like noted here: owlcs/owlapi#1102 (comment)
@kltm if I merge this PR, would it immediately start getting built and used in the GO pipeline, or is the pipeline getting owltools from ODK now? |
@balhoff Mixed bag, perhaps. |
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.
@balhoff eyeballing the changes, mostly adding ignores and updating endpoints, it seems to make sense. Everything else I would leave to your judgement.
@kltm I was thinking about the timing of things. I would merge this into master and then make an update to ODK. Once that is out the ontology build will start using it. But are you okay with the pipeline immediately using the new OWL API? I think it will be okay for the ontology, since it won't be saving out ontology files. |
I'm not sure of the scope of any disruption, if any, as we're locked to ODK in the ontology build itself. Or, to ask another way, why would the solr loads or predictions creation change if this went in? |
I don't think they would! Just wanted to check for any apprehension you may have. But it ought to be unchanged. |
@balhoff Nah--my only ask would be that this is done either late today or early tomorrow, to give the current |
I updated a bunch of tests, but had to ignore a few due to some unfamiliarity and suspected disused code. This OWL API upgrade is important for maintaining OBO format parity with Protege and ROBOT.