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

feat!: update /search/studies endpoint #279

Merged
merged 50 commits into from
Mar 28, 2024
Merged

feat!: update /search/studies endpoint #279

merged 50 commits into from
Mar 28, 2024

Conversation

korikuzma
Copy link
Member

@korikuzma korikuzma commented Jan 4, 2024

close #273 . I will open once #266 is merged

@jsstevenson again, fresh eyes would be helpful for this 😄

Notes:

  • This PR updates the previously named /search/statements endpoint. Will address Move variant.definingContext.expressions to variant.members #309 at a later time. This PR is to kind of mirror what was done previously, so I'd like to address "major" response changes in separate issues if possible.
  • There are some places where I should use enums. I will do this in Create enums for database node labels and relationships #270 or a separate issue. I opened an issue in VRS-Python to create str enums
  • For input variation, I decided to query based on a definingContext or a members match
  • The way that identifiers are computed in VRS-Python made it so that our FastAPI endpoints only return the minimal information needed (ga4gh keys) to create a digest. See more in add back missing models to openapi.json #305 . I haven't had time to dive much deeper into this issue

Changes:

* Update database nodes and relationships
  * Dropped Variation Group. We can add back in new issue if we want to retain this
* Update versions for disease-normalizer, civicpy, pydantic, and neo4j 
* Changed fixture scope in `tests/conftest.py` to `session`
* fix: typo in CODING_KEYS
* change: study now has alleleOrigin property with HAS_GENE_CONTEXT rel
@korikuzma korikuzma added enhancement New feature or request priority:high High priority labels Jan 4, 2024
@korikuzma korikuzma self-assigned this Jan 4, 2024
@korikuzma korikuzma marked this pull request as draft January 4, 2024 19:00
@korikuzma
Copy link
Member Author

I think I will wait to open until @bwalsh and @ahwagner give feedback on dev instance.

After initial talk with @ahwagner , I should move variant.definingContext.expressions into variant.members. For CIViC variants, the genomic hgvs expression should already be there.

Screenshot 2024-01-08 at 10 03 41 AM

@korikuzma
Copy link
Member Author

The last commit added support for providing genomic variation query. At the moment, only CIViC studies will be returned. Once #283 is complete, MOA assertions should also be returned

@korikuzma
Copy link
Member Author

korikuzma commented Feb 2, 2024

Something is happening with the civic.eid:2880 evidence item and associated queries such as NC_000013.10:g.32890600G>T and BRCA2 M1I. We are getting a No studies found with the provided query parameters. in the warnings, but the study_ids field has civic.eid:2880 in it. I will look into this later.

Using 20240103

@korikuzma korikuzma added enhancement New feature or request and removed priority:high High priority labels Mar 27, 2024
@korikuzma
Copy link
Member Author

I think I will wait to open until @bwalsh and @ahwagner give feedback on dev instance.

After initial talk with @ahwagner , I should move variant.definingContext.expressions into variant.members. For CIViC variants, the genomic hgvs expression should already be there.

Screenshot 2024-01-08 at 10 03 41 AM

Made new issue for this #309

* start can be 0, so needed to do null check
@korikuzma korikuzma changed the base branch from epic-231 to fix-db-keys March 27, 2024 12:11
@korikuzma
Copy link
Member Author

Something is happening with the civic.eid:2880 evidence item and associated queries such as NC_000013.10:g.32890600G>T and BRCA2 M1I. We are getting a No studies found with the provided query parameters. in the warnings, but the study_ids field has civic.eid:2880 in it. I will look into this later.

Using 20240103

#310 fixes this

@korikuzma korikuzma marked this pull request as ready for review March 27, 2024 14:37
Base automatically changed from fix-db-keys to epic-231 March 27, 2024 18:07
Copy link
Member

@jsstevenson jsstevenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks good

metakb/query.py Show resolved Hide resolved
metakb/query.py Outdated Show resolved Hide resolved
metakb/query.py Outdated Show resolved Hide resolved
@korikuzma
Copy link
Member Author

korikuzma commented Mar 28, 2024

@jsstevenson applied your suggestions. I also don't love the ordering of methods. I can re-order after you review latest changes

jsstevenson
jsstevenson previously approved these changes Mar 28, 2024
Copy link
Member

@jsstevenson jsstevenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@jsstevenson jsstevenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah seems reasonable

@korikuzma korikuzma merged commit 4803be3 into epic-231 Mar 28, 2024
@korikuzma korikuzma deleted the issue-273 branch March 28, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:medium Medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants