-
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
useful but not critical: ability to do "scrolling" GET queries with v3 Monarch API #789
Comments
@rjawesome Jackson said this may be an issue you could handle. It's not high priority though, if you have other tasks. |
I added a special case to the pagination logic to support Monarch. I also added a feature where pagination can be specified through SmartAPI yamls. For example, for the MonarchAPI the appropriate lines to be added to the yaml for a given operation would be: parameters:
...
limit: 500
offset: "{{ start }}" # {{ start }} is a special field in templating to denote what numbered entry the pagination is at ( similar to {{ queryInputs }})
...
pagination:
countField: items # number of hits from this response, can point to an array (length is taken) or a numerical field
totalField: total # field in response containing total number of hits from this request
pageSize: 500
... (these don't need to be added for Monarch since I implemented it as a special case similar to biothigns) |
@colleenXu I believe the next step for this issue would be for you to review/approve @rjawesome's interface in the SmartAPI yamls. If that interface makes sense, I can start testing and then add to the dev branch/instance. |
That x-bte annotation looks okay from my POV and looks simple/generic enough to work for other external API cases. Unfortunately, we don't have any such cases right now, so I can't back up my words :P. This is my understanding, but maybe @rjawesome can clarify:
@tokebe Do you want to switch Monarch over to the x-bte annotation method, or are you just testing this new feature out? It sounds like @rjawesome implemented Monarch as a special case without using the new feature with SmartAPI-yaml/x-bte annotation editing... |
I got a 500 error while testing this: I ran locally in CI mode ( In the console logs below, I removed the Sentry logs. TRAPI query used for testing, should get 2257 records from Monarch
Based on this query to Monarch API
The pagination logs look good. It does several rounds to get all 2257 records
But then the 500 error comes up after scoring
But this smaller query (only 1 round of pagination) ran successfully without errors: TRAPI query, should retrieve 506 records from Monarch
from this comment
Console logs retrieving all 506 records
|
This appears to happen regardless of the new subclassing code, I'm looking into it. |
Turns out our HP and MONDO ontologies contain id prefixes that are not HP or MONDO, respectively, which breaks our ontology source detection. I'm going to have to make some changes to node-expansion to return the specific source of each expanded ID, because there's overlap. |
Context: the error revealed a bug in node-expansion, coming from MONDO ontology having non-MONDO ID info - including mismatched ID namespaces for parent-child pairs. HP has this issue too. The first test query used an UBERON ID, which had some child info in those files with a different ID namespace. The solution is to remove those mismatched pairs. Jackson has put a fix into the |
Things look good! I retested using the
|
Relevant changes deployed to Prod. |
In #774, we migrated to the new v3 Monarch API (using the /association endpoint). This is one of the follow-up tasks that would be useful to do.
The endpoint seems to only allow us to retrieve 500 items at a time (I encountered error 500 (Internal Service Error) when trying to set the limit parameter to > 500 or to
-1
.-1
worked w/ the old API to return all hits).Being able to retrieve all the items in the response would be nice, as "scrolling" GET queries. The API responses have a
total
field that tells us how many total items there are. Ex: this has > 2000 items.And there is an
offset
parameter for requests, that we could use to retrieve the next 500 items, and the next, until we've retrieved all the items.Jackson says there should be code to handle scrolling GET queries already, it just needs to be applied (and customized?) for v3 Monarch API.
They also said that it may be helpful to have an operation-level flag (like
supportBatch
) that flags operations where we want to do these scrolling GET queries.On thing I'm unsure of is if all cases can be handled exactly the same way though (ex: same parameter name, same field the response to look for).
The text was updated successfully, but these errors were encountered: