It would be nice to "traffic light" the new xDTD

[saramsey]

@edeutsch wrote in RTXteam/RTX Issue 2052:

I suggest that it would be nice to "traffic light" the new xDTD instead.

Originally posted by @edeutsch in #2052 (comment)

[isbluis]

Hi @saramsey . I started to look into this, but xDTD is "special", in that it is called first before all other KPs (and not in parallel) -- so I may need @amykglen 's help in making sure this is done correctly.

[amykglen]

ah - yeah, since xDTD isn't called via a web request (instead it essentially consists of doing local sqlite queries), it's not done concurrently (since there's no i/o waiting time for asyncio to go work on other things).

what exactly is the traffic light based on @isbluis? or, in other words, is there something missing you need to make it work?

[edeutsch]

just need to call response.update_query_plan() with some suitable messages before the xDTD work begins and then again after when the xDTD work is complete.

[amykglen]

ah, got it. yeah we can do that easily

[amykglen]

is there an infores curie we should use to refer to xDTD in the query plan?

also, I'm assuming we'd probably like to have a traffic light for xCRG as well? (easy to add in response.update_query_plan() calls for that as well while I'm in there..)

and @isbluis - is there a branch I should commit this to? or just in master?

and one more for @isbluis - did you already make those changes you proposed to fully remove the old DTD as a KP? if you haven't I'm happy to do that

[isbluis]

Hi Amy.
I have a few DTD changes in my dev area that I can commit, sure.

The issue that made me pause is that there are multiple paths within the xDTD code block that exit before moving on to launching queries to other KPs, in which case it may be tricky to update the query_plan.

I think it can all be done in master, as these are not likely to be breaking changes.
Thanks!

[amykglen]

ah, nice. yeah, that's a good point about the code path exiting - I'm actually not sure how lookup results are combined with ineferred results? I think that's something @kvnthomas98 did recently? that probably is important to understand in order to figure out how to properly update the query plan... so I'll hold off on making changes for now (discussing how this works with Kevin in Slack)

[amykglen]

ok, Kevin helped me understand how lookup answers are merged with inferred answers, and I just pushed some code to master that I think should allow the traffic light to work for xdtd/xcrg (I added a few update_query_plan() calls). it's slightly weird since they currently aren't called at quite the same place as other KPs, but hopefully this works?

[edeutsch]

Looks good to me, thanks!

Is infores:arax-xdtd a real registered thing?

I don't see it.

Maybe add it to Steve's PR? https://github.com/biolink/biolink-model/pull/1403/files

[edeutsch]

Or maybe it doesn't need to be. It looks like the primary_knowledge_source is recorded as infores:arax. So maybe having a fake KP name is okay here.

Maybe it would be more honest to have the KP name be just "arax-xdtd" instead of "infores:arax-xdtd", since the latter is not a real thing.

Same with infores:arax-normalized-google-distance.

I think if we're going to use the infores prefix, it really ought to be in the ontology. Otherwise, just use a friendly name without a curie prefix.

a trivial thing though..

[isbluis]

I also notice that our NGD is almost always skipped -- is it the case that it has a very limited set of supported qedge types?

[edeutsch]

definitely a weird observation. I never really understood what it was there for. Maybe the answer is that it doesn't actually do anything. Would be nice to understand and fix.

[amykglen]

Maybe it would be more honest to have the KP name be just "arax-xdtd" instead of "infores:arax-xdtd", since the latter is not a real thing.

great point, @edeutsch. I just removed the infores prefixes for those (in master)

[amykglen]

I also notice that our NGD is almost always skipped -- is it the case that it has a very limited set of supported qedge types?

@isbluis - yeah, NGD is supposed to be used as a KP for qedges with the biolink:occurs_together_in_literature_with predicate, although I don't think it's been in a working state for a while. we weren't really using it at all (it had some serious limitations) so I let it slide a ways back... I've been meaning to just remove it from the code base. I'll create an issue about that... (#2176)

[edeutsch]

super, thanks!

[amykglen]

cool - can we close this issue? verified the traffic light appears as expected on /beta

[edeutsch]

I'm still seeing an infores prefix for NGD in /beta?

[amykglen]

yes, I only just merged that removal of the NGD KP to master at 10:17am, so I'm guessing that hasn't been deployed yet? (the NGD line won't appear at all anymore)

[amykglen]

also, it's possible the NGD traffic light won't disappear until the KP info cache refreshes (post-deployment)

[edeutsch]

I have redeployed the latest master on arax.ncats.io and this seems fixed, thanks!

I also saw in the logs:

Detected a stale KP in meta map (infores:arax-normalized-google-distance) - deleting it

which seems like a good sign.
Closing!