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

TranQL query optimization, loading error/state improvements, variable view color fix #325

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

frostyfan109
Copy link
Collaborator

  • 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)

Copy link
Contributor

@gaurav gaurav left a 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)
Copy link
Contributor

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?

Copy link
Collaborator Author

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}"`,
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

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.

2 participants