Fix logic on adding is_set:true to QNodes during ID/node-expansion

[colleenXu]

Copied from internal lab Slack thread. See comments on this issue for the rest of the Slack discussion + decisions on what to do (made during today's lab meeting)

---

I notice that is_set: true is being added to non-creative-mode queries at times that I don't expect it.

For example, if I run the query below, I get "merged" results like the screenshot, even though I expected two separate results (one for KDR and one for TNK2).

Query

POST to dev https://api.bte.ncats.io/v1/smartapi/8f08d1446e0bb9c2b323713ce83e2bd3/query

{
    "message": {
        "query_graph": {
            "nodes": {
                "n0": {
                    "ids":["UniProtKB:P35968", "UniProtKB:Q07912"],
                    "categories":["biolink:Gene"]
                },
                "n1": {
                    "categories":["biolink:SmallMolecule"]
               }
            },
            "edges": {
                "e1": {
                    "subject": "n0",
                    "object": "n1"
                }
            }
        }
    }
}

It looks like the is_set: true is being added to the query_graph during the ID/node-expansion step, even though NO descendant IDs were found and added during that step. So maybe the logic needs tweaking?

[colleenXu]

Rest of the Slack discussion:

Jackson:
This appears to be an intentional changes for node expansion, code here, commit here. I wasn't too involved in the decisionmaking for this feature so I'm not sure if that's fully intended behavior or not across the board...but I did find the issue comment where the decision was made. It might be that this was supposed to be applied in a more limited scope?

andrew:
yeah, I can see how the potential logic here gets a little stickier if we specify multiple IDs in the query. For example, suppose n0 specified ids A1 and B1, and those got expanded to include A2 and B2. Would we only want merging between A1 and A2 and separately between B1 and B2 ? The bookkeeping logic to accomplish that might be significantly more complex... I think this would be a good candidate for discussion on our Wed meeting...

[colleenXu]

Today's meeting discussion + decisions:

A. decided to change the current logic to:

if IDs actually get expanded for QNode X (new set size > old set size):
    Set QNode X to is_set: True (overrides anything else)
else:
    Go with what is already on QNode X

B. Going to set-aside-for-now: how to keep track of which original ID each expanded ID came from.

• related to the old NodeBinding.query_id discussions (see Part B and later comments here)
• What Andrew said above on having different results for each original ID+its descendants
  • CX's view is that this idea makes some sense but isn't necessary. It also gets complicated if original IDs share descendants, which was also discussed in the Part B of the linked issue

[tokebe]

See biothings/bte_trapi_query_graph_handler#134

[tokebe]

Deployed to prod 🚀