Return 0 results rather than throw an error?

[dkoslicki]

Via Ryan Roper (Multiomics Provider): the following DSL returns an error No pathways were found in {'BTE,'RTX-KG2'} satisfying qedge e03:

add_qnode(key=n0, categories=biolink:Gene, is_set=True)
add_qnode(key=n1, ids=CHEBI:5280, categories=biolink:ChemicalSubstance)
add_qedge(key=e01, subject=n0, object=n1, predicates=biolink:related_to)
add_qnode(key=n2, ids=MONDO:0020066, categories=biolink:Disease)
add_qedge(key=e02, subject=n0, object=n2, predicates=biolink:related_to)
add_qnode(key=n3, categories=biolink:ChemicalSubstance)
add_qedge(key=e03, subject=n0, object=n3, predicates=biolink:related_to)
expand()
resultify()
filter_results(action=limit_number_of_results, max_results=50, prune_kg=true)

Perhaps we should instead return 0 results (and potentially whatever happens to be in the message KG up to that point), instead of throwing an error? Thoughts @saramsey @edeutsch @amykglen ?

[edeutsch]

Here's what comes back in TRAPI snippets (ellipses manually inserted for brevity):

{
  "message": {
    "results": [],
    "query_graph": {
      "nodes": {
...
      },
      "edges": {
...
      }
    },
    "knowledge_graph": {
      "nodes": {},
      "edges": {}
    }
  },
  "status": "NoResults",
  "description": "No paths were found in {'BTE', 'RTX-KG2'} satisfying qedge e03",
  "logs": [
...
    {
      "timestamp": "2021-06-15T18:47:24.117817",
      "level": "DEBUG",
      "code": "",
      "message": "After pruning, KG counts are: e01: 3, e02: 1, n0: 1, n1: 1, n2: 1"
    },
    {
      "timestamp": "2021-06-15T18:47:24.117890",
      "level": "ERROR",
      "code": "NoResults",
      "message": "No paths were found in {'BTE', 'RTX-KG2'} satisfying qedge e03"
    }
  ],
  },
  "reasoner_id": "ARAX",
  "tool_version": "ARAX 0.8.0",
  "schema_version": "1.1.0",
  "datetime": "2021-06-15 18:47:19"
}

Technically if Response.status is anything but "OK", it is an error. And I think processing stopped at expand()

The final log event is set to level ERROR, which halts processing by the DSL processor. If this last logEvent were set to WARNING, then I think the system would continue.

If expand() issues just a warning, but it would be up to resultify() and filter_results() not to throw an error despite having nothing to do. If they all complete with just warnings, then I think the final result would be Response.status='OK'.

[amykglen]

yeah, in the early days of ARAX I think the team decided throwing an error vs. returning 0 results was best, although I like the idea of just returning 0 results. and I think resultify actually already handles an empty KG fine (returns 0 results).

[dkoslicki]

From how the standups are going, it appears that showing 0 results rather than an error is more desirable (and also from a UX perspective, imho). Shall we move to that @amykglen ?

[amykglen]

sounds good to me! will do.

[amykglen]

as part of this issue I'm thinking we could get rid of expand's continue_if_no_results parameter. but I guess we could also just change its default to true, rather than get rid of it.

is anyone against getting rid of that parameter?

[dkoslicki]

No objections from me!

[edeutsch]

agreed, no objections.

[amykglen]

great! I removed that parameter and made it so that expand just logs a warning (vs. error) when no KPs find answers. I think all modules are good to go in terms of this new behavior (resultify and filter already seemed fine with it, and I added a little check that I think takes care of overlay). changes are in master, all fast and slow tests passing.

might be worth @finnagin making sure that filter_kg can handle partial KGs (where the KG isn't empty, but it might not have any nodes/edges fulfilling the qnode(s)/qedge(s) the filter was requested on).

[amykglen]

this is looking good to me on production. e.g.: https://arax.ncats.io/?r=12975

look good to you, @dkoslicki?

[finnagin]

Looks good to me!

So currently filter_kg does error when you put in an attribute that does not exist in the kg. I could change it to a warning instead but I thought we wanted it as an error for some reason. Was that correct @dkoslicki or should I go ahead and change it to a warning?

[dkoslicki]

Let's go ahead and put it as a warning: the motivation behind an error was to catch things like misspelling attribute names (eg. semeddb) so the user wouldn't think the attribute was filtered out when it actually wasn't. But I guess we can put a bit more responsibility on the user to verify what they asked for. @finnagin is this ok to close, or should I wait until the filter_kg warning has been implemented?

[finnagin]

Ok I'll go ahead and do that then. Feel free to close this. I'll open a new issue for the warning change.