geneontology / noctua

Graph-based modeling environment for biology, including prototype editor and services
http://noctua.geneontology.org/
BSD 3-Clause "New" or "Revised" License
36 stars 13 forks source link

Update noctua violation handling to conform to nodes-as-model in ShEx in minerva return format for validation #714

Closed kltm closed 3 years ago

kltm commented 3 years ago

Looking at some of the recent discussions around violation checking (reasoner/shex), I think it would be good to go over the expectations around the responses from minerva.

As pointed out here: https://github.com/geneontology/minerva/issues/371#issuecomment-786404487 minerva is returning model identifiers for node, which I do not believe was part of the original spec. (I'm still looking for where Ben and myself hashed this out originally.)

To move other tickets along, like https://github.com/geneontology/noctua/issues/711, I think it might be good to come to a conclusion about the schema and appropriate contents are for the violations and then work forward from there.

Tagging @balhoff @tmushayahama

kltm commented 3 years ago

Noting that as a separate concern, we should also make the graph editor less "crashy" when things go wrong here.

kltm commented 3 years ago

Noting that the main issues that discuss this to date seem to be: https://github.com/geneontology/minerva/issues/212 and https://github.com/geneontology/go-shapes/issues/197

As well. the test used https://github.com/berkeleybop/bbop-graph-noctua/blob/cc126170af38f0e962de56d3f9e1b865cab3757a/tests/response-gomodel-5d88482400000052-2019-09-25.json

Looking through the conversation, I believe that a model for a node is probably a bug. (Again, disregarding that we can make the client less crashy.)

goodb commented 3 years ago

@kltm if it helps, the shex validator does intentionally use the model identifier as a node when checking and when reporting about validation of the model-level metadata (e.g. title). It seems maybe there is conflict with the concept of "node" in shex sense that allows the identifier of the model itself to be what it considers a node and the concept of a node in the graph client which likely does not.

kltm commented 3 years ago

@goodb Ah, well, yeah--that's a good clarification :)

I'm going to hollow out this out and repurpose this.

kltm commented 3 years ago

@vanaukenk @goodb Does somebody have a good test model for this?

goodb commented 3 years ago

@kltm if you create a new model and run the reasoner/validator before adding a title you get the invalid response.

vanaukenk commented 3 years ago

@kltm - I tested running the reasoner in the graph editor on a model that doesn't have a title.

The reasoner runs fine and the background on the graph editor turns green (there were otherwise no logical errors), but the Valid status on the model is 'INVALID' .

Adding a title to the model sets its Valid status to 'true'.

This looks like the expected behavior, although future work could be done to feedback about needing a model title.

I'm closing this ticket for now. Please feel free to re-open, if need be.