-
Notifications
You must be signed in to change notification settings - Fork 22
Bump Pydantic to v2+ #1285
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
base: main
Are you sure you want to change the base?
Bump Pydantic to v2+ #1285
Conversation
dd4c8f2
to
23fa39f
Compare
datalab
|
Project |
datalab
|
Branch Review |
bc/bump-pydantic
|
Run status |
|
Run duration | 07m 04s |
Commit |
|
Committer | Ben Charmes |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
0
|
|
0
|
|
336
|
View all changes introduced in this branch ↗︎ |
24080dc
to
23fa39f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1285 +/- ##
==========================================
- Coverage 78.49% 77.95% -0.55%
==========================================
Files 67 69 +2
Lines 4645 4885 +240
==========================================
+ Hits 3646 3808 +162
- Misses 999 1077 +78
🚀 New features to boost your workflow:
|
6edc151
to
46cc0c8
Compare
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.
Hi @BenjaminCharmes, here's some partial initial comments/suggestions. I'll try to deploy a system based on this PR from scratch (and also investigate some of the new incantations that you've had to use for pydantic 2 compat).
pydatalab/src/pydatalab/login.py
Outdated
if "_id" in user: | ||
user["immutable_id"] = str(user.pop("_id")) | ||
|
||
if "managers" not in user: | ||
user["managers"] = None | ||
|
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.
Note to self that I want to come back and verify this wrt. model aliases
Sample.model_rebuild() | ||
StartingMaterial.model_rebuild() | ||
Cell.model_rebuild() | ||
Equipment.model_rebuild() | ||
Collection.model_rebuild() |
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.
Sample.model_rebuild() | |
StartingMaterial.model_rebuild() | |
Cell.model_rebuild() | |
Equipment.model_rebuild() | |
Collection.model_rebuild() | |
{model.model_rebuild() for model in ITEM_MODELS.values()} |
Might be neater, though not sure why we need this now and not before (will play around with it later)
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.
Doesn't work with pre-commit for me, need to find out why.
It seems that this is necessary because Pydantic v2 is stricter with forward reference and circular dependencies
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.
Also need to keep Collection since it's not in ITEM_MODELS
pydatalab now start, but not working well pydatalab now start, but not working well pydatalab now start, but not working well pydatalab now start, but not working well pydatalab now start, but not working well Fix login with Pydantic v2+ Fix sample creation Fix sample creation Fix edit page
Fix pytest Fix pytest Fix pytest Fix pytest Fix pytest Fix pytest Fix pytest
Try fix .env PYDATALAB_TESTING Try fix .env PYDATALAB_TESTING Add temp. debug Add temp. debug Add temp. debug Add temp. debug Add temp. debug Add temp. debug Add temp. debug
66c974c
to
e5ebeb1
Compare
947d1a6
to
1cb7e5c
Compare
@model_validator(mode="before") | ||
@classmethod | ||
def add_missing_synthesis_relationships(cls, values): | ||
"""Add any missing sample synthesis constituents to parent relationships""" |
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've moved this whole module, but want to check this validator as it seems a bit excessive
@model_validator(mode="before") | ||
@classmethod | ||
def add_missing_collection_relationships(cls, values): | ||
if values.get("collections") is not None: |
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.
Likewise this validator
except Exception as e: | ||
LOGGER.exception(f"Error in get_item_data: {e}") | ||
return jsonify({"status": "error", "message": str(e), "error_type": type(e).__name__}), 500 |
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.
Better to avoid this -- there are app-level error handlers that will produce better error messages for all routes + it makes PRs hard to review
|
||
if contact_email or contact_email in (None, ""): | ||
if contact_email in ("", None): | ||
update["contact_email"] = None | ||
else: | ||
update["contact_email"] = EmailStr(contact_email) | ||
if "@" not in contact_email or len(contact_email) > 1000: |
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.
Things like this really need to be handled at the validator level
Update Pydantic from v1.10 to v2+
Also closes #1282