geneontology / minerva

BSD 3-Clause "New" or "Revised" License
6 stars 8 forks source link

reset & undo not working from the graph #371

Open lpalbou opened 3 years ago

lpalbou commented 3 years ago

I was working on this, trying to understand why the model was invalid according to shex: https://github.com/geneontology/go-shapes/issues/257

I went ahead and deleted the "DNA motif" node, which fixed the shex issue. I then wanted to restore the model to its initial state and used the menu "undo" functionality. When activating the reasoner, it's now throwing an error and freeze:

Screen Shot 2021-02-25 at 5 03 51 PM

I then tried a model/reset from the menu. It should have restored the model as it was after its last save (I obviously didn't saved my removal of the DNA motif node). It didn't and I still have the option to undo and reset the model, and still the same error as above.

I don't know if that issue is at the minerva level or the graph level. I would hope this is at the graph level, otherwise this could have numerous side effects including in ART. @cmungall @kltm

kltm commented 3 years ago

Can this behavior be duplicated on noctua-dev?

lpalbou commented 3 years ago

Unfortunately the model on Noctua dev is not the same: http://noctua-dev.berkeleybop.org/editor/graph/gomodel:59bee34700000179 so I couldn't reproduce or try to understand the error there.

kltm commented 3 years ago

Attempted duplication with:

http://noctua-dev.berkeleybop.org/editor/graph/gomodel:60297eed00000021

The reasoner goes on and off without issue. Assuming that this is a functionally the same thing, it sound like something deeper may be happening in minerva.

lpalbou commented 3 years ago

I reproduced the same steps on your link:

  1. activate reasoner
  2. compact view
  3. delete DNA motif node
  4. notice shex violations disappearing
  5. undo last
  6. refresh + activate reasoner

But this time, it was working. I also tried to do more undo and more reset (we should probably grey out those options when there is nothing to undo or reset) and it still work. So I don't know what happened on prod. Maybe the logs there will tell more. Possibly some code change on dev (more recent than prod) fixed that too (possibly ben work on reset ?)

kltm commented 3 years ago

I sat on and watched, hoping to get a chance to see it as you worked. Nothing like seeing a bug in progress, but alas. Hm, a little frustrating. We have another reason to want to cycle the production server, so maybe it's time to start considering a refresh.

It looks like dev and production have diverged a bit, but I'm not aware of the current state of dev (much less what client dependencies may be). https://github.com/geneontology/minerva/network @vanaukenk @balhoff Any thoughts here?

cmungall commented 3 years ago

I recommend writing a minerva unit test. of course it may be a bug that only manifests in certain deployed contexts and interactions with barista. But still: unit tests!!

Aside: I note yall say 'reasoner' to be inclusive of shex validation. Massively confusing, this is not reasoning.

tmushayahama commented 3 years ago

@lpalbou @kltm @cmungall @balhoff I know this error when I tried to implemented Human readable Shex errors. In short it is a violation that a model level annotation of providedBy cardinality is 0 and expected is 1 or more. So the the shex is okay, However the error comes from the graph editor's assumptions that "node" is always an individual, but a node in shex can be a model. Therefore node.types() will give you an error. Diagrams below

Shex Error and node Object (model not individual) image

The snippet of the code where it appears Getting types of undefined image

And the reason why the dev model is working somehow providedBy is not a shex error But it seems like it has the same model level annotations idk image

lpalbou commented 3 years ago

Well, thanks for the feedback @tmushayahama , but I think you missed the point.

This ticket Is to describe an issue in minerva undo/reset, not issue with shex. The shex issue is further detailed here: https://github.com/geneontology/go-shapes/issues/257

My guess for this specific issue:

Anyhow, we can close this issue for now if we can not reproduce; possibly creating another ticket as suggested by @cmungall to add a unit test for those delete/undo delete/reset operations on minerva side.

~@tmushayahama if you really want, try to do the reset on this prod model from an API call. If we can reactivate the reasoner, then it means the issue is in how Noctua graph request undo/reset.~ . actually we can now activate the reasoner on the model without failing.. that's odd.

tmushayahama commented 3 years ago

@lpalbou the issue is more caused by unreachable JavaScript code on the button events. During my ART development, I get this error a lot and I have manually call the barista API to reset/undo model away from GE and it works.

tmushayahama commented 3 years ago

@lpalbou I think we should not close this ticket because there is clearly an unhandled error on the JavaScript side which is potentially causing more errors. It will be nice the Graph Editor to handle the types error when dealing with human readable shex code (above snippet)., so to be sure that the reset and undo button button events don't have side effects. Because calling the reset and undo manually using API (i.e. in ART) doesn't cause error, but gets the errors if I click the buttons