-
Notifications
You must be signed in to change notification settings - Fork 11
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
SmartAPI annotation + bug-chasing: ORPHANET -> orphanet #640
Comments
The PR allows lowercase orphanet (or uppercase ORPHANET) to be used in input curies. However the output from bte is still uppercase OPRHANET due to node normalizer output. |
I just tested, and the linked, merged PR and current main-branch code don't seem to address this issue. Here's how I'm testing:
contents of biothings_explorer/src/config/smartapi_overrides.json
query for testing
console logs when ORPHANET is used
console logs when orphanet is used: sub-query not generated properly
|
See new PR |
Still doesn't seem to work, when I test, records are dropped during the "edge-management" step. @rjawesome I'd like to pause the PR / coding work, and discuss first (see next post). console logs
|
Would you say this is mainly happening because of the NodeNorm output for the ID being (I may not understood your earlier post >.<) If "yes, the main issue is NodeNorm output", then I can raise this issue to NodeNorm / biolink-model folks. It may be more an issue of their output than a bug in our tool's behavior... |
From what I'm seeing, It seems like that is the main issue and it should be fixed if NodeNorm fixes their capitalization. However, I still think it would be better for BTE to be case insensitive so that it is overall easier to use. |
Okay, I'll raise this as an issue for Node Norm / biolink-model tomorrow. On your second point on "case insensitive"...it seems like there are multiple ways to define this:
|
|
Related to #591 |
Update! NodeNorm is rolling out an update that will change ORPHANET -> orphanet in their responses.
EDIT, NOTE: I'm not sure if the NodeNorm/prefix change will break any of BTE's tests. I see some test info in bte-server that has |
first two (mydisease, biothings rare-source) are for biothings/biothings_explorer#640 last one (ctd) is for https://github.com/biothings/biothings_explorer/issues/584\#issuecomment-1842376052
We've decided to use overrides to implement the x-bte changes as the NodeNorm update rolls out.
Jackson said he plans to work on the "BTE using instance-specific NodeNorm" feature
|
Update: we're using overrides for the 3 KPs that have orphanet IDs (mydisease, biothings rare-source, ComplexPortal) -> see this commit I think we can close this issue once:
We'll then have a separate process to remove the overrides (not needed once the yaml PRs are all merged / registrations refreshed). |
I double-checked and it's not working on CI, probably because of the larger cache-update issues (recent lab Slack convo) My test
POST to MyDisease through BTE CI
Right now, it seems like the MetaEdges aren't successfully turned into sub-queries. This could be because NodeNorm CI is using I only see the logs in the TRAPI response
|
Issue should now be addressed by 3019cec, please test again |
Now it's working on BTE CI! Yay! EDIT: And while we are deploying this to Test soon, it may not work until NodeNorm Test gets the orphanet prefix update... |
I've confirmed that things work as-expected after the Prod deployment. Closing issue, updating the registered yamls and registrations, and opening another issue for removing the overrides. Example: POST to https://bte.transltr.io/v1/smartapi/671b45c0301c8624abbd26ae78449ca2/query, will get a response with results
|
biolink-model folks say the prefix should be
orphanet
, not theORPHANET
that we've been using. See biolink/biolink-model#1198MyDisease and BioThings RARe-SOURCE are the two APIs we have that use this ID-namespace. Earlier I tried changing MyDisease to use the all-lowercase prefix (oops mixed into this commit), but I encountered issues when testing and decided to revert it back.
While the issue could be the x-bte annotation, it could also be a bug in BTE or an issue with the SRI Node Normalizer response. The SRI Node Normalizer currently uses the all-CAPS prefix, but it seems to be case-agnostic for the input so I'm not sure what's going on...
The text was updated successfully, but these errors were encountered: