Skip to content
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

Protege hangs when attempting to save certain obo files #501

Closed
cmungall opened this issue Aug 6, 2016 · 22 comments
Closed

Protege hangs when attempting to save certain obo files #501

cmungall opened this issue Aug 6, 2016 · 22 comments
Assignees
Labels
Status: Fixed Added to indicate that a closed issue represents a bug that has been fixed Type: Critical OWL API Bug A bug that originates in the OWL API that leads to data loss etc.

Comments

@cmungall
Copy link

cmungall commented Aug 6, 2016

The plant trait ontology editors would like to keep their source file in obo (for diffs) and edit in Protege. Unfortunately Protege hangs whenever they try and save.

The ontology can be parsed and saved using the OWLAPI so it seems likely there is something in Protege causing this. I have been unable to reduce this to a small test example (since it requires opening up Protege, making a change, saving, which is time consuming). There are no obvious culprits, and I have not seen this behavior in any ontology.

To reproduce:

  1. clone the following repo: https://github.com/Planteome/plant-trait-ontology
  2. use commit 11bb1305eabecd3c02e8f7d25e6c55a108c7c4ba (not necessarily important but just to be sure our environments are identical)
  3. Open the file plant-trait-ontology.obo in Protege
  4. Edit anything in the main ontology (not an important). Try carbon sensitivity, modify the textual definition
  5. Save

Protege will hang.

We are using the latest Protege -- About shows "Protege 5.0.0. (Build )"

cc @marieALaporte @austinmeier @cooperl09

@matthewhorridge
Copy link
Contributor

I can reproduce this with the particular commit mentioned. Just to note, with the head of master things work as expected. Will look into it.

@matthewhorridge
Copy link
Contributor

For information, I'm getting a bunch of errors when saving the commit:

Duplicate clause 'alt_id( TO:0000309)' generated in frame: TO:0000576 
Duplicate clause 'alt_id( TO:0000150)' generated in frame: TO:0000587 
Duplicate clause 'alt_id( TO:0002711)' generated in frame: TO:0020088 
Duplicate clause 'alt_id( TO:0000585)' generated in frame: TO:0000373 
Duplicate clause 'alt_id( TO:0000584)' generated in frame: TO:0000373 
Duplicate clause 'alt_id( TO:0000533)' generated in frame: TO:0000382 
Duplicate clause 'alt_id( TO:0000500)' generated in frame: TO:0000267 
Duplicate clause 'alt_id( TO:0000518)' generated in frame: TO:0000269 
Duplicate clause 'alt_id( TO:0000395)' generated in frame: TO:0000146 
Duplicate clause 'alt_id( TO:0000535)' generated in frame: TO:0000339 
Duplicate clause 'alt_id( TO:0000005)' generated in frame: TO:0000341

The errors are thrown by OWLAPIOwl2Obo

Protege should of course handle this a bit better.

@matthewhorridge
Copy link
Contributor

The above error messages are red herrings.

I think I've found the actual problem, which looks like it is caused by a deadlock that happens in the OWL API when there is essentially an attempt to upgrade a manager read lock to a write lock (which isn't possible). This is happens when the OWLAPIOwl2Obo code creates a fresh ontology during the save. I haven't checked, but the code path that does this isn't always invoked on a save.

@matthewhorridge
Copy link
Contributor

Another update...

The deadlock arises when the save code has to translate axioms that cannot be serialised to OBO syntax. In this case the axioms are saved in functional syntax and this serialisation is written into the OBO file - as far as I can tell. To do this, a fresh ontology is created. This requires a write lock, which blocks due to the read lock that is held during the save operation. It would be best if a separate manager was created in order to generate the functional syntax. However, we should also check the locking strategy in the OWL API to see whether it is doing the right thing here.

@cmungall
Copy link
Author

Thanks! Yes, you are quite right, see 5.0.4 of the spec for treatment of untranslatable axioms. Just knowing this is the cause should give us some short term options (these are probably unintended in this ontology).

cmungall added a commit to Planteome/plant-trait-ontology that referenced this issue Aug 24, 2016
Removing the owl-axioms header. This contained two axioms that couldn't be translated to .obo:

 * TO_0000241 had an rdfs:comment that was an empty string
 * TO_0000980 had a created_by that was an empty string

These were presumably unintentional/errors

