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

Replaced json with orjson for large size loads #662

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

nevoodoo
Copy link
Collaborator

Description

This is based off #658 which proposes to switch to orjson in place of the standard json library, for deserialising values that might be large. This switch should offer a significant cut in loading times, without introducing any code complexity.

Changes Made

  • Replaced json.loads calls with orjson.loads where deemed appropriate. This replacement has not been done on low-impact calls that deserialise small objects.

Notes

  • We should probably use orjson when introducing any new features/fixes that involving deserialising of large objects.

Closes

@nevoodoo nevoodoo requested a review from illusional January 25, 2024 04:11
Copy link
Collaborator

@illusional illusional left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Note my 2 review comments before merging, but broadly supportive of this change. I was going to suggest looking at replacing it for Pydantic (I don't actually know which version we use), but looks like there is some ongoing discussion here: pydantic/pydantic#6388

So maybe best to leave that for now.

openapi-templates/api_client.mustache Show resolved Hide resolved
SearchItem,
parse_sql_bool,
)
from models.models import (AssayInternal, FamilySimpleInternal,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a fun linting one, soon to be resolved in metamist, but your default isort profile in VSCode is incorrect, in your VSCode settings.json you can add:

    "isort.args": [
        "--profile=black"
    ],

We intend to add this to metamist's pyproject.toml soon (to replicate https://github.com/populationgenomics/cpg-python-template-repo/blob/6e2da21e1012e6d2b15ba0c385da579a9b80b550/pyproject.toml#L7), but we were waiting for other big PRs to be finalised before introducing lots of merge conflicts.

Copy link
Collaborator Author

@nevoodoo nevoodoo Jan 25, 2024

Choose a reason for hiding this comment

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

Reverted and fixed! I assume that replication to metamist will happen along with the new pre-commit hooks?

@nevoodoo
Copy link
Collaborator Author

nevoodoo commented Jan 25, 2024

Awesome stuff! Note my 2 review comments before merging, but broadly supportive of this change. I was going to suggest looking at replacing it for Pydantic (I don't actually know which version we use), but looks like there is some ongoing discussion here: pydantic/pydantic#6388

So maybe best to leave that for now.

I'm in this rabbit hole now since the discussion leads to mgspec which reports to be even quicker than orjson as per their benchmarks. I'll hold off on merging this PR until I've done some more tests to see what the better option is I think. I see Pydantic are also having discussions with the creator of mgspec so there's a good chance they may borrow from that library instead. Thoughts?

@illusional
Copy link
Collaborator

I don't feel well versed with the different python libraries, looks like the conversation is pretty fresh so happy to wait - (what version of Pydanctic do we use?)

@nevoodoo
Copy link
Collaborator Author

I don't feel well versed with the different python libraries, looks like the conversation is pretty fresh so happy to wait - (what version of Pydanctic do we use?)

Believe fastapi==0.85.1 and pydantic==1.10.13 from circa 2022. Current versions are fastapi==0.109.0 and pydantic==2.5.3

@illusional
Copy link
Collaborator

Awesome, I tried to bump our fastapi, but there's an issue with the openapi version and the generator we use #643 (I think this issue has more commentary).

Is this PR good to merge if we don't want to touch that side?

@nevoodoo
Copy link
Collaborator Author

nevoodoo commented Feb 1, 2024

Awesome, I tried to bump our fastapi, but there's an issue with the openapi version and the generator we use #643 (I think this issue has more commentary).

Is this PR good to merge if we don't want to touch that side?

I think this should be good to merge for now. mgspec is still relatively new so maybe it's something to be looked at again in the future :)

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.

Use orjson in place of json
2 participants