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

Issue noctua models 170 zfin import test #182

Closed
wants to merge 23 commits into from

Conversation

sierra-moxon
Copy link
Member

fixes #170
fixes #180
fixes #181

Adding ZFIN models to master.

@sierra-moxon
Copy link
Member Author

@dustine32 - I'll have Sabrina take a look at the deleted models.

@dustine32
Copy link
Contributor

@sierra-moxon Thanks! Looks like you cleared up the contributor bug.

@sierra-moxon
Copy link
Member Author

@dustine32 - confirmed with Sabrina that she is expecting a handful of deleted models. Do model deletes (triggered via noctua interface) only happen when a PR to master occurs?

@dustine32
Copy link
Contributor

@sierra-moxon Thanks for confirming, at least it's not 100% weird then! Gonna have to confirm with @kltm. My guess is that a model delete through the Noctua UI will remove the model from the in-memory datastore (in minerva) but not immediately in the noctua-models repo. To trigger the delete from the repo, that "automated commit" process would have to run, which dumps out the in-memory models to files and commits them to master, deleting the models in repo if they didn't exist in-memory at that time.

Sort of a question: It looks like your PR here includes the delete changes in the diff, which seems not right to me? I would expect a PR of only new ZFIN models would not touch any of the models created through the Noctua UI. Does this make sense?

@kltm
Copy link
Member

kltm commented Apr 21, 2021

Talking to @sierra-moxon , I think there is still a bit of a mystery here as to how the deletes happened here. AFAIK, there is no mechanism to literally delete files in any system right now, including minerva (although @balhoff may know something I don't). I believe(d) that deletes are just an inactive element at this point and we're still working through what exactly we want "delete" to mean.

@balhoff
Copy link
Member

balhoff commented Apr 21, 2021

@kltm I noticed that Ben added a deletion mechanism to Minerva. I'm not familiar enough with the situation here to know if it's relevant. There is an arg here skipMarkedDelete used when loading to the triplestore: https://github.com/geneontology/minerva/blob/07a2e5690a6f39264e5ca7db8600ccf8bd8131c4/minerva-core/src/main/java/org/geneontology/minerva/BlazegraphMolecularModelManager.java#L608

It's set to true here: https://github.com/geneontology/minerva/blob/07a2e5690a6f39264e5ca7db8600ccf8bd8131c4/minerva-cli/src/main/java/org/geneontology/minerva/cli/CommandLineInterface.java#L484

@kltm
Copy link
Member

kltm commented Apr 21, 2021

@balhoff Okay, that actually sounds like what we're experiencing, so I think we're good--we were curious as to why models marked as "delete" were no longer accessible in some cases. (We still have an issue where a few models got removed from a PR, but I think that is separate.)

@sierra-moxon sierra-moxon mentioned this pull request May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

migrate travis build to github actions fix travis build Load remaining ZFIN Models into noctua
4 participants