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

Updating to Pydantic v2 #495

Merged
merged 16 commits into from
Jan 10, 2024
Merged

Updating to Pydantic v2 #495

merged 16 commits into from
Jan 10, 2024

Conversation

glass-ships
Copy link
Collaborator

Closes #260

Summary

  • Updates Pydantic to v2.5, along with some other packages to allow this
  • Updates fixtures, scripts, and migrate related code
  • Updates tests
    • curie expansion fails: on omim, ensembl, uberon, and wormbase (potentially unrelated?)

Copy link

netlify bot commented Dec 6, 2023

Deploy Preview for monarch-app ready!

Name Link
🔨 Latest commit 299ef2c
🔍 Latest deploy log https://app.netlify.com/sites/monarch-app/deploys/659e17a169ed0a000870c73c
😎 Deploy Preview https://deploy-preview-495--monarch-app.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@glass-ships
Copy link
Collaborator Author

curie expansion fails on: omim, ensembl, uberon, and wormbase (returns None)
@cthoyt do you by chance have an idea why this would be the case?
related code: https://github.com/monarch-initiative/monarch-app/blob/pydantic-v2/backend/src/monarch_py/service/curie_service.py

@cthoyt
Copy link
Contributor

cthoyt commented Dec 6, 2023

I can suggest you update the tests to use strict=True in the expand call then debug how prefix maps is constructing the EPM. This seems like a data and not a code issue

@glass-ships
Copy link
Collaborator Author

interesting, yeah...

        prefix, identifier = self.parse_curie(curie)
        rv = self.expand_pair(prefix, identifier)
        if rv:
            return rv
        if strict:
>           raise ExpansionError(curie)
E           curies.api.ExpansionError: WormBase:WBGene00000001

not sure where to start digging to sus this out

@cthoyt
Copy link
Contributor

cthoyt commented Dec 7, 2023

@glass-ships I wrote a minimal test that shows that there is no WormBase prefix available through the "merged" context in prefixmaps https://gist.github.com/cthoyt/ef44b41c11da966cfde322a9fac26859.

Here's the situation:

  1. The source file for the v0.2.0 release, merged.csv, has several different related records Screenshot 2023-12-07 at 09 40 38 that differ in http vs. https, but this shouldn't cause the issue

  2. On a first pass, I thought this was related to an issue with the merging algorithm (which is definitely apparent from this screenshot), but I don't think this is the case. Some context about that thought process:

    I think there is an assumption that this situation does not happen - there should only be one "canonical" record with a given namespace. Therefore, this seems like an issue with the construction of the merged context.

    Discussion on Add tests for integrity of merged contexts linkml/prefixmaps#26 is also relevant - there, I suggested to apply some data integrity testing to the merged contexts

  3. On a second thought, this seems like an even weirder error since WormBase isn't getting loaded at all in the converter (see the notebook). I still think this is a data issue and is not coming from curies

@glass-ships
Copy link
Collaborator Author

maybe a silly question but, when you say data error, what data in particular do you refer to?

@cthoyt
Copy link
Contributor

cthoyt commented Dec 7, 2023

The issue is happening because the wrong data is inside the curies.Converter object, not because there is a bug in the curies code. This means we need to look in the prefixmaps code or data to figure out why the right stuff doesn't make it inside the object that you're actually interacting with

@kevinschaper kevinschaper merged commit 2e3e930 into main Jan 10, 2024
10 checks passed
@kevinschaper kevinschaper deleted the pydantic-v2 branch January 10, 2024 17:16
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.

Updating to Pydantic V2
3 participants