It seems the owl->obo converter refuses to convert empty-string fields, which forces them into the OWL header (http://owlcollab.github.io/oboformat/doc/obo-syntax.html#5.04)

This is not itself a problem normally, but until protegeproject/protege#501 is fixed, the file cannot be saved in obo from protege
@dosumis
Copy link

dosumis commented Dec 12, 2016

Any progress on this? Was hoping it would be fixed in 5.1, but (from the file I'm working with right now at least) it seems not.

Many thanks,
David

@matthewhorridge
Copy link
Contributor

Hi David,

I believe that the fix for this needs to occur in the OBO writer. I'm not sure who's best placed to fix this. Copying in @ignazio1977 incase he has thoughts about it!

@ignazio1977
Copy link

OWLAPIOwl2Obo has a translationManager included that's used only for untranslatable axioms and to obtain data factories - looks like it was designed to act as a separate manager. However in the OWLAPI renderer this manager is initialised with the ontology manager, which would cause the problem Matthew is observing. I'll try to replicate this in a unit test.

@dosumis
Copy link

dosumis commented Jul 10, 2017 via email

@cmungall
Copy link
Author

cmungall commented Jul 10, 2017 via email

@matthewhorridge
Copy link
Contributor

This needs retesting with 5.5.0-beta-8 or later as we've upgraded the OWL API

@matthewhorridge matthewhorridge added the Status: Needs Reproducing Assigned to things that are bugs, but that have not been checked label Jan 15, 2019
@matthewhorridge matthewhorridge added this to the Protégé 5.5.0 milestone Jan 15, 2019
@csnyulas
Copy link
Member

I can confirm that the problem is still present in the latest snapshot of Protege-5.5.0-beta-9.

@csnyulas csnyulas added Status: Reproduced For issues that are (critical) bugs, denotes that the bug is reproduced, but no further action taken and removed Status: Needs Reproducing Assigned to things that are bugs, but that have not been checked labels Jan 25, 2019
@ignazio1977
Copy link

Are these large files? There's a long-standing issue with large obo files, the translator requires a lot of memory.

@matthewhorridge
Copy link
Contributor

I don't think the files are large @ignazio1977. I'm pretty sure that the problem is caused by the deadlock described above. Basically this comment and the one above it.

@matthewhorridge matthewhorridge added Type: Critical OWL API Bug A bug that originates in the OWL API that leads to data loss etc. Status: Waiting for OWL API Release Waiting for an updated release of the OWL API for this issue to be resolved and removed Status: Reproduced For issues that are (critical) bugs, denotes that the bug is reproduced, but no further action taken labels Jan 25, 2019
@ignazio1977
Copy link

Ok, deadlock can be replicated with a small test:

@Test
public void testNoDeadlock() throws OWLOntologyStorageException, OWLOntologyCreationException {
    OWLOntology o = OWLManager.createConcurrentOWLOntologyManager()
        .createOntology(IRI.create("urn:test:ontology"));
    o.getOWLOntologyManager().addAxiom(o,
        df.getOWLSubClassOfAxiom(df.getOWLNothing(), df.getOWLThing()));
    OWLOntologyDocumentTarget target = new StringDocumentTarget();
    o.saveOntology(new OBODocumentFormat(), target);
}

I'll look into a solution, but it might be tricky.

@matthewhorridge
Copy link
Contributor

Beautiful test case @ignazio1977

ignazio1977 added a commit to owlcs/owlapi that referenced this issue Jan 27, 2019
protegeproject/protege#501

OBO renderers serialise untranslatable axioms as a functional syntax
string, but the FS renderer needs an ontology to work on. Using the
manager that hosts the ontology to save to create the new ontology
causes a deadlock - the read lock has already been acquired, the
creation of a new ontology tries to engage the write lock -> deadlock.

Fix is to use an injector to get a new manager to create the ontology. A
manager provider would be a better solution but there are static methods
and interface changes involved.
@ignazio1977
Copy link

Fix is up, I'm not overly proud of it but it's the best fix that does not involve interface changes.

@matthewhorridge
Copy link
Contributor

Thanks @ignazio1977. Will keep my eyes open for a 4 release.

@ignazio1977
Copy link

I'd like to fix the different individuals annotation issue before releasing

@ignazio1977
Copy link

OWLAPI 4.5.9 has been released, fixes three bugs that affect Protege.

@matthewhorridge matthewhorridge added Status: Fixed Added to indicate that a closed issue represents a bug that has been fixed and removed Status: Waiting for OWL API Release Waiting for an updated release of the OWL API for this issue to be resolved labels Feb 1, 2019
@matthewhorridge
Copy link
Contributor

Thanks so much for the fix @ignazio1977

@cmungall
Copy link
Author

cmungall commented Feb 1, 2019

Thanks!!!

ignazio1977 added a commit to owlcs/owlapi that referenced this issue Feb 9, 2019
protegeproject/protege#501

OBO renderers serialise untranslatable axioms as a functional syntax
string, but the FS renderer needs an ontology to work on. Using the
manager that hosts the ontology to save to create the new ontology
causes a deadlock - the read lock has already been acquired, the
creation of a new ontology tries to engage the write lock -> deadlock.

Fix is to use an injector to get a new manager to create the ontology. A
manager provider would be a better solution but there are static methods
and interface changes involved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Fixed Added to indicate that a closed issue represents a bug that has been fixed Type: Critical OWL API Bug A bug that originates in the OWL API that leads to data loss etc.
Projects
None yet
Development

No branches or pull requests

5 participants