-
Notifications
You must be signed in to change notification settings - Fork 6
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
TranQL query optimization, loading error/state improvements, variable view color fix #325
base: develop
Are you sure you want to change the base?
Conversation
frostyfan109
commented
Oct 28, 2024
- Related concept & related variable queries are now performed as a single TranQL query per CDE. Before, related studies required 1 query and related concepts required 5 queries for a total of 6 queries per CDE.
- Adds loading/error states to the studies and knowledge graph tabs in the concept model. Previously, it seemed they were relying on their API requests completing fairly quickly and not failing.
- Fixes coloring of study sources in the variable view for certain searches.
- Fixes context to support capitalized "CDE" study/data sources (currently "cde" for both)
…source tags, small bug fixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the technical depth to evaluate these changes, but I was able to run the code and confirm that things seem to be working well (and very fast!) and only have two minor comments. Good job!
if (e.name !== "CanceledError") throw e | ||
if (e.name !== "CanceledError") { | ||
console.warning(e) | ||
setStudies(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be useful to propagate the exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this scenario. The exception is "implicitly" handled by setting the state to null, indicating an error state.
{ | ||
headers: { 'Content-Type': 'text/plain' }, | ||
method: 'POST', | ||
body: `SELECT publication->named_thing FROM "redis:" WHERE publication="${cdeId}"`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful that this could pick up (1) things that aren't concepts, or (2) concepts that we haven't loaded. For example, when I tried running this locally, I found the related concept "Rheum", which we haven't actually loaded, so trying to select it (via "Open") resulted in a synonymousConcepts is undefined
error.
I prefer the approach we were using previously, which is to limit this query to certain types (['disease', 'anatomical_entity', 'phenotypic_feature', 'biological_process']
in https://github.com/helxplatform/helx-ui/pull/325/files#diff-e11890aa96a2d27536af10ab83b21362d569d6467f13afd9f6792b8196bd65afL2400). I'm not sure if this will fix this problem entirely, but I think it would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good point. I considered that it could possibly lead to unindexed concepts from unrestricting the types, but I didn't find in an example of such a thing happening during my testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll note that querying against these specific types in tranql isn't going to fix the issue of getting results back that aren't indexed in dug. This can be done by checking the concept returned by tranql is linked to a study variable, but this also isn't particularly practical. There was a bug with the concept lookup mechanism (used here) which I've fixed now, so you shouldn't be seeing that error anymore.
Really, what needs to happen is the addition of a new Dug endpoint that let's you pull any documents out of the concept index by ID. Then we could do a subsequent lookup against the results returned by tranql and filter out any that aren't indexed. It's just that this feature isn't particularly prominent so the work hasn't been done.