-
Notifications
You must be signed in to change notification settings - Fork 13
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
DM-40002: Try to support pydantic v1 and v2 #866
Conversation
We always tend to install the optionals for testing but it turns out the import tests were not working properly. Consider removing S3 completely.
Somehow pydantic v1 did not notice.
Still some failures. Lots of deprecation warnings.
AstropyArrow and Dataframe use classes that are not part of the core butler dependencies. This leads to the tests failing in a minimal environment. Replace with two storage classes that are available to the core environment.
This enables pydantic v2
d50053e
to
a4fcc53
Compare
This is meant to be the rquivalent of using __setattr__ and __field_set__ directly. The latter has been renamed in pydantic v2
This all worked before but now we have to force the Config to a plain dict before it will be accepted.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #866 +/- ##
==========================================
- Coverage 87.94% 87.72% -0.22%
==========================================
Files 273 274 +1
Lines 35896 35937 +41
Branches 7510 7537 +27
==========================================
- Hits 31567 31526 -41
- Misses 3170 3241 +71
- Partials 1159 1170 +11
☔ View full report in Codecov by Sentry. |
There is clearly some issue with the RootModel constructor that we will need to sort out.
This required that we allow unused igores since the location of the ignores depends on the version of pydantic and mypy always picks up the first definition it sees even if that is a pydantic V2 code path and pydantic is v1.
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.
Looks OK after a quick look, one question is whether _BaseModelCompat.model_construct
could be changed to avoid if/else in many model classes.
from moto import mock_s3 # type: ignore[import] | ||
except ImportError: | ||
boto3 = None | ||
|
||
def mock_s3(cls): # type: ignore[no-untyped-def] | ||
def mock_s3(*args: Any, **kwargs: Any) -> Any: # type: ignore[no-untyped-def] |
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'd expect you can drop type: ignore
here?
@andy-slac you are right that I could implement v1 model_construct in _BaseModelCompat and just use it in the same way for both pydantic. Then I could even make v2 do the explicit setter approach if we decided we didn't want to use the base v2 base class model_construct. I will change. |
If the BaseModel.construct() method is slow we can still reimplement it as was done in the individual direct() methods but with the direct() methods still being much simpler.
The previous version used by the direct() methods would be something like: def model_construct(cls, _fields_set: set[str] | None = None, **values: Any) -> Self: node = cls.__new__(cls) setter = object.__setattr__ for k, v in values.items(): setter(node, k, v) if _fields_set is None: _fields_set = set(values.keys()) setter(node, "__fields_set__", _fields_set) return node Testing with 100MB graph indicates that construct() might be fractionally slower than implementing it ourselves as above but it is not conclusive. The direct() method only accounts for 14% of the graph load time.
Checklist
doc/changes