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

add trapi version to Connections Hypothesis Provider #624

Closed
wants to merge 1 commit into from

Conversation

andrewsu
Copy link
Member

minor change to standardize with other entries, make it easier to parse out the x-bte meta-kg

@tokebe
Copy link
Member

tokebe commented Apr 13, 2023

This would bring our internal representation out of spec with the API name from SmartAPI, resulting in console warnings. We can still do it if the clarity is preferred.

@tokebe
Copy link
Member

tokebe commented Apr 13, 2023

On a related note, currently we specify COHD TRAPI 1.3 instead of the SmartAPI COHD TRAPI API, resulting in warnings.
Tagging @colleenXu for opinions.

@colleenXu
Copy link
Collaborator

colleenXu commented Apr 13, 2023

I'm not sure what @andrewsu's intent is: is it to change the API name in SmartAPI metaKG? I'm also not clear if the PR actually accomplishes this intent too...


My understanding is that the API names in the API_LIST objects work like the registry IDs in those objects....they identify the registrations of KP APIs that we want BTE to use. So our current practice of making these names exact matches to the info.title values (in the registered OpenAPI yamls) makes sense to me...

And for context:

  • I think if there's a mismatch between the API name in the API_LIST entry and the current info.title of the registration, BTE / its internal metaKG representation will use the current info.title. I'm not sure what the SmartAPI metaKG code does. Because of these two things, I'm not sure that this PR does what it's intended to do.
    • EDIT: yep, if I understand my local testing correctly, if there's a mismatch, BTE seems to use info.title and ignore the API name in the API_LIST entry. I changed the Automat CTD name, compiled, ran smartapi-sync (and checked the data files), and a one-hop query MONDO:0004766 -> PhenotypicFeature (and checked the TRAPI logs for what the API name was).
  • API names refer to the info.title values because we only used those to specify which registrations of KP APIs to use (aka we didn't use the registry IDs). But this led to problems when multiple registrations had the same info.title or registration owners changed their info.title...
  • I imagine refactoring would be needed if we had a different intent/purpose for the API name in the API_LIST entries...

@colleenXu
Copy link
Collaborator

colleenXu commented Apr 13, 2023

And for what @tokebe asked, it looks like the COHD owners changed the info.title of their registration and added a second registration....which is likely related to the TRAPI 1.4 migration process. I first noted this here

I'd prefer not having these warnings....so I guess a quick fix is to change the API name for COHD so it matches the current info.title. It looks like that's COHD TRAPI right now, but this may be in active flux?

But.....I don't want to confuse the original intent and stuff in this PR with the COHD stuff you're asking about...

@andrewsu
Copy link
Member Author

Yeah, I'd forgotten about the check we do relative to SmartAPI, a check that I still think is valuable. My intent was to have an easy and automated way to differentiate TRAPI APIs and x-bte APIs in the config file (for the purposes of creating a visualization of the BTE meta-kg). But I didn't think this one through enough -- not the right solution for that. I'd welcome alternate ideas on the goal above, but going to close this PR...

(And yes, creating another PR for the COHD change sounds good to me...)

@andrewsu andrewsu closed this Apr 14, 2023
@colleenXu
Copy link
Collaborator

colleenXu commented Apr 15, 2023

@andrewsu you have a point that we only use comments to differentiate the different "categories" of APIs we have in the config file.

I wonder if you have access to the SmartAPI yaml tags, since the TRAPI KPs will have the "trapi" tag and the APIs we use x-bte with won't...So, we actually don't have a tag for "this SmartAPI yaml has x-bte annotation"; and maybe it'd be useful to have this.

I think BTE does keep track of which APIs are using x-bte annotations, which are BioThings APIs, which are TRAPI APIs.....but I'm not fully sure of how or where it does that. I think there's some amount of parsing in https://github.com/biothings/biothings_explorer/blob/main/src/controllers/cron/update_local_smartapi.js

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.

3 participants