-
Notifications
You must be signed in to change notification settings - Fork 6
MAINT: migrated code for CheckedArray/Session to use pydantic v2 #1144
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
MAINT: migrated code for CheckedArray/Session to use pydantic v2 #1144
Conversation
gdementen
left a comment
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.
Thanks A LOT for tackling this. It would have taken me ages to do this myself. This looks very good.
There are a few error messages which now have redundant parts because of the new "Error while assigning value to variable X" part, but this can be improved later.
| will open a window and pause the running script until the window is closed by | ||
| the user. To revert to the previous behavior, use show=False. | ||
|
|
||
| * Using :py:obj:`CheckedParameters` or :py:obj:`CheckedParameters` now requires |
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.
There is probably a typo in there. CheckedArray? CheckedSession?
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.
Yes, it was a typo. I will change it to CheckedSession.
larray/core/checked.py
Outdated
|
|
||
| # Simplified version of the ModelMetaclass class from pydantic: | ||
| # https://github.com/samuelcolvin/pydantic/blob/master/pydantic/main.py | ||
| # https://github.com/pydantic/pydantic/blob/main/pydantic/_internal/_model_construction.py |
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.
Could you use a permalink or a link to a particular release? main/master tend to change over time and the link will probably break at some point.
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.
OK
| from pydantic._internal._decorators import DecoratorInfos | ||
| from pydantic._internal._namespace_utils import NsResolver | ||
| from pydantic._internal._fields import is_valid_field_name | ||
| from pydantic._internal._model_construction import (inspect_namespace, set_model_fields, |
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.
Ouch. That's a lot of internal api usage. I suppose there was no other way (honestly, I don't remember why we couldn't use stock pydantic previously), but we can only hope it stays stable 🤞.
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.
Indeed. What we could do for now is to fix the pydantic version to 2.12 in requirements.txt and in make_release.py ?
I do not remember why I implemented a simplified version of the ModelMetaclass. I think it was to allow unloaded members after the initialization of the checked session (when reading from a file). I was also to allow the declaration of members without a default value in CheckedParameters (defined using the frozen flag of pydantic.
In the __init__ method of the CheckedSession class, we have:
for name, field in self.__pydantic_fields__.items():
value = input_data.pop(name, NotLoaded())
if isinstance(value, NotLoaded):
if field.default is PydanticUndefined:
warnings.warn(f"No value passed for the declared variable '{name}'", stacklevel=2)
self.__setattr__(name, value, skip_frozen=True, skip_validation=True)
else:
self.__setattr__(name, field.default, skip_frozen=True)
else:
self.__setattr__(name, value, skip_frozen=True)
# --- undeclared variables
for name, value in input_data.items():
self.__setattr__(name, value, skip_frozen=True, stacklevel=2)Maybe the v2 version of pydantic has a special mechanism to write a sort of dedicated __init__ method or to skip validation and set frozen to False during the initialization process only. So that we could make CheckedSession to inherit from pydantic BaseModel. But that requires more investigation in the pydantic documentation plus the time to make some proof of concept implementation. But I don't have time for the moment. But I can open an issue for that and come back to it later if you want.
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.
Yes, that would be very much appreciated. This can come later though.
larray/tests/test_checked_session.py
Outdated
| assert diff.ano01 is ano01 | ||
| assert diff.c is c | ||
| assert diff.d is d | ||
| assert diff.b.equals(b) |
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.
do the instances actually change now or did you only change the tests? If the instance do not change, I would rather keep the "is" tests because that is a stronger test.
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.
Yes it seems that the instances change for some types (but not all). At least, they change for dict objects (but not the others). I can go back to is for all members except d (which is of type dict)
| tables # ==pytables | ||
| openpyxl | ||
| pydantic ==1.* | ||
| pydantic >=2.12 |
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.
Do you actually use 2.12 specific features or is it because you fear it does not work with earlier pydantic versions?
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.
Second option. It because you fear it does not work with earlier pydantic versions. I made changes in the code and I runned all the tests with pydantic 2.12 installed on my PC.
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 checked on conda (with conda search pydantic) that pydantic 2.12 is available for Python 3.9+ :
> conda search pydantic
(...)
pydantic 2.12.2 py39haa95532_0 pkgs/main
pydantic 2.12.3 py310haa95532_0 pkgs/main
pydantic 2.12.3 py310haa95532_1 pkgs/main
pydantic 2.12.3 py311haa95532_0 pkgs/main
pydantic 2.12.3 py311haa95532_1 pkgs/main
pydantic 2.12.3 py312haa95532_0 pkgs/main
pydantic 2.12.3 py312haa95532_1 pkgs/main
pydantic 2.12.3 py313haa95532_0 pkgs/main
pydantic 2.12.3 py313haa95532_1 pkgs/main
pydantic 2.12.3 py314haa95532_1 pkgs/main
pydantic 2.12.4 py310haa95532_0 pkgs/main
pydantic 2.12.4 py311haa95532_0 pkgs/main
pydantic 2.12.4 py312haa95532_0 pkgs/main
pydantic 2.12.4 py313haa95532_0 pkgs/main
pydantic 2.12.4 py314haa95532_0 pkgs/mainThere 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 did that check too 😉
fix #1